Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options

[Michael Paquier]

On Sun, Oct 06, 2019 at 03:47:46PM +0300, Nikolay Shaplov wrote:
> This message is follow up to the "Get rid of the StdRdOptions" patch thread:
> https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m
> 
> I've split patch into even smaller parts and commitfest want each patch in 
> separate thread. So it is new thread.

Splitting concepts into different threads may be fine, and usually
recommended.  Splitting a set of patches into multiple entries to ease
review and your goal to get a patch integrated and posted all these
into the same thread is usually recommended.  Now posting a full set
of patches across multiple threads, in way so as they have
dependencies with each other, is what I would call a confusing
situation.  That's hard to follow.

> The idea of this patch is following: If you read the code, partitioned tables 
> do not have any options (you will not find RELOPT_KIND_PARTITIONED in 
> boolRelOpts, intRelOpts, realRelOpts, stringRelOpts and enumRelOpts in 
> reloption.c), but it uses StdRdOptions to store them (these no options).

I am not even sure that we actually need that.  What kind of reloption
you would think would suit into this subset?
--
Michael


signature.asc
Description: PGP signature

Re: Change atoi to strtol in same place

[David Rowley]

On Mon, 7 Oct 2019 at 18:27, Ashutosh Sharma  wrote:
> AFAIU from the information given in the wiki page -[1], the port
> numbers in the range of 1-1023 are for the standard protocols and
> services. And there is nowhere mentioned that it is only true for some
> OS and not for others. But, having said that I've just verified it on
> Linux so I'm not aware of the behaviour on Windows.
>
> [1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers

Here are the results of a quick test on a windows machine:

L:\Projects\Postgres\install\bin>echo test > c:\windows\test.txt
Access is denied.

L:\Projects\Postgres\install\bin>cat ../data/postgresql.conf | grep "port = "
port = 543  # (change requires restart)

L:\Projects\Postgres\install\bin>psql -p 543 postgres
psql (11.5)
WARNING: Console code page (850) differs from Windows code page (1252)
 8-bit characters might not work correctly. See psql reference
 page "Notes for Windows users" for details.
Type "help" for help.

postgres=#

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Change atoi to strtol in same place

[Ashutosh Sharma]

On Mon, Oct 7, 2019 at 10:40 AM David Rowley
 wrote:
>
> On Mon, 7 Oct 2019 at 13:21, Joe Nelson  wrote:
> >
> > Ashutosh Sharma wrote:
> > > One suggestion: The start value for port number is set to 1, however
> > > it seems like the port number that falls in the range of 1-1023 is
> > > reserved and can't be used. So, is it possible to have the start value
> > > as 1024 instead of 1 ?
> >
> > Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
> > so the range can be updated in one place for all utilities.
>
> (I've only had a very quick look at this, and FWIW, here's my opinion)
>
> It's not for this patch to decide what ports clients can connect to
> PostgreSQL on. As far as I'm aware Windows has no restrictions on what
> ports unprivileged users can listen on. I think we're likely going to
> upset a handful of people if we block the client tools from connecting
> to ports < 1024.
>

AFAIU from the information given in the wiki page -[1], the port
numbers in the range of 1-1023 are for the standard protocols and
services. And there is nowhere mentioned that it is only true for some
OS and not for others. But, having said that I've just verified it on
Linux so I'm not aware of the behaviour on Windows.

[1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers

> > > Further, I encountered one syntax error (INT_MAX undeclared) as the
> > > header file "limits.h" has not been included in postgres_fe.h or
> > > option.h
> >
> > Oops. Added limits.h now in option.h. The Postgres build accidentally
> > worked on my system without explicitly including this header because
> > __has_builtin(__builtin_isinf) is true for me so src/include/port.h
> > pulled in math.h with an #if which pulled in limits.h.
> >
> > > Alvaro Herrera  wrote:
> > > > The wording is a bit strange.  How about something like pg_standy:
> > > > invalid argument to -k: %s
> >
> > I updated the error messages in pg_standby.
> >
> > > > >   *error = psprintf(_("could not parse '%s' as integer"), str);
> > > >
> > > > ... except that they would rather be more explicit about what the
> > > > problem is.  "insufficient digits" or "extraneous character", etc.
>
> This part seems over-complex to me. What's wrong with just returning a
> bool and if the caller gets "false", then just have it emit the error,
> such as:
>
> "compression level must be between %d and %d"
>
> I see Michael's patch is adding this new return type, but really, is
> there a good reason why we need to do something special when the user
> does not pass in an integer?
>
> Current patch:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
>
> I propose:
> $ pg_dump -Z blah
> compression level must be an integer in range 0..9
>
> This might save a few round trips, e.g the current patch will do:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
> $ pg_dump -Z 12345
> invalid compression level: 12345 is outside range 0..9
> $ ...
>
> Also:
>
> + case PG_STRTOINT_RANGE_ERROR:
> + *error = psprintf(_("%s is outside range "
> + INT64_FORMAT ".." INT64_FORMAT),
> +   str, min, max);
>
> The translation string here must be consistent over all platforms. I
> think this will cause issues if the translation string uses %ld and
> the platform requires %lld?
>
> I think what this patch should be really aiming for is to simplify the
> client command-line argument parsing and adding what benefits it can.
> I don't think there's really a need to make anything more complex than
> it is already here.
>
> I think you should maybe aim for 2 patches here.
>
> Patch 1: Add new function to validate range and return bool indicating
> if the string is an integer within range. Set output argument to the
> int value if it is valid.  Modify all locations where we currently
> validate the range of the input arg to use the new function.
>
> Patch 2: Add additional validation where we don't currently do
> anything. e.g pg_dump -j
>
> We can then see if there's any merit in patch 2 of if it's adding more
> complexity than is really needed.
>
> I also think some compilers won't like:
>
> + compressLevel = parsed;
>
> given that "parsed" is int64 and "compressLevel" is int, surely some
> compilers will warn of possible truncation? An explicit cast to int
> should fix those or you could consider just writing a version of the
> function for int32 and int64 and directly passing in the variable to
> be set.
>
> --
>  David Rowley   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Mon, Oct 7, 2019 at 10:00 AM Masahiko Sawada 
wrote:

> On Sat, Oct 5, 2019 at 8:22 PM Amit Kapila 
> wrote:
> >
> > On Fri, Oct 4, 2019 at 7:57 PM Masahiko Sawada 
> wrote:
> >>
> >> On Fri, Oct 4, 2019 at 2:31 PM Amit Kapila 
> wrote:
> >> >>
> >> >
> >> > Do we really need to log all those messages?  The other places where
> we launch parallel workers doesn't seem to be using such messages.  Why do
> you think it is important to log the messages here when other cases don't
> use it?
> >>
> >> Well I would rather think that parallel create index doesn't log
> >> enough messages. Parallel maintenance operation is invoked manually by
> >> user. I can imagine that DBA wants to cancel and try the operation
> >> again later if enough workers are not launched. But there is not a
> >> convenient way to confirm how many parallel workers planned and
> >> actually launched. We need to see ps command or pg_stat_activity.
> >> That's why I think that log message would be helpful for users.
> >
> >
> > Hmm, what is a guarantee at a later time the user will get the required
> number of workers?  I think if the user decides to vacuum, then she would
> want it to start sooner.  Also, to cancel the vacuum, for this reason, the
> user needs to monitor logs which don't seem to be an easy thing considering
> this information will be logged at DEBUG2 level.  I think it is better to
> add in docs that we don't guarantee that the number of workers the user has
> asked or expected to use for a parallel vacuum will be available during
> execution.  Even if there is a compelling reason (which I don't see)  to
> log this information, I think we shouldn't use more than one message to log
> (like there is no need for a separate message for cleanup and vacuuming)
> this information.
> >
>
> I think that there is use case where user wants to cancel a
> long-running analytic query using parallel workers to use parallel
> workers for parallel vacuum instead. That way the lazy vacuum will
> eventually complete soon. Or user would want to see the vacuum log to
> check if lazy vacuum has been done with how many parallel workers for
> diagnostic when the vacuum took a long time. This log information
> appears when VERBOSE option is specified. When executing VACUUM
> command it's quite common to specify VERBOSE option to see the vacuum
> execution more details and VACUUM VERBOSE already emits very detailed
> information such as how many frozen pages are skipped and OldestXmin.
> So I think this information would not be too odd for that. Are you
> concerned that this information takes many lines of code? or it's not
> worth to be logged?
>

To an extent both, but I see the point you are making.  So, we should try
to minimize the number of lines used to log this message.  If we can use
just one message to log this information, that would be ideal.


>
> I agreed to add in docs that we don't guarantee that the number of
> workers user requested will be available.
>

Okay.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Change atoi to strtol in same place

[David Rowley]

On Mon, 7 Oct 2019 at 13:21, Joe Nelson  wrote:
>
> Ashutosh Sharma wrote:
> > One suggestion: The start value for port number is set to 1, however
> > it seems like the port number that falls in the range of 1-1023 is
> > reserved and can't be used. So, is it possible to have the start value
> > as 1024 instead of 1 ?
>
> Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
> so the range can be updated in one place for all utilities.

(I've only had a very quick look at this, and FWIW, here's my opinion)

It's not for this patch to decide what ports clients can connect to
PostgreSQL on. As far as I'm aware Windows has no restrictions on what
ports unprivileged users can listen on. I think we're likely going to
upset a handful of people if we block the client tools from connecting
to ports < 1024.

> > Further, I encountered one syntax error (INT_MAX undeclared) as the
> > header file "limits.h" has not been included in postgres_fe.h or
> > option.h
>
> Oops. Added limits.h now in option.h. The Postgres build accidentally
> worked on my system without explicitly including this header because
> __has_builtin(__builtin_isinf) is true for me so src/include/port.h
> pulled in math.h with an #if which pulled in limits.h.
>
> > Alvaro Herrera  wrote:
> > > The wording is a bit strange.  How about something like pg_standy:
> > > invalid argument to -k: %s
>
> I updated the error messages in pg_standby.
>
> > > >   *error = psprintf(_("could not parse '%s' as integer"), str);
> > >
> > > ... except that they would rather be more explicit about what the
> > > problem is.  "insufficient digits" or "extraneous character", etc.

This part seems over-complex to me. What's wrong with just returning a
bool and if the caller gets "false", then just have it emit the error,
such as:

"compression level must be between %d and %d"

I see Michael's patch is adding this new return type, but really, is
there a good reason why we need to do something special when the user
does not pass in an integer?

Current patch:
$ pg_dump -Z blah
invalid compression level: could not parse 'blah' as integer

I propose:
$ pg_dump -Z blah
compression level must be an integer in range 0..9

This might save a few round trips, e.g the current patch will do:
$ pg_dump -Z blah
invalid compression level: could not parse 'blah' as integer
$ pg_dump -Z 12345
invalid compression level: 12345 is outside range 0..9
$ ...

Also:

+ case PG_STRTOINT_RANGE_ERROR:
+ *error = psprintf(_("%s is outside range "
+ INT64_FORMAT ".." INT64_FORMAT),
+   str, min, max);

The translation string here must be consistent over all platforms. I
think this will cause issues if the translation string uses %ld and
the platform requires %lld?

I think what this patch should be really aiming for is to simplify the
client command-line argument parsing and adding what benefits it can.
I don't think there's really a need to make anything more complex than
it is already here.

I think you should maybe aim for 2 patches here.

Patch 1: Add new function to validate range and return bool indicating
if the string is an integer within range. Set output argument to the
int value if it is valid.  Modify all locations where we currently
validate the range of the input arg to use the new function.

Patch 2: Add additional validation where we don't currently do
anything. e.g pg_dump -j

We can then see if there's any merit in patch 2 of if it's adding more
complexity than is really needed.

I also think some compilers won't like:

+ compressLevel = parsed;

given that "parsed" is int64 and "compressLevel" is int, surely some
compilers will warn of possible truncation? An explicit cast to int
should fix those or you could consider just writing a version of the
function for int32 and int64 and directly passing in the variable to
be set.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: dropping column prevented due to inherited index

[Ashutosh Sharma]

I think we could have first deleted all the dependency of child object
on parent and then deleted the child itself using performDeletion().
As an example let's consider the case of toast table where we first
delete the dependency of toast relation on main relation and then
delete the toast table itself otherwise the toast table deletion would
fail. But, the problem I see here is that currently we do not have any
entry in pg_attribute table that would tell us about the dependency of
child column on parent.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Mon, Oct 7, 2019 at 7:31 AM Amit Langote  wrote:
>
> Hello,
>
> On Fri, Oct 4, 2019 at 5:57 PM Michael Paquier  wrote:
> >
> > On Thu, Oct 03, 2019 at 09:18:12AM -0300, Alvaro Herrera wrote:
> > > Hmm.  I wonder if we shouldn't adopt the coding pattern we've used
> > > elsewhere of collecting all columns to be dropped first into an
> > > ObjectAddresses array, then use performMultipleDeletions.
> >
> > +1.  That's the common pattern these days, because that's more
> > performant.
>
> Actually I don't see the peformMultipleDeletions() pattern being used
> for the situations where there are multiple objects to drop due to
> inheritance.  I only see it where there are multiple objects related
> to one table.  Maybe it's possible to apply to the inheritance
> situation though, but in this particular case, it seems a bit hard to
> do, because ATExecDropColumn steps through an inheritance tree level
> at a time.
>
> But maybe I misunderstood Alvaro's suggestion?
>
> >  I think that the patch should have regression tests.
>
> I have added one in the attached updated patch.
>
> Thanks,
> Amit

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Sat, Oct 5, 2019 at 8:22 PM Amit Kapila  wrote:
>
> On Fri, Oct 4, 2019 at 7:57 PM Masahiko Sawada  wrote:
>>
>> On Fri, Oct 4, 2019 at 2:31 PM Amit Kapila  wrote:
>> >>
>> >
>> > Do we really need to log all those messages?  The other places where we 
>> > launch parallel workers doesn't seem to be using such messages.  Why do 
>> > you think it is important to log the messages here when other cases don't 
>> > use it?
>>
>> Well I would rather think that parallel create index doesn't log
>> enough messages. Parallel maintenance operation is invoked manually by
>> user. I can imagine that DBA wants to cancel and try the operation
>> again later if enough workers are not launched. But there is not a
>> convenient way to confirm how many parallel workers planned and
>> actually launched. We need to see ps command or pg_stat_activity.
>> That's why I think that log message would be helpful for users.
>
>
> Hmm, what is a guarantee at a later time the user will get the required 
> number of workers?  I think if the user decides to vacuum, then she would 
> want it to start sooner.  Also, to cancel the vacuum, for this reason, the 
> user needs to monitor logs which don't seem to be an easy thing considering 
> this information will be logged at DEBUG2 level.  I think it is better to add 
> in docs that we don't guarantee that the number of workers the user has asked 
> or expected to use for a parallel vacuum will be available during execution.  
> Even if there is a compelling reason (which I don't see)  to log this 
> information, I think we shouldn't use more than one message to log (like 
> there is no need for a separate message for cleanup and vacuuming) this 
> information.
>

I think that there is use case where user wants to cancel a
long-running analytic query using parallel workers to use parallel
workers for parallel vacuum instead. That way the lazy vacuum will
eventually complete soon. Or user would want to see the vacuum log to
check if lazy vacuum has been done with how many parallel workers for
diagnostic when the vacuum took a long time. This log information
appears when VERBOSE option is specified. When executing VACUUM
command it's quite common to specify VERBOSE option to see the vacuum
execution more details and VACUUM VERBOSE already emits very detailed
information such as how many frozen pages are skipped and OldestXmin.
So I think this information would not be too odd for that. Are you
concerned that this information takes many lines of code? or it's not
worth to be logged?

I agreed to add in docs that we don't guarantee that the number of
workers user requested will be available.

-- 
Regards,

--
Masahiko Sawada

Re: Updated some links which are not working with new links

[vignesh C]

On Sat, Oct 5, 2019 at 7:13 AM vignesh C  wrote:
>
> Hi,
>
> There are some links referred in the source files which are currently
> not working.
>
> The below link:
> 
> is updated with:
> 
>
> The below links:
>
http://www-01.ibm.com/support/knowledgecenter/SSGH2K_11.1.0/com.ibm.xlc111.aix.doc/language_ref/function_attributes.html
>
http://www-01.ibm.com/support/knowledgecenter/SSGH2K_11.1.0/com.ibm.xlc111.aix.doc/language_ref/type_attrib.html
> are updated with:
>
http://www-01.ibm.com/support/knowledgecenter/SSGH2K_13.1.2/com.ibm.xlc131.aix.doc/language_ref/function_attributes.html
>
http://www-01.ibm.com/support/knowledgecenter/SSGH2K_13.1.2/com.ibm.xlc131.aix.doc/language_ref/type_attrib.html
>
> In c.h the link was not updated but in generic-xlc.h the link has been
> updated earlier.
>
Hi Michael,
The attached patch in previous mail contain the changes for the updated
links requested in [1]. It is not the complete set, but it is the first set
for which I could find the equivalent links.
https://www.postgresql.org/message-id/20191006074122.GC14532%40paquier.xyz

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Re: stress test for parallel workers

[Tom Lane]

Thomas Munro  writes:
> On Wed, Aug 7, 2019 at 4:29 PM Tom Lane  wrote:
>> Yeah, I've been wondering whether pg_ctl could fork off a subprocess
>> that would fork the postmaster, wait for the postmaster to exit, and then
>> report the exit status.  Where to report it *to* seems like the hard part,
>> but maybe an answer that worked for the buildfarm would be enough for now.

> Oh, right, you don't even need subreaper tricks (I was imagining we
> had a double fork somewhere we don't).

I got around to looking at how to do this.  Seeing that chipmunk hasn't
failed again, I'm inclined to write that off as perhaps unrelated.
That leaves us to diagnose the pg_upgrade failures on wobbegong and
vulpes.  The pg_upgrade test uses pg_ctl to start the postmaster,
and the only simple way to wedge this requirement into pg_ctl is as
attached.  Now, the attached is completely *not* suitable as a permanent
patch, because it degrades or breaks a number of pg_ctl behaviors that
rely on knowing the postmaster's actual PID rather than that of the
parent shell.  But it gets through check-world, so I think we can stick it
in transiently to see what it can teach us about the buildfarm failures.
Given wobbegong's recent failure rate, I don't think we'll have to wait
long.

Some notes about the patch:

* The core idea is to change start_postmaster's shell invocation
so that the shell doesn't just exec the postmaster, but runs a
mini shell script that runs the postmaster and then reports its
exit status.  I found that this still needed a dummy exec to force
the shell to perform the I/O redirections on itself, else pg_ctl's
TAP tests fail.  (I think what was happening was that if the shell
continued to hold open its original stdin, IPC::Run didn't believe
the command was done.)

* This means that what start_postmaster returns is not the postmaster's
own PID, but that of the parent shell.  So we have to lobotomize
wait_for_postmaster to handle the PID the same way as on Windows
(where that was already true); it can't test for exact equality
between the child process PID and what's in postmaster.pid.
(trap_sigint_during_startup is also broken, but we don't need that
to work to get through the regression tests.)

* That makes recovery/t/017_shm.pl fail, because there's a race
condition: after killing the old postmaster, the existing
postmaster.pid is enough to fool "pg_ctl start" into thinking the new
postmaster is already running.  I fixed that by making pg_ctl reject
any PID seen in a pre-existing postmaster.pid file.  That has a
nonzero probability of false match, so I would not want to stick it
in as a permanent thing on Unix ... but I wonder if it wouldn't be
an improvement over the current situation on Windows.

regards, tom lane

diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index dd76be6..316651c 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -106,6 +106,7 @@ static char promote_file[MAXPGPATH];
 static char logrotate_file[MAXPGPATH];
 
 static volatile pgpid_t postmasterPID = -1;
+static pgpid_t old_postmaster_pid = 0;
 
 #ifdef WIN32
 static DWORD pgctl_start_type = SERVICE_AUTO_START;
@@ -490,16 +491,17 @@ start_postmaster(void)
 
 	/*
 	 * Since there might be quotes to handle here, it is easier simply to pass
-	 * everything to a shell to process them.  Use exec so that the postmaster
-	 * has the same PID as the current child process.
+	 * everything to a shell to process them.
+	 *
+	 * Since we aren't telling the shell to directly exec the postmaster,
+	 * the returned PID is a parent process, the same as on Windows.
 	 */
 	if (log_file != NULL)
-		snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1",
- exec_path, pgdata_opt, post_opts,
- DEVNULL, log_file);
+		snprintf(cmd, MAXPGPATH, "exec < \"%s\" >> \"%s\" 2>&1; \"%s\" %s%s; echo postmaster exit status is $?",
+ DEVNULL, log_file, exec_path, pgdata_opt, post_opts);
 	else
-		snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" 2>&1",
- exec_path, pgdata_opt, post_opts, DEVNULL);
+		snprintf(cmd, MAXPGPATH, "exec < \"%s\" 2>&1; \"%s\" %s%s; echo postmaster exit status is $?",
+ DEVNULL, exec_path, pgdata_opt, post_opts);
 
 	(void) execl("/bin/sh", "/bin/sh", "-c", cmd, (char *) NULL);
 
@@ -586,12 +588,8 @@ wait_for_postmaster(pgpid_t pm_pid, bool do_checkpoint)
 			pmpid = atol(optlines[LOCK_FILE_LINE_PID - 1]);
 			pmstart = atol(optlines[LOCK_FILE_LINE_START_TIME - 1]);
 			if (pmstart >= start_time - 2 &&
-#ifndef WIN32
-pmpid == pm_pid
-#else
-			/* Windows can only reject standalone-backend PIDs */
-pmpid > 0
-#endif
+			/* If pid is the value we saw before starting, assume it's stale */
+pmpid > 0 && pmpid != old_postmaster_pid
 )
 			{
 /*
@@ -621,7 +619,7 @@ wait_for_postmaster(pgpid_t pm_pid, bool do_checkpoint)
 		 * Check whether the child postmaster process is still alive.  This
 		 * lets us exit early if the postmaster fails during 

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Sun, Oct 6, 2019 at 7:59 PM Amit Kapila  wrote:
>
> On Fri, Oct 4, 2019 at 7:34 PM Masahiko Sawada  wrote:
>>
>> On Fri, Oct 4, 2019 at 2:02 PM Amit Kapila  wrote:
>> >>
>> >> I'd also prefer to use maintenance_work_mem at max during parallel
>> >> vacuum regardless of the number of parallel workers. This is current
>> >> implementation. In lazy vacuum the maintenance_work_mem is used to
>> >> record itempointer of dead tuples. This is done by leader process and
>> >> worker processes just refers them for vacuuming dead index tuples.
>> >> Even if user sets a small amount of maintenance_work_mem the parallel
>> >> vacuum would be helpful as it still would take a time for index
>> >> vacuuming. So I thought we should cap the number of parallel workers
>> >> by the number of indexes rather than maintenance_work_mem.
>> >>
>> >
>> > Isn't that true only if we never use maintenance_work_mem during index 
>> > cleanup?  However, I think we are using during index cleanup, see forex. 
>> > ginInsertCleanup.  I think before reaching any conclusion about what to do 
>> > about this, first we need to establish whether this is a problem.  If I am 
>> > correct, then only some of the index cleanups (like gin index) use 
>> > maintenance_work_mem, so we need to consider that point while designing a 
>> > solution for this.
>> >
>>
>> I got your point. Currently the single process lazy vacuum could
>> consume the amount of (maintenance_work_mem * 2) memory at max because
>> we do index cleanup during holding the dead tuple space as you
>> mentioned. And ginInsertCleanup is also be called at the beginning of
>> ginbulkdelete. In current parallel lazy vacuum, each parallel vacuum
>> worker could consume other memory apart from the memory used by heap
>> scan depending on the implementation of target index AM. Given that
>> the current single and parallel vacuum implementation it would be
>> better to control the amount memory in total rather than the number of
>> parallel workers. So one approach I came up with is that we make all
>> vacuum workers use the amount of (maintenance_work_mem / # of
>> participants) as new maintenance_work_mem.
>
>
> Yeah, we can do something like that, but I am not clear whether the current 
> memory usage for Gin indexes is correct.  I have started a new thread, let's 
> discuss there.
>

Thank you for starting that discussion!

Regards,

--
Masahiko Sawada

Re: identity column behavior in WHEN condition for BEFORE EACH ROW trigger

[Suraj Kharage]

> whereas, for identity columns, server allows us to create trigger for same
> and trigger gets invoked as defined. Is this behavior expected? or we need
> to restrict the identity columns in such scenario because anyone one
> override the identity column value in trigger.
>

Also, I think it is breaking the OVERRIDING SYSTEM VALUE clause in INSERT
statement. i.e: without this clause, can insert the modified value from
trigger in identity column. I don't find any document reference for this
behavior.

Thoughts?

-- 
--

Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.

Re: dropping column prevented due to inherited index

[Amit Langote]

Hello,

On Fri, Oct 4, 2019 at 5:57 PM Michael Paquier  wrote:
>
> On Thu, Oct 03, 2019 at 09:18:12AM -0300, Alvaro Herrera wrote:
> > Hmm.  I wonder if we shouldn't adopt the coding pattern we've used
> > elsewhere of collecting all columns to be dropped first into an
> > ObjectAddresses array, then use performMultipleDeletions.
>
> +1.  That's the common pattern these days, because that's more
> performant.

Actually I don't see the peformMultipleDeletions() pattern being used
for the situations where there are multiple objects to drop due to
inheritance.  I only see it where there are multiple objects related
to one table.  Maybe it's possible to apply to the inheritance
situation though, but in this particular case, it seems a bit hard to
do, because ATExecDropColumn steps through an inheritance tree level
at a time.

But maybe I misunderstood Alvaro's suggestion?

>  I think that the patch should have regression tests.

I have added one in the attached updated patch.

Thanks,
Amit


ATExecDropColumn-inh-recursion-fix_v2.patch
Description: Binary data

Re: How to retain lesser paths at add_path()?

[Robert Haas]

On Sun, Oct 6, 2019 at 3:23 PM Tomas Vondra
 wrote:
> >I don't think this hook is a very useful approach to this problem, and
> >I'm concerned that it might have a measurable performance cost.
>
> Can you be more specific why you don't think this approach is not
> useful? I'm not sure whether you consider all hooks to have this issue
> or just this proposed one.

I'll start by admitting that that remark was rather off-the-cuff.  On
further reflection, add_path() is not really a crazy place to try to
add a new dimension of merit, which is really what KaiGai wants to do
here.  On the other hand, as Tom and I noted upthread, that system is
creaking under its weight as it is, and making it extensible seems
like it might therefore be a bad idea - specifically, because it might
slow down planning quite a bit on large join problems, either because
of the additional cost testing the additional dimension of merit or
because of the additional cost of dealing with the extra paths that
get kept.

It is maybe worth noting that join/aggregate pushdown for FDWs has a
somewhat similar problem, and we didn't solve it this way. Should we
have? Maybe it would have worked better and been less buggy. But
slowing down all planning for the benefit of that feature also sounds
bad. I think any changes in this area need careful though.

> As for the performance impact, I think that's not difficult to measure.
> I'd be surprised if it has measurable impact on cases with no hook
> installed (there's plenty more expensive stuff going on). Of course, it
> may have some impact for cases when the hook retains many more paths
> and/or does something expensive, but that's kinda up to whoever writes
> that particular hook. I think the assumption is that the savings from
> building better plans far outweight that extra cost.

You might be right, but add_path() is a pretty darn hot path in the
planner.  You probably wouldn't see a significant overhead on a
single-table query, but on a query with many tables I would not be
surprised if even the overhead of an empty function call was
measurable. But I could be wrong, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

JIT: Optimize generated functions earlier to lower memory usage

[Soumyadeep Chakraborty]

Hello,

This is to introduce a patch to lower the memory footprint of JITed
code by optimizing functions at the function level (i.e. with
function-level optimization passes) as soon as they are generated.
This addresses the code comment inside llvm_optimize_module():

/*
 * Do function level optimization. This could be moved to the point where
 * functions are emitted, to reduce memory usage a bit.
 */
 LLVMInitializeFunctionPassManager(llvm_fpm);

 --
 Soumyadeep (Deep)


v1-0001-Optimize-generated-functions-earlier-to-lower-mem.patch
Description: Binary data

Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

[Michael Paquier]

On Fri, Oct 04, 2019 at 05:21:25PM +0300, Alexey Kondratov wrote:
> I've checked your patch, but it seems that it cannot be applied as is, since
> it e.g. adds a comment to 005_same_timeline.pl without actually changing the
> test. So I've slightly modified your patch and tried to fit both dry-run and
> ensureCleanShutdown testing together. It works just fine and fails
> immediately if any of recent fixes is reverted. I still think that dry-run
> testing is worth adding, since it helped to catch this v12 refactoring
> issue, but feel free to throw it way if it isn't commitable right now, of
> course.

I can guarantee the last patch I sent can be applied on top of HEAD:
https://www.postgresql.org/message-id/20191004083721.ga1...@paquier.xyz

It would be nice to add the --dry-run part, though I think that we
could just make that part of one of the existing tests, and stop the
target server first (got to think about that part, please see below).

> As for incompatible options and sanity checks testing, yes, I agree that it
> is a matter of different patch. I attached it as a separate WIP patch just
> for history. Maybe I will try to gather more cases there later.

Thanks.  I have applied the first patch for the various improvements
around --no-ensure-shutdown.

Regarding the rest, I have hacked my way through as per the attached.
The previous set of patches did the following, which looked either
overkill or not necessary:
- Why running test 005 with the remote mode?
- --dry-run coverage is basically the same with the local and remote
modes, so it seems like a waste of resource to run it for all the
tests and all the modes.  I tend to think that this would live better
as part of another existing test, only running for say the local mode.
It is also possible to group all your tests from patch 2 and
006_actions.pl in this area.
- There is no need for the script checking for options combinations to
initialize a data folder.  It is important to design the tests to be
cheap and meaningful.

Patch v3-0002 also had a test to make sure that the source server is
shut down cleanly before using it.  I have included that part as
well, as the flow feels right.

So, Alexey, what do you think?
--
Michael
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl
index c3293e93df..645dc24578 100644
--- a/src/bin/pg_rewind/t/001_basic.pl
+++ b/src/bin/pg_rewind/t/001_basic.pl
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 11;
+use Test::More tests => 15;
 
 use FindBin;
 use lib $FindBin::RealBin;
@@ -66,6 +66,76 @@ sub run_test
 	master_psql("DELETE FROM tail_tbl WHERE id > 10");
 	master_psql("VACUUM tail_tbl");
 
+	# Before running pg_rewind, do a couple of extra tests with several
+	# option combinations.  As the code paths taken by those tests
+	# does not change for the "local" and "remote" modes, just run them
+	# in "local" mode for simplicity's sake.
+	if ($test_mode eq 'local')
+	{
+		my $master_pgdata  = $node_master->data_dir;
+		my $standby_pgdata = $node_standby->data_dir;
+
+		# First check that pg_rewind fails if the target cluster is
+		# not stopped as it fails to start up for the forced recovery
+		# step.
+		command_fails(
+			[
+'pg_rewind',
+"--debug",
+"--source-pgdata=$standby_pgdata",
+"--target-pgdata=$master_pgdata",
+"--no-sync"
+			],
+			'pg_rewind with running target');
+
+		# Again with --no-ensure-shutdown, which should equally fail.
+		# This time pg_rewind complains without attempting to perform
+		# recovery once.
+		command_fails(
+			[
+'pg_rewind',
+"--debug",
+"--source-pgdata=$standby_pgdata",
+"--target-pgdata=$master_pgdata",
+"--no-sync",
+'--no-ensure-shutdown'
+			],
+			'pg_rewind --no-ensure-shutdown with running target');
+
+		# Stop the target, and attempt to run with a local source
+		# still running.  This fail as pg_rewind requires to have
+		# a source cleanly stopped.
+		$node_master->stop;
+		command_fails(
+			[
+'pg_rewind',
+"--debug",
+"--source-pgdata=$standby_pgdata",
+"--target-pgdata=$master_pgdata",
+"--no-sync",
+'--no-ensure-shutdown'
+			],
+			'pg_rewind with running source');
+
+		# Stop the target cluster cleanly, and run again pg_rewind
+		# with --dry-run mode.  If anything gets generated in the data
+		# folder, the follow-up run of pg_rewind will most likely fail,
+		# so keep this test as the last one of this subset.
+		$node_standby->stop;
+		command_ok(
+			[
+'pg_rewind', "--debug",
+"--source-pgdata=$standby_pgdata",
+"--target-pgdata=$master_pgdata",
+"--no-sync", '--dry-run'
+			],
+			'pg_rewind --dry-run with running target');
+
+		# Both clusters need to be alive moving forward.
+		$node_standby->start;
+		$node_master->start;
+	}
+
 	RewindTest::run_pg_rewind($test_mode);
 
 	check_query(
diff --git a/src/bin/pg_rewind/t/006_options.pl b/src/bin/pg_rewind/t/006_options.pl
new file mode 100644
index 

adding partitioned tables to publications

[Amit Langote]

One cannot currently add partitioned tables to a publication.

create table p (a int, b int) partition by hash (a);
create table p1 partition of p for values with (modulus 3, remainder 0);
create table p2 partition of p for values with (modulus 3, remainder 1);
create table p3 partition of p for values with (modulus 3, remainder 2);

create publication publish_p for table p;
ERROR:  "p" is a partitioned table
DETAIL:  Adding partitioned tables to publications is not supported.
HINT:  You can add the table partitions individually.

One can do this instead:

create publication publish_p1 for table p1;
create publication publish_p2 for table p2;
create publication publish_p3 for table p3;

but maybe that's too much code to maintain for users.

I propose that we make this command:

create publication publish_p for table p;

automatically add all the partitions to the publication.  Also, any
future partitions should also be automatically added to the
publication.  So, publishing a partitioned table automatically
publishes all of its existing and future partitions.  Attached patch
implements that.

What doesn't change with this patch is that the partitions on the
subscription side still have to match one-to-one with the partitions
on the publication side, because the changes are still replicated as
being made to the individual partitions, not as the changes to the
root partitioned table.  It might be useful to implement that
functionality on the publication side, because it allows users to
define the replication target any way they need to, but this patch
doesn't implement that.

Thanks,
Amit


0001-Support-adding-partitioned-tables-to-publication.patch
Description: Binary data

Re: Change atoi to strtol in same place

[Joe Nelson]

Ashutosh Sharma wrote:
> One suggestion: The start value for port number is set to 1, however
> it seems like the port number that falls in the range of 1-1023 is
> reserved and can't be used. So, is it possible to have the start value
> as 1024 instead of 1 ?

Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
so the range can be updated in one place for all utilities.

> Further, I encountered one syntax error (INT_MAX undeclared) as the
> header file "limits.h" has not been included in postgres_fe.h or
> option.h

Oops. Added limits.h now in option.h. The Postgres build accidentally
worked on my system without explicitly including this header because
__has_builtin(__builtin_isinf) is true for me so src/include/port.h
pulled in math.h with an #if which pulled in limits.h. 

> Alvaro Herrera  wrote:
> > The wording is a bit strange.  How about something like pg_standy:
> > invalid argument to -k: %s

I updated the error messages in pg_standby.

> > >   *error = psprintf(_("could not parse '%s' as integer"), str);
> >
> > ... except that they would rather be more explicit about what the
> > problem is.  "insufficient digits" or "extraneous character", etc.

Sadly pg_strtoint64 returns the same error code for both cases. So we
could either petition for more detailed errors in the thread for that
other patch, or examine the string ourselves to check. Maybe it's not
needed since "could not parse 'abc' as integer" kind of does show the
problem.

> > I hope that no callers would like to have the messages not translated,
> > because that seems like it would become a mess.

That's true... I think it should be OK though, since we return the
pg_strtoint_status so callers can inspect that rather than relying on certain
words being in the error string. I'm guessing the translated string would be
most appropriate for end users.

-- 
Joe Nelson  https://begriffs.com
diff --git a/contrib/pg_standby/Makefile b/contrib/pg_standby/Makefile
index 0bca2f8e9e..cb9292d0f4 100644
--- a/contrib/pg_standby/Makefile
+++ b/contrib/pg_standby/Makefile
@@ -6,6 +6,8 @@ PGAPPICON = win32
 PROGRAM = pg_standby
 OBJS	= pg_standby.o $(WIN32RES)
 
+PG_LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 031b1b5cd5..9ce736249b 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -33,6 +33,7 @@
 #include "pg_getopt.h"
 
 #include "access/xlog_internal.h"
+#include "fe_utils/option.h"
 
 const char *progname;
 
@@ -678,6 +679,10 @@ main(int argc, char **argv)
 
 	while ((c = getopt(argc, argv, "cdk:lr:s:t:w:")) != -1)
 	{
+		pg_strtoint_status s;
+		int64		parsed;
+		char	   *parse_error;
+
 		switch (c)
 		{
 			case 'c':			/* Use copy */
@@ -687,12 +692,15 @@ main(int argc, char **argv)
 debug = true;
 break;
 			case 'k':			/* keepfiles */
-keepfiles = atoi(optarg);
-if (keepfiles < 0)
+s = pg_strtoint64_range(optarg, ,
+		0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
 {
-	fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname);
+	fprintf(stderr, "%s: invalid argument for -k keepfiles: %s\n",
+			progname, parse_error);
 	exit(2);
 }
+keepfiles = parsed;
 break;
 			case 'l':			/* Use link */
 
@@ -706,31 +714,39 @@ main(int argc, char **argv)
 #endif
 break;
 			case 'r':			/* Retries */
-maxretries = atoi(optarg);
-if (maxretries < 0)
+s = pg_strtoint64_range(optarg, ,
+		0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
 {
-	fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname);
+	fprintf(stderr, "%s: invalid argument for -r maxretries: %s\n",
+			progname, parse_error);
 	exit(2);
 }
+maxretries = parsed;
 break;
 			case 's':			/* Sleep time */
-sleeptime = atoi(optarg);
-if (sleeptime <= 0 || sleeptime > 60)
+s = pg_strtoint64_range(optarg, , 1, 60, _error);
+if (s != PG_STRTOINT_OK)
 {
-	fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname);
+	fprintf(stderr, "%s: invalid argument for -s sleeptime: %s\n",
+			progname, parse_error);
 	exit(2);
 }
+sleeptime = parsed;
 break;
 			case 't':			/* Trigger file */
 triggerPath = pg_strdup(optarg);
 break;
 			case 'w':			/* Max wait time */
-maxwaittime = atoi(optarg);
-if (maxwaittime < 0)
+s = pg_strtoint64_range(optarg, ,
+		0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
 {
-	fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname);
+	fprintf(stderr, "%s: invalid argument for -w maxwaittime: %s\n",
+			progname, parse_error);
 	exit(2);
 }
+maxwaittime = parsed;
 break;
 			default:
 fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
diff --git 

Re: v12 and pg_restore -f-

[Andrew Gierth]

BTW, the prior discussion is here:

https://www.postgresql.org/message-id/24868.1550106683%40sss.pgh.pa.us

-- 
Andrew (irc:RhodiumToad)

Re: v12 and pg_restore -f-

[Tom Lane]

Andrew Gierth  writes:
> "Tom" == Tom Lane  writes:
>  Tom> Perhaps we could change the back branches so that they interpret
>  Tom> "-f -" as "write to stdout", but without enforcing that you use
>  Tom> that syntax.

> We should definitely do that.

>  Tom> Alternatively, we could revert the v12 behavior change. On the
>  Tom> whole that might be the wiser course. I do not think the costs and
>  Tom> benefits of this change were all that carefully thought through.

> Failing to specify -d is a _really fricking common_ mistake for
> inexperienced users, who may not realize that the fact that they're
> seeing a ton of SQL on their terminal is not the normal result.
> Seriously, this comes up on a regular basis on IRC (which is why I
> suggested initially that we should do something about it).

No doubt, but that seems like a really poor excuse for breaking
maintenance scripts in a way that basically can't be fixed.  Even
with the change suggested above, scripts couldn't rely on "-f -"
working anytime soon, because you couldn't be sure whether a
back-rev pg_restore had the update or not.

The idea I'm leaning to after more thought is that we should change
*all* the branches to accept "-f -", but not throw an error if you
don't use it.  Several years from now, we could put the error back in;
but not until there's a plausible argument that nobody is running
old versions of pg_restore anymore.

regards, tom lane

Re: v12 and pg_restore -f-

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> Perhaps we could change the back branches so that they interpret
 Tom> "-f -" as "write to stdout", but without enforcing that you use
 Tom> that syntax.

We should definitely do that.

 Tom> Alternatively, we could revert the v12 behavior change. On the
 Tom> whole that might be the wiser course. I do not think the costs and
 Tom> benefits of this change were all that carefully thought through.

Failing to specify -d is a _really fricking common_ mistake for
inexperienced users, who may not realize that the fact that they're
seeing a ton of SQL on their terminal is not the normal result.
Seriously, this comes up on a regular basis on IRC (which is why I
suggested initially that we should do something about it).

-- 
Andrew (irc:RhodiumToad)

Re: v12 and pg_restore -f-

[Tom Lane]

[ redirecting to -hackers ]

Justin Pryzby  writes:
> I saw this and updated our scripts with pg_restore -f-

> https://www.postgresql.org/docs/12/release-12.html
> |In pg_restore, require specification of -f - to send the dump contents to 
> standard output (Euler Taveira)
> |Previously, this happened by default if no destination was specified, but 
> that was deemed to be unfriendly.

> What I didn't realize at first is that -f- has no special meaning in v11 - it
> just writes a file called ./-

Ugh.  I didn't realize that either, or I would have made a stink about
this patch.  Reducing the risk of getting a dump spewed at you is
completely not worth the cost of making it impossible to have
cross-version-compatible scripting of pg_restore.

Perhaps we could change the back branches so that they interpret "-f -"
as "write to stdout", but without enforcing that you use that syntax.
Nobody is going to wish that to mean "write to a file named '-'", so
I don't think this would be an unacceptable change.

Alternatively, we could revert the v12 behavior change.  On the whole
that might be the wiser course.  I do not think the costs and benefits
of this change were all that carefully thought through.

regards, tom lane

Re: How to retain lesser paths at add_path()?

[Tomas Vondra]

On Fri, Oct 04, 2019 at 12:19:06PM -0400, Robert Haas wrote:

On Mon, Aug 12, 2019 at 12:28 AM Kohei KaiGai  wrote:

For implementation of the concept, this patch puts a hook on add_path
/ add_partial_path
to override the path removal decision by extensions, according to its
own viewpoint.


I don't think this hook is a very useful approach to this problem, and
I'm concerned that it might have a measurable performance cost.



Can you be more specific why you don't think this approach is not
useful? I'm not sure whether you consider all hooks to have this issue
or just this proposed one.

As for the performance impact, I think that's not difficult to measure.
I'd be surprised if it has measurable impact on cases with no hook
installed (there's plenty more expensive stuff going on). Of course, it
may have some impact for cases when the hook retains many more paths
and/or does something expensive, but that's kinda up to whoever writes
that particular hook. I think the assumption is that the savings from
building better plans far outweight that extra cost.

That does not necessarily mean the proposed hook is correct - I only
briefly looked at it, and it's not clear to me why would it be OK to
call the hook for remove_old=true but not also for accept_new=false? How
do we know whether the "better" path arrives first?


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 

Missed check for too-many-children in bgworker spawning

[Tom Lane]

Over in [1] we have a report of a postmaster shutdown that seems to
have occurred because some client logic was overaggressively spawning
connection requests, causing the postmaster's child-process arrays to
be temporarily full, and then some parallel query tried to launch a
new bgworker process.  The postmaster's bgworker-spawning logic lacks
any check for the arrays being full, so when AssignPostmasterChildSlot
failed to find a free slot, kaboom!

The attached proposed patch fixes this by making bgworker spawning
include a canAcceptConnections() test.  That's perhaps overkill, since
we really just need to check the CountChildren() total; but I judged
that going through that function and having it decide what to test or
not test was a better design than duplicating the CountChildren() test
elsewhere.

I'd first imagined also replacing the one-size-fits-all check

if (CountChildren(BACKEND_TYPE_ALL) >= MaxLivePostmasterChildren())
result = CAC_TOOMANY;

with something like

switch (backend_type)
{
case BACKEND_TYPE_NORMAL:
if (CountChildren(backend_type) >= 2 * MaxConnections)
result = CAC_TOOMANY;
break;
case BACKEND_TYPE_AUTOVAC:
if (CountChildren(backend_type) >= 2 * autovacuum_max_workers)
result = CAC_TOOMANY;
break;
...
}

so as to subdivide the pool of child-process slots and prevent client
requests from consuming slots meant for background processes.  But on
closer examination that's not really worth the trouble, because this
pool is already considerably bigger than MaxBackends; so even if we
prevented a failure here we could still have bgworker startup failure
later on when it tries to acquire a PGPROC.

Barring objections, I'll apply and back-patch this soon.

regards, tom lane

[1] 
https://www.postgresql.org/message-id/flat/CADCf-WNZk_9680Q0YjfBzuiR0Oe8LzvDs2Ts3_tq6Tv1e8raQQ%40mail.gmail.com

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index eb9e022..7947c96 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -409,7 +409,7 @@ static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
 static void processCancelRequest(Port *port, void *pkt);
 static int	initMasks(fd_set *rmask);
 static void report_fork_failure_to_client(Port *port, int errnum);
-static CAC_state canAcceptConnections(void);
+static CAC_state canAcceptConnections(int backend_type);
 static bool RandomCancelKey(int32 *cancel_key);
 static void signal_child(pid_t pid, int signal);
 static bool SignalSomeChildren(int signal, int targets);
@@ -2401,13 +2401,15 @@ processCancelRequest(Port *port, void *pkt)
  * canAcceptConnections --- check to see if database state allows connections.
  */
 static CAC_state
-canAcceptConnections(void)
+canAcceptConnections(int backend_type)
 {
 	CAC_state	result = CAC_OK;
 
 	/*
 	 * Can't start backends when in startup/shutdown/inconsistent recovery
-	 * state.
+	 * state.  We treat autovac workers the same as user backends for this
+	 * purpose.  However, bgworkers are excluded from this test; we expect
+	 * bgworker_should_start_now() decided whether the DB state allows them.
 	 *
 	 * In state PM_WAIT_BACKUP only superusers can connect (this must be
 	 * allowed so that a superuser can end online backup mode); we return
@@ -2415,7 +2417,8 @@ canAcceptConnections(void)
 	 * that neither CAC_OK nor CAC_WAITBACKUP can safely be returned until we
 	 * have checked for too many children.
 	 */
-	if (pmState != PM_RUN)
+	if (pmState != PM_RUN &&
+		backend_type != BACKEND_TYPE_BGWORKER)
 	{
 		if (pmState == PM_WAIT_BACKUP)
 			result = CAC_WAITBACKUP;	/* allow superusers only */
@@ -2435,9 +2438,9 @@ canAcceptConnections(void)
 	/*
 	 * Don't start too many children.
 	 *
-	 * We allow more connections than we can have backends here because some
+	 * We allow more connections here than we can have backends because some
 	 * might still be authenticating; they might fail auth, or some existing
-	 * backend might exit before the auth cycle is completed. The exact
+	 * backend might exit before the auth cycle is completed.  The exact
 	 * MaxBackends limit is enforced when a new backend tries to join the
 	 * shared-inval backend array.
 	 *
@@ -4114,7 +4117,7 @@ BackendStartup(Port *port)
 	bn->cancel_key = MyCancelKey;
 
 	/* Pass down canAcceptConnections state */
-	port->canAcceptConnections = canAcceptConnections();
+	port->canAcceptConnections = canAcceptConnections(BACKEND_TYPE_NORMAL);
 	bn->dead_end = (port->canAcceptConnections != CAC_OK &&
 	port->canAcceptConnections != CAC_WAITBACKUP);
 
@@ -5486,7 +5489,7 @@ StartAutovacuumWorker(void)
 	 * we have to check to avoid race-condition problems during DB state
 	 * changes.
 	 */
-	if (canAcceptConnections() == CAC_OK)
+	if (canAcceptConnections(BACKEND_TYPE_AUTOVAC) == CAC_OK)
 	{
 		/*
 

Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT

[Matheus de Oliveira]

Sorry about the long delay in answering that, I hope to get to a consensus
on how to do that feature, which I think it is really valuable. Sending few
options and observations bellow...

On Sun, Jul 28, 2019 at 2:37 PM Tom Lane  wrote:

> Matheus de Oliveira  writes:
> > [ postgresql-alter-constraint.v5.patch ]
>
> Somebody seems to have marked this Ready For Committer without posting
> any review, which is not very kosher,


Sorry. I know Lucas, will talk to him for a better review ;D


> but I took a quick look at it
> anyway.
>
>
Thank you so much by that.

* It's failing to apply, as noted by the cfbot, because somebody added
> an unrelated test to the same spot in foreign_key.sql.  I fixed that
> in the attached rebase.
>
>
That was a mistake on rebase, sorry.


> * It also doesn't pass "git diff --check" whitespace checks, so
> I ran it through pgindent.
>
>
Still learning here, will take more care.


> * Grepping around for other references to struct Constraint,
> I noticed that you'd missed updating equalfuncs.c.  I did not
> fix that.
>
>
Certainly true, I fixed that just to keep it OK for now.


> The main issue I've got though is a definitional one --- I'm not at all
> sold on your premise that constraint deferrability syntax should mean
> different things depending on the previous state of the constraint.
>

I see the point, but I really believe we should have a simpler way to
change just specific properties
of the constraint without touching the others, and I do believe it is
valuable. So I'd like to check with
you all what would be a good option to have that.

Just as a demonstration, and a PoC, I have changed the patch to accept two
different syntaxes:
1. The one we have today with ALTER CONSTRAINT, and it change every
constraint property
2. A similar one with SET keyword in the middle, to force changing only the
given properties, e.g.:
ALTER TABLE table_name ALTER CONSTRAINT constr_name *SET* ON UPDATE
CASCADE;

I'm not at all happy with the syntax, doens't seem very clear. But I
proceeded this way nonetheless
just to verify the code on tablecmds.c would work. Please, does NOT
consider the patch as "ready",
it is more like a WIP and demonstration now (specially the test part, which
is no longer complete,
and gram.y that I changed the lazy way - both easy to fix if the syntax is
good).

I would really appreciate opinions on that, and I'm glad to work on a
better patch after we decide
the best syntax and approach.


> We don't generally act that way in other ALTER commands


That is true. I think one exception is ALTER COLUMN, which just acts on the
changes explicitly provided.
And I truly believe most people would expect changes on only provided
information on ALTER CONSTRAINT
as well. But I have no real research on that, more like a feeling :P


> and I don't see
> a strong argument to start doing so here.  If you didn't do this then
> you wouldn't (I think) need the extra struct Constraint fields in the
> first place, which is why I didn't run off and change equalfuncs.c.
>
>
Indeed true, changes on `Constraint` struct were only necessary due to
that, the patch would in fact
be way simpler without it (that is why I still insist on finding some way
to make it happen, perhaps
with a better syntax).


> In short, I'm inclined to argue that this variant of ALTER TABLE
> should replace *all* the fields of the constraint with the same
> properties it'd have if you'd created it fresh using the same syntax.
> This is by analogy to CREATE OR REPLACE commands, which don't
> preserve any of the old properties of the replaced object.


I agree for CREATE OR REPLACE, but in my POV REPLACE makes it clearer to
the user that
*everything* is changed, ALTER not so much. Again, this is just *my
opinion*, not a very strong
one though, but following first messages on that thread current behaviour
can be easily confused
with a bug (although it is not, the code clear shows it is expected,
specially on tests).


> Given
> the interactions between these fields, I think you're going to end up
> with a surprising mess of ad-hoc choices if you do differently.
> Indeed, you already have, but I think it'll get worse if anyone
> tries to extend the feature set further.
>
>
Certainly agree with that, the code is harder that way, as I said above.
Still thinking that
having the option is valuable though, we should be able to find a better
syntax/approach
for that.


> Perhaps the right way to attack it, given that, is to go ahead and
> invent "ALTER TABLE t ADD OR REPLACE CONSTRAINT c ...".  At least
> in the case at hand with FK constraints, we could apply suitable
> optimizations (ie skip revalidation) when the new definition shares
> the right properties with the old, and otherwise treat it like a
> drop-and-add.
>

I believe this path is quite easy for me to do now, if you all agree it is
a good approach.
What worries me is that we already have ALTER CONSTRAINT syntax, so what
would
we do with that? I see a 

Re: New "-b slim" option in 2019b zic: should we turn that on?

[Andrew Dunstan]

On 10/5/19 10:33 PM, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 10/5/19 6:33 PM, Tom Lane wrote:
>>> I had contemplated injecting the -b switch via
>>> -ZIC_OPTIONS =
>>> +ZIC_OPTIONS = -b slim
>> I don't think that's going to work very well with a buildfarm member,
>> where there's no convenient way to set it.
> Can't you set that from build_env?
>
>   


No, build_env sets the environment, not makefile variables, and
configure doesn't fill in ZIC_OPTIONS, unlike what it does with ZIC.


Anyway, it turns out that avoiding the issue I was having here just
postpones the problem for a few seconds, so while we should probably do
something here it's not urgent from my POV.


cheers


andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

[Nikolay Shaplov]

В письме от пятница, 27 сентября 2019 г. 17:24:49 MSK пользователь Michael 
Paquier написал:

> The patch is in this state for two months now, so I have switched it
> to "returned with feedback". 

So I've split this patch into even smaller parts, so it would be more easy to 
review.

Do not use StdRdOptions in Access Methods
https://www.postgresql.org/message-id/4127670.gFlpRb6XCm@x200m

Use empty PartitionedRelOption structure for storing partitioned table options 
instead of StdRdOption
https://www.postgresql.org/message-id/1627387.Qykg9O6zpu@x200m

and 

Some useful asserts in ViewOptions Macroses
https://www.postgresql.org/message-id/3634983.eHpMQ1mJnI@x200m

as for removing StdRdOptions for Heap and Toast, I would suggest for commit it 
later. It is not critical for my reloptions refactoring plans. I can do it 
having one structure for two relation kinds. So we can split it into two 
later, or do not split at all...

-- 
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing  (Russian)

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH] Do not use StdRdOptions in Access Methods

[Nikolay Shaplov]

Hi! I am starting a new thread as commitfest wants new thread for new patch.

This new thread is a follow up to an https://www.postgresql.org/message-id/
2620882.s52SJui4ql%40x200m thread, where I've been trying to get rid of 
StdRdOpions structure, and now I've splitted the patch into smaller parts.

Read the quote below, to get what this patch is about

> I've been thinking about this patch and came to a conclusion that it
> would be better to split it to even smaller parts, so they can be
> easily reviewed, one by one. May be even leaving most complex
> Heap/Toast part for later.
> 
> This is first part of this patch. Here we give each Access Method it's
> own option binary representation instead of StdRdOptions.
> 
> I think this change is obvious. Because, first, Access Methods do not
> use most of the values defined in StdRdOptions.
> 
> Second this patch make Options structure code unified for all core
> Access Methods. Before some AM used StdRdOptions, some AM used it's own
> structure, now all AM uses own structures and code is written in the
> same style, so it would be more easy to update it in future.
> 
> John Dent, would you join reviewing this part of the patch? I hope it
> will be more easy now...

And now I have a newer version of the patch, as I forgot to remove 
vacuum_cleanup_index_scale_factor from StdRdOptions as it was used only in 
Btree index and now do not used at all. New version fixes it.

-- 
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing  (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..02ee3c9 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -23,7 +23,7 @@
 #include "access/htup_details.h"
 #include "access/nbtree.h"
 #include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "commands/tablespace.h"
@@ -1513,8 +1513,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
 		offsetof(StdRdOptions, user_catalog_table)},
 		{"parallel_workers", RELOPT_TYPE_INT,
 		offsetof(StdRdOptions, parallel_workers)},
-		{"vacuum_cleanup_index_scale_factor", RELOPT_TYPE_REAL,
-		offsetof(StdRdOptions, vacuum_cleanup_index_scale_factor)},
 		{"vacuum_index_cleanup", RELOPT_TYPE_BOOL,
 		offsetof(StdRdOptions, vacuum_index_cleanup)},
 		{"vacuum_truncate", RELOPT_TYPE_BOOL,
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 838ee68..eec2100 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -359,7 +359,7 @@ _hash_init(Relation rel, double num_tuples, ForkNumber forkNum)
 	data_width = sizeof(uint32);
 	item_width = MAXALIGN(sizeof(IndexTupleData)) + MAXALIGN(data_width) +
 		sizeof(ItemIdData);		/* include the line pointer */
-	ffactor = RelationGetTargetPageUsage(rel, HASH_DEFAULT_FILLFACTOR) / item_width;
+	ffactor = (BLCKSZ * HashGetFillFactor(rel) / 100) / item_width;
 	/* keep to a sane range */
 	if (ffactor < 10)
 		ffactor = 10;
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index 3fb92ea..5170634 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -289,7 +289,28 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
 bytea *
 hashoptions(Datum reloptions, bool validate)
 {
-	return default_reloptions(reloptions, validate, RELOPT_KIND_HASH);
+	relopt_value *options;
+	HashRelOptions *rdopts;
+	int			numoptions;
+	static const relopt_parse_elt tab[] = {
+		{"fillfactor", RELOPT_TYPE_INT, offsetof(HashRelOptions, fillfactor)},
+	};
+
+	options = parseRelOptions(reloptions, validate, RELOPT_KIND_HASH,
+			  );
+
+	/* if none set, we're done */
+	if (numoptions == 0)
+		return NULL;
+
+	rdopts = allocateReloptStruct(sizeof(HashRelOptions), options, numoptions);
+
+	fillRelOptions((void *) rdopts, sizeof(HashRelOptions), options, numoptions,
+   validate, tab, lengthof(tab));
+
+	pfree(options);
+
+	return (bytea *) rdopts;
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd528..b460d13 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -816,7 +816,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
 	}
 	else
 	{
-		StdRdOptions *relopts;
+		BTRelOptions *relopts;
 		float8		cleanup_scale_factor;
 		float8		prev_num_heap_tuples;
 
@@ -827,7 +827,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
 		 * tuples exceeds vacuum_cleanup_index_scale_factor fraction of
 		 * original tuples count.
 		 */
-		relopts = (StdRdOptions *) info->index->rd_options;
+		relopts = (BTRelOptions *) info->index->rd_options;
 		cleanup_scale_factor = (relopts &&
 relopts->vacuum_cleanup_index_scale_factor >= 0)
 			

[PATCH] use separate PartitionedRelOptions structure to store partitioned table options

[Nikolay Shaplov]

Hi!

This message is follow up to the "Get rid of the StdRdOptions" patch thread:
https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m

I've split patch into even smaller parts and commitfest want each patch in 
separate thread. So it is new thread.

The idea of this patch is following: If you read the code, partitioned tables 
do not have any options (you will not find RELOPT_KIND_PARTITIONED in 
boolRelOpts, intRelOpts, realRelOpts, stringRelOpts and enumRelOpts in 
reloption.c), but it uses StdRdOptions to store them (these no options).

If partitioned table is to have it's own option set (even if it is empty now) 
it would be better to save it into separate structure, like it is done for 
Views, not adding them to StdRdOptions, like current code hints to do.

So in this patch I am creating a structure that would store partitioned table 
options (it is empty for now as there are no options for this relation kind), 
and all other code that would use this structure as soon as the first option 
comes.

I think it is bad idea to suggest option adder to ad it to StdRdOption, we 
already have a big mess there. Better if he add it to an new empty structure.

-- 
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing  (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..f219ad3 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1099,9 +1099,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
-		case RELKIND_PARTITIONED_TABLE:
 			options = heap_reloptions(classForm->relkind, datum, false);
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			options = partitioned_reloptions(datum, false);
+			break;
 		case RELKIND_VIEW:
 			options = view_reloptions(datum, false);
 			break;
@@ -1538,6 +1540,25 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
 }
 
 /*
+ * Option parser for partitioned relations
+ */
+bytea *
+partitioned_reloptions(Datum reloptions, bool validate)
+{
+   int   numoptions;
+
+  /*
+   * Since there is no options for patitioned table for now, we just do
+   * validation to report incorrect option error and leave.
+   */
+
+  if (validate)
+		parseRelOptions(reloptions, validate, RELOPT_KIND_PARTITIONED,
+		);
+   return NULL;
+}
+
+/*
  * Option parser for views
  */
 bytea *
@@ -1593,9 +1614,6 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
 		case RELKIND_RELATION:
 		case RELKIND_MATVIEW:
 			return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
-		case RELKIND_PARTITIONED_TABLE:
-			return default_reloptions(reloptions, validate,
-	  RELOPT_KIND_PARTITIONED);
 		default:
 			/* other relkinds are not supported */
 			return NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 05593f3..db27f30 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -720,10 +720,17 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
 	 true, false);
 
-	if (relkind == RELKIND_VIEW)
-		(void) view_reloptions(reloptions, true);
-	else
-		(void) heap_reloptions(relkind, reloptions, true);
+	switch ((int)relkind)
+	{
+		case RELKIND_VIEW:
+			(void) view_reloptions(reloptions, true);
+			break;
+		case RELKIND_PARTITIONED_TABLE:
+			(void) partitioned_reloptions(reloptions, true);
+			break;
+		default:
+			(void) heap_reloptions(relkind, reloptions, true);
+	}
 
 	if (stmt->ofTypename)
 	{
@@ -12158,9 +12165,11 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
-		case RELKIND_PARTITIONED_TABLE:
 			(void) heap_reloptions(rel->rd_rel->relkind, newOptions, true);
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			(void) partitioned_reloptions(newOptions, true);
+			break;
 		case RELKIND_VIEW:
 			(void) view_reloptions(newOptions, true);
 			break;
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 6bde209..0ad5c74 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -301,6 +301,7 @@ extern bytea *default_reloptions(Datum reloptions, bool validate,
  relopt_kind kind);
 extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate);
 extern bytea *view_reloptions(Datum reloptions, bool validate);
+extern bytea *partitioned_reloptions(Datum reloptions, bool validate);
 extern bytea *index_reloptions(amoptions_function amoptions, Datum reloptions,
 			   bool validate);
 extern bytea *attribute_reloptions(Datum reloptions, bool validate);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 

parallel restore sometimes fails for FKs to partitioned tables

[Alvaro Herrera]

Hello

While playing around I noticed that depending on the number of parallel
workers in pg_restore compared to the number of partitions a table has,
restoring an FK fails because the FK itself is restored before the index
partitions have completed restoring.  The exact conditions to cause the
failure seem to vary depending on whether the dump is schema-only or not.

This can seemingly be fixed by having pg_dump make the constraint depend
on the attach of each partition, as in the attached patch.  With this
patch I no longer see failures.


This patch is a bit weird because I added a new "simple list" type, to
store pointers.  One alternative would be to store the dumpId values for
the partitions instead, but we don't have a dumpId-typed simple list
either.  We could solve that by casting the dumpId to Oid, but that
seems almost as strange as the current proposal.

The other thing that makes this patch a little weird is that we have to
scan the list of indexes in the referenced partitioned table in order to
find the correct one.  This should be okay, as the number of indexes in
any one table is not expected to grow very large.  This isn't easy to
fix because we don't have a bsearchable array of indexes like we do of
other object types, and this already requires some contortions nearby.
Still, I'm not sure that this absolutely needs fixing now.


-- 
Álvaro Herrera Developer, https://www.PostgreSQL.org/
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 02a865f456..3549f7bc08 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -412,6 +412,9 @@ flagInhIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 			addObjectDependency([k].dobj,
 parentidx->indextable->dobj.dumpId);
 
+			/* keep track of the list of partitions in the parent index */
+			simple_ptr_list_append(>partattaches, [k].dobj);
+
 			k++;
 		}
 	}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a9c868b9af..06b07a5c96 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7109,6 +7109,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 			indxinfo[j].indisclustered = (PQgetvalue(res, j, i_indisclustered)[0] == 't');
 			indxinfo[j].indisreplident = (PQgetvalue(res, j, i_indisreplident)[0] == 't');
 			indxinfo[j].parentidx = atooid(PQgetvalue(res, j, i_parentidx));
+			indxinfo[j].partattaches = (SimplePtrList) { NULL, NULL };
 			contype = *(PQgetvalue(res, j, i_contype));
 
 			if (contype == 'p' || contype == 'u' || contype == 'x')
@@ -7241,6 +7242,7 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 i_conoid,
 i_conname,
 i_confrelid,
+i_conindid,
 i_condef;
 	int			ntups;
 
@@ -7266,7 +7268,7 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 		resetPQExpBuffer(query);
 		if (fout->remoteVersion >= 11)
 			appendPQExpBuffer(query,
-			  "SELECT tableoid, oid, conname, confrelid, "
+			  "SELECT tableoid, oid, conname, confrelid, conindid, "
 			  "pg_catalog.pg_get_constraintdef(oid) AS condef "
 			  "FROM pg_catalog.pg_constraint "
 			  "WHERE conrelid = '%u'::pg_catalog.oid "
@@ -7275,7 +7277,7 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 			  tbinfo->dobj.catId.oid);
 		else
 			appendPQExpBuffer(query,
-			  "SELECT tableoid, oid, conname, confrelid, "
+			  "SELECT tableoid, oid, conname, confrelid, 0 as conindid, "
 			  "pg_catalog.pg_get_constraintdef(oid) AS condef "
 			  "FROM pg_catalog.pg_constraint "
 			  "WHERE conrelid = '%u'::pg_catalog.oid "
@@ -7289,12 +7291,15 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 		i_conoid = PQfnumber(res, "oid");
 		i_conname = PQfnumber(res, "conname");
 		i_confrelid = PQfnumber(res, "confrelid");
+		i_conindid = PQfnumber(res, "conindid");
 		i_condef = PQfnumber(res, "condef");
 
 		constrinfo = (ConstraintInfo *) pg_malloc(ntups * sizeof(ConstraintInfo));
 
 		for (j = 0; j < ntups; j++)
 		{
+			TableInfo *reftable;
+
 			constrinfo[j].dobj.objType = DO_FK_CONSTRAINT;
 			constrinfo[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, i_contableoid));
 			constrinfo[j].dobj.catId.oid = atooid(PQgetvalue(res, j, i_conoid));
@@ -7311,6 +7316,34 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 			constrinfo[j].condeferred = false;
 			constrinfo[j].conislocal = true;
 			constrinfo[j].separate = true;
+
+			/*
+			 * Restoring an FK that points to a partitioned table requires
+			 * that all partition indexes have been attached beforehand.
+			 * Ensure that happens by making the constraint depend on each
+			 * index partition attach object.
+			 */
+			reftable = findTableByOid(constrinfo[j].confrelid);
+
+			if (reftable->relkind == RELKIND_PARTITIONED_TABLE)
+			{
+IndxInfo   *refidx;
+OId			indexOid = atooid(PQgetvalue(res, j, i_conindid));
+
+for (int k = 0; 

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Fri, Oct 4, 2019 at 7:34 PM Masahiko Sawada 
wrote:

> On Fri, Oct 4, 2019 at 2:02 PM Amit Kapila 
> wrote:
> >>
> >> I'd also prefer to use maintenance_work_mem at max during parallel
> >> vacuum regardless of the number of parallel workers. This is current
> >> implementation. In lazy vacuum the maintenance_work_mem is used to
> >> record itempointer of dead tuples. This is done by leader process and
> >> worker processes just refers them for vacuuming dead index tuples.
> >> Even if user sets a small amount of maintenance_work_mem the parallel
> >> vacuum would be helpful as it still would take a time for index
> >> vacuuming. So I thought we should cap the number of parallel workers
> >> by the number of indexes rather than maintenance_work_mem.
> >>
> >
> > Isn't that true only if we never use maintenance_work_mem during index
> cleanup?  However, I think we are using during index cleanup, see forex.
> ginInsertCleanup.  I think before reaching any conclusion about what to do
> about this, first we need to establish whether this is a problem.  If I am
> correct, then only some of the index cleanups (like gin index) use
> maintenance_work_mem, so we need to consider that point while designing a
> solution for this.
> >
>
> I got your point. Currently the single process lazy vacuum could
> consume the amount of (maintenance_work_mem * 2) memory at max because
> we do index cleanup during holding the dead tuple space as you
> mentioned. And ginInsertCleanup is also be called at the beginning of
> ginbulkdelete. In current parallel lazy vacuum, each parallel vacuum
> worker could consume other memory apart from the memory used by heap
> scan depending on the implementation of target index AM. Given that
> the current single and parallel vacuum implementation it would be
> better to control the amount memory in total rather than the number of
> parallel workers. So one approach I came up with is that we make all
> vacuum workers use the amount of (maintenance_work_mem / # of
> participants) as new maintenance_work_mem.


Yeah, we can do something like that, but I am not clear whether the current
memory usage for Gin indexes is correct.  I have started a new thread,
let's discuss there.

[1] -
https://www.postgresql.org/message-id/CAA4eK1LmcD5aPogzwim5Nn58Ki%2B74a6Edghx4Wd8hAskvHaq5A%40mail.gmail.com

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

maintenance_work_mem used by Vacuum

[Amit Kapila]

As per docs [1] (see maintenance_work_mem), the maximum amount of memory
used by the Vacuum command must be no more than maintenance_work_mem.
However, during the review/discussion of the "parallel vacuum" patch [2],
we observed that it is not true.  Basically, if there is a gin index
defined on a table, then the vacuum on that table can consume up to 2
* maintenance_work_mem memory space.  The vacuum can use
maintenance_work_mem memory space to keep track of dead tuples and
another maintenance_work_mem memory space to move tuples from pending pages
into regular GIN structure (see ginInsertCleanup).   The behavior related
to Gin index consuming extra maintenance_work_mem memory is introduced by
commit  e2c79e14d998cd31f860854bc9210b37b457bb01.  It is not clear to me if
this is acceptable behavior and if so, shouldn't we document it?

We wanted to decide how a parallel vacuum should use memory?  Can each
worker consume maintenance_work_mem to clean up the gin Index or all
workers should use no more than maintenance_work_mem?  We were thinking of
later but before we decide what is the right behavior for parallel vacuum,
I thought it is better to once discuss if the current memory usage model is
right.


[1] - https://www.postgresql.org/docs/devel/runtime-config-resource.html
[2] -
https://www.postgresql.org/message-id/CAD21AoARj%3De%3D6_KOZnaR66jRkDmGaVdLcrt33Ua-zMUugKU3mQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: dropdb --force

[Pavel Stehule]

ne 6. 10. 2019 v 10:19 odesílatel vignesh C  napsal:

> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule 
> wrote:
> >
> >
> >
> > čt 3. 10. 2019 v 19:48 odesílatel vignesh C 
> napsal:
> >>
> >> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule 
> wrote:
> >> >
> >> > Thank you for careful review. I hope so all issues are out.
> >> >
> >> >
> >> Thanks Pavel for fixing the comments.
> >> Few comments:
> >> The else part cannot be hit in DropDatabase function as gram.y expects
> FORCE.
> >> +
> >> + if (strcmp(opt->defname, "force") == 0)
> >> + force = true;
> >> + else
> >> + ereport(ERROR,
> >> + (errcode(ERRCODE_SYNTAX_ERROR),
> >> + errmsg("unrecognized DROP DATABASE option \"%s\"", opt->defname),
> >> + parser_errposition(pstate, opt->location)));
> >> + }
> >> +
> >
> >
> > I know - but somebody can call DropDatabase function outside parser. So
> is better check all possibilities.
> >
> >>
> >> We should change gram.y to accept any keyword and throw error from
> >> DropDatabase function.
> >> + */
> >> +drop_option: FORCE
> >> + {
> >> + $$ = makeDefElem("force", NULL, @1);
> >> + }
> >> + ;
> >
> >
> > I spent some time with thinking about it, and I think so this variant
> (with keyword) is well readable and very illustrative. This will be lost
> with generic variant.
> >
> > When the keyword FORCE already exists, then I prefer current state.
> >
> >>
> >>
> >> "This will also fail" should be "This will fail"
> >> + 
> >> +  This will fail, if current user has no permissions to terminate
> other
> >> +  connections. Required permissions are the same as with
> >> +  pg_terminate_backend, described
> >> +  in .
> >> +
> >> +  This will also fail if we are not able to terminate connections
> or
> >> +  when there are active prepared transactions or active logical
> replication
> >> +  slots.
> >> + 
> >
> >
> > fixed
> >
> >>
> >>
> >> Can we add few tests for this feature.
> >
> >
> > there are not any other test for DROP DATABASE
> >
> > We can check syntax later inside second patch (for -f option of dropdb
> command)
> >
> Changes in this patch looks fine to me.
> I'm not sure if we have forgotten to miss attaching the second patch
> or can you provide the link to second patch.
>

I plan to work on the second patch after committing of this first. Now we
are early on commit fest, and the complexity of this or second patch is not
too high be necessary to prepare patch series.

Regards

Pavel


> Regards,
> Vignesh
> EnterpriseDB: http://www.enterprisedb.com
>

Re: dropdb --force

[vignesh C]

On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule  wrote:
>
>
>
> čt 3. 10. 2019 v 19:48 odesílatel vignesh C  napsal:
>>
>> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule  
>> wrote:
>> >
>> > Thank you for careful review. I hope so all issues are out.
>> >
>> >
>> Thanks Pavel for fixing the comments.
>> Few comments:
>> The else part cannot be hit in DropDatabase function as gram.y expects FORCE.
>> +
>> + if (strcmp(opt->defname, "force") == 0)
>> + force = true;
>> + else
>> + ereport(ERROR,
>> + (errcode(ERRCODE_SYNTAX_ERROR),
>> + errmsg("unrecognized DROP DATABASE option \"%s\"", opt->defname),
>> + parser_errposition(pstate, opt->location)));
>> + }
>> +
>
>
> I know - but somebody can call DropDatabase function outside parser. So is 
> better check all possibilities.
>
>>
>> We should change gram.y to accept any keyword and throw error from
>> DropDatabase function.
>> + */
>> +drop_option: FORCE
>> + {
>> + $$ = makeDefElem("force", NULL, @1);
>> + }
>> + ;
>
>
> I spent some time with thinking about it, and I think so this variant (with 
> keyword) is well readable and very illustrative. This will be lost with 
> generic variant.
>
> When the keyword FORCE already exists, then I prefer current state.
>
>>
>>
>> "This will also fail" should be "This will fail"
>> + 
>> +  This will fail, if current user has no permissions to terminate other
>> +  connections. Required permissions are the same as with
>> +  pg_terminate_backend, described
>> +  in .
>> +
>> +  This will also fail if we are not able to terminate connections or
>> +  when there are active prepared transactions or active logical 
>> replication
>> +  slots.
>> + 
>
>
> fixed
>
>>
>>
>> Can we add few tests for this feature.
>
>
> there are not any other test for DROP DATABASE
>
> We can check syntax later inside second patch (for -f option of dropdb 
> command)
>
Changes in this patch looks fine to me.
I'm not sure if we have forgotten to miss attaching the second patch
or can you provide the link to second patch.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Re: Non-Active links being referred in our source code

[Michael Paquier]

On Sun, Oct 06, 2019 at 09:06:44AM +0530, vignesh C wrote:
> I could not find the equivalent links for the same.
> Should we update the links for the same?

If it could be possible to find equivalent links which could update
to, it would be nice.
--
Michael


signature.asc
Description: PGP signature