Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Fri, Oct 18, 2019 at 8:45 AM Dilip Kumar  wrote:
>
> On Thu, Oct 17, 2019 at 4:00 PM Amit Kapila  wrote:
> >
> > On Thu, Oct 17, 2019 at 3:25 PM Dilip Kumar  wrote:
> > >
> > > On Thu, Oct 17, 2019 at 2:12 PM Masahiko Sawada  
> > > wrote:
> > > >
> > > > On Thu, Oct 17, 2019 at 5:30 PM Amit Kapila  
> > > > wrote:
> > > > >
> > > > > Another point in this regard is that the user anyway has an option to
> > > > > turn off the cost-based vacuum.  By default, it is anyway disabled.
> > > > > So, if the user enables it we have to provide some sensible behavior.
> > > > > If we can't come up with anything, then, in the end, we might want to
> > > > > turn it off for a parallel vacuum and mention the same in docs, but I
> > > > > think we should try to come up with a solution for it.
> > > >
> > > > I finally got your point and now understood the need. And the idea I
> > > > proposed doesn't work fine.
> > > >
> > > > So you meant that all workers share the cost count and if a parallel
> > > > vacuum worker increase the cost and it reaches the limit, does the
> > > > only one worker sleep? Is that okay even though other parallel workers
> > > > are still running and then the sleep might not help?
> > > >
> >
> > Remember that the other running workers will also increase
> > VacuumCostBalance and whichever worker finds that it becomes greater
> > than VacuumCostLimit will reset its value and sleep.  So, won't this
> > make sure that overall throttling works the same?
> >
> > > I agree with this point.  There is a possibility that some of the
> > > workers who are doing heavy I/O continue to work and OTOH other
> > > workers who are doing very less I/O might become the victim and
> > > unnecessarily delay its operation.
> > >
> >
> > Sure, but will it impact the overall I/O?  I mean to say the rate
> > limit we want to provide for overall vacuum operation will still be
> > the same.  Also, isn't a similar thing happens now also where heap
> > might have done a major portion of I/O but soon after we start
> > vacuuming the index, we will hit the limit and will sleep.
>
> Actually, What I meant is that the worker who performing actual I/O
> might not go for the delay and another worker which has done only CPU
> operation might pay the penalty?  So basically the worker who is doing
> CPU intensive operation might go for the delay and pay the penalty and
> the worker who is performing actual I/O continues to work and do
> further I/O.  Do you think this is not a practical problem?
>

I don't know.  Generally, we try to delay (if required) before
processing (read/write) one page which means it will happen for I/O
intensive operations, so I am not sure if the point you are making is
completely correct.

> Stepping back a bit,  OTOH, I think that we can not guarantee that the
> one worker who has done more I/O will continue to do further I/O and
> the one which has not done much I/O will not perform more I/O in
> future.  So it might not be too bad if we compute shared costs as you
> suggested above.
>

I am thinking if we can write the patch for both the approaches (a.
compute shared costs and try to delay based on that, b. try to divide
the I/O cost among workers as described in the email above[1]) and do
some tests to see the behavior of throttling, that might help us in
deciding what is the best strategy to solve this problem, if any.
What do you think?


[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2BySETHCaCnAsEC-dC4GSXaE2sNGMOgD6J%3DX%2BN43bBqJQ%40mail.gmail.com

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

Re: Non working timeout detection in logical worker

[Julien Rouhaud]

On Fri, Oct 18, 2019 at 7:32 AM Michael Paquier  wrote:
>
> On Thu, Oct 17, 2019 at 08:00:15PM +0200, Julien Rouhaud wrote:
> > Jehan-Guillaume (in Cc) reported me today a problem with logical
> > replication, where in case of network issue the walsender is correctly
> > terminating at the given wal_sender_timeout but the logical worker
> > kept waiting indefinitely.
> >
> > The issue is apparently a simple thinko, the timestamp of the last
> > received activity being unconditionally set at the beginning of the
> > main processing loop, making any reasonable timeout setting
> > ineffective.  Trivial patch to fix the problem attached.
>
> Right, good catch.  That's indeed incorrect.  The current code would
> just keep resetting the timeout if walrcv_receive() returns 0 roughly
> once per NAPTIME_PER_CYCLE.  The ping sent to the server once reaching
> half of wal_receiver_timeout was also broken because of that.
>
> In short, applied and back-patched down to 10.

Thanks Michael!

Re: Non working timeout detection in logical worker

[Michael Paquier]

On Thu, Oct 17, 2019 at 08:00:15PM +0200, Julien Rouhaud wrote:
> Jehan-Guillaume (in Cc) reported me today a problem with logical
> replication, where in case of network issue the walsender is correctly
> terminating at the given wal_sender_timeout but the logical worker
> kept waiting indefinitely.
> 
> The issue is apparently a simple thinko, the timestamp of the last
> received activity being unconditionally set at the beginning of the
> main processing loop, making any reasonable timeout setting
> ineffective.  Trivial patch to fix the problem attached.

Right, good catch.  That's indeed incorrect.  The current code would
just keep resetting the timeout if walrcv_receive() returns 0 roughly
once per NAPTIME_PER_CYCLE.  The ping sent to the server once reaching
half of wal_receiver_timeout was also broken because of that.

In short, applied and back-patched down to 10.
--
Michael


signature.asc
Description: PGP signature

Re: Questions/Observations related to Gist vacuum

[Amit Kapila]

On Fri, Oct 18, 2019 at 9:41 AM Dilip Kumar  wrote:
>
> On Wed, Oct 16, 2019 at 7:22 PM Heikki Linnakangas  wrote:
> >
> > On 16 October 2019 12:57:03 CEST, Amit Kapila  
> > wrote:
> > >On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas 
> > >wrote:
> > >> All things
> > >> considered, I'm not sure which is better.
> > >
> > >Yeah, this is a tough call to make, but if we can allow it to delete
> > >the pages in bulkdelete conditionally for parallel vacuum workers,
> > >then it would be better.
> >
> > Yeah, if it's needed for parallel vacuum, maybe that tips the scale.
>
> Are we planning to do this only if it is called from parallel vacuum
> workers or in general?
>

I think we can do it in general as adding some check for parallel
vacuum there will look bit hackish.  It is not clear if we get enough
benefit by keeping it for cleanup phase of the index as discussed in
emails above.  Heikki, others, let us know if you don't agree here.

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

Re: Questions/Observations related to Gist vacuum

[Amit Kapila]

On Fri, Oct 18, 2019 at 9:34 AM Dilip Kumar  wrote:
>
> On Thu, Oct 17, 2019 at 6:32 PM Dilip Kumar  wrote:
> >
> > On Thu, 17 Oct 2019, 14:59 Amit Kapila,  wrote:
> >>
> >> On Thu, Oct 17, 2019 at 1:47 PM Dilip Kumar  wrote:
> >> >
> >> > On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas  
> >> > wrote:
> >> > >
> >> > > Thanks! Looks good to me. Did either of you test it, though, with a
> >> > > multi-pass vacuum?
> >> >
> >> > From my side, I have tested it with the multi-pass vacuum using the
> >> > gist index and after the fix, it's using expected memory context.
> >> >
> >>
> >> I have also verified that, but I think what additionally we can test
> >> here is that without the patch it will leak the memory in
> >> TopTransactionContext (CurrentMemoryContext), but after patch it
> >> shouldn't leak it during multi-pass vacuum.
> >>
> >> Make sense to me, I will test that by tomorrow.
>
> I have performed the test to observe the memory usage in the
> TopTransactionContext using the MemoryContextStats function from gdb.
>
> For testing this, in gistvacuumscan every time, after it resets the
> page_set_context, I have collected the sample.  I have collected 3
> samples for both the head and the patch.  We can clearly see that on
> the head the memory is getting accumulated in the
> TopTransactionContext whereas with the patch there is no memory
> getting accumulated.
>

Thanks for the test.  It shows that prior to patch the memory was
getting leaked in TopTransactionContext during multi-pass vacuum and
after patch, there is no leak.  I will commit the patch early next
week unless Heikki or someone wants some more tests.

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

Re: SegFault on 9.6.14

[Amit Kapila]

On Thu, Oct 17, 2019 at 10:51 AM Thomas Munro  wrote:
>
> On Fri, Sep 13, 2019 at 1:35 AM Robert Haas  wrote:
> > On Thu, Sep 12, 2019 at 8:55 AM Amit Kapila  wrote:
> > > Robert, Thomas, do you have any more suggestions related to this.  I
> > > am planning to commit the above-discussed patch (Forbid Limit node to
> > > shutdown resources.) coming Monday, so that at least the reported
> > > problem got fixed.
> >
> > I think that your commit message isn't very clear about what the
> > actual issue is.  And the patch itself doesn't add any comments or
> > anything to try to clear it up. So I wouldn't favor committing it in
> > this form.
>
> Is the proposed commit message at the bottom of this email an improvement?
>
> Do I understand correctly that, with this patch, we can only actually
> lose statistics in the case where we rescan?
>

No, it will lose without rescan as well.  To understand in detail, you
might want to read the emails pointed by me in one of the above email
[1] in this thread.

>  That is, precisely the
> case that crashes (9.6) or spews warnings (10+)?  In a quick
> non-rescan test with the ExecShutdownNode() removed, I don't see any
> problem with the buffer numbers on my screen:
>

Try by removing aggregate function.  Basically, the Limit node has to
finish before consuming all the rows sent by a parallel node beneath
it.

>
> ===
> Don't shut down Gather[Merge] early under Limit.
>
> Revert part of commit 19df1702f5.
>
> Early shutdown was added by that commit so that we could collect
> statistics from workers, but unfortunately it interacted badly with
> rescans.  Rescanning a Limit over a Gather node could produce a SEGV
> on 9.6 and resource leak warnings on later releases.  By reverting the
> early shutdown code, we might lose statistics in some cases of Limit
> over Gather, but that will require further study to fix.
>

How about some text like below?  I have added slightly different text
to explain the reason for the problem.

"Early shutdown was added by that commit so that we could collect
statistics from workers, but unfortunately, it interacted badly with
rescans.  The problem is that we ended up destroying the parallel
context which is required for rescans.  This leads to rescans of a
Limit node over a Gather node to produce unpredictable results as it
tries to access destroyed parallel context.  By reverting the early
shutdown code, we might lose statistics in some cases of Limit over
Gather, but that will require further study to fix."

I am not sure but we can even add a comment in the place where we are
removing some code (in nodeLimit.c) to indicate that 'Ideally we
should shutdown parallel resources here to get the correct stats, but
that would lead to rescans misbehaving when there is a Gather [Merge]
node beneath it.  (Explain the reason for misbehavior and the ideas we
discussed in this thread to fix the same) ."

I can try to come up with comments in nodeLimit.c on the above lines
if we think that is a good idea?

[1] - 
https://www.postgresql.org/message-id/CAA4eK1Ja-eoavXcr0eq7w7hP%3D64VP49k%3DNMFxwhtK28NHfBOdA%40mail.gmail.com

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

Re: Questions/Observations related to Gist vacuum

[Dilip Kumar]

On Wed, Oct 16, 2019 at 7:22 PM Heikki Linnakangas  wrote:
>
> On 16 October 2019 12:57:03 CEST, Amit Kapila  wrote:
> >On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas 
> >wrote:
> >> All things
> >> considered, I'm not sure which is better.
> >
> >Yeah, this is a tough call to make, but if we can allow it to delete
> >the pages in bulkdelete conditionally for parallel vacuum workers,
> >then it would be better.
>
> Yeah, if it's needed for parallel vacuum, maybe that tips the scale.

Are we planning to do this only if it is called from parallel vacuum
workers or in general?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Questions/Observations related to Gist vacuum

[Dilip Kumar]

On Thu, Oct 17, 2019 at 6:32 PM Dilip Kumar  wrote:
>
> On Thu, 17 Oct 2019, 14:59 Amit Kapila,  wrote:
>>
>> On Thu, Oct 17, 2019 at 1:47 PM Dilip Kumar  wrote:
>> >
>> > On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas  
>> > wrote:
>> > >
>> > > On 17/10/2019 05:31, Amit Kapila wrote:
>> > > >
>> > > > The patch looks good to me.  I have slightly modified the comments and
>> > > > removed unnecessary initialization.
>> > > >
>> > > > Heikki, are you fine me committing and backpatching this to 12?  Let
>> > > > me know if you have a different idea to fix.
>> > >
>> > > Thanks! Looks good to me. Did either of you test it, though, with a
>> > > multi-pass vacuum?
>> >
>> > From my side, I have tested it with the multi-pass vacuum using the
>> > gist index and after the fix, it's using expected memory context.
>> >
>>
>> I have also verified that, but I think what additionally we can test
>> here is that without the patch it will leak the memory in
>> TopTransactionContext (CurrentMemoryContext), but after patch it
>> shouldn't leak it during multi-pass vacuum.
>>
>> Make sense to me, I will test that by tomorrow.

I have performed the test to observe the memory usage in the
TopTransactionContext using the MemoryContextStats function from gdb.

For testing this, in gistvacuumscan every time, after it resets the
page_set_context, I have collected the sample.  I have collected 3
samples for both the head and the patch.  We can clearly see that on
the head the memory is getting accumulated in the
TopTransactionContext whereas with the patch there is no memory
getting accumulated.

head:
TopTransactionContext: 1056832 total in 2 blocks; 3296 free (5
chunks); 1053536 used
  GiST VACUUM page set context: 112 total in 0 blocks (0 chunks); 0
free (0 chunks); 112 used
Grand total: 1056944 bytes in 2 blocks; 3296 free (5 chunks); 1053648 used

TopTransactionContext: 1089600 total in 4 blocks; 19552 free (14
chunks); 1070048 used
  GiST VACUUM page set context: 112 total in 0 blocks (0 chunks); 0
free (0 chunks); 112 used
Grand total: 1089712 bytes in 4 blocks; 19552 free (14 chunks); 1070160 used

TopTransactionContext: 1122368 total in 5 blocks; 35848 free (20
chunks); 1086520 used
  GiST VACUUM page set context: 112 total in 0 blocks (0 chunks); 0
free (0 chunks); 112 used
Grand total: 1122480 bytes in 5 blocks; 35848 free (20 chunks); 1086632 used


With Patch:
TopTransactionContext: 1056832 total in 2 blocks; 3296 free (1
chunks); 1053536 used
  GiST VACUUM page set context: 112 total in 0 blocks (0 chunks); 0
free (0 chunks); 112 used
Grand total: 1056944 bytes in 2 blocks; 3296 free (1 chunks); 1053648 used

TopTransactionContext: 1056832 total in 2 blocks; 3296 free (1
chunks); 1053536 used
  GiST VACUUM page set context: 112 total in 0 blocks (0 chunks); 0
free (0 chunks); 112 used
Grand total: 1056944 bytes in 2 blocks; 3296 free (1 chunks); 1053648 used

TopTransactionContext: 1056832 total in 2 blocks; 3296 free (1
chunks); 1053536 used
  GiST VACUUM page set context: 112 total in 0 blocks (0 chunks); 0
free (0 chunks); 112 used
Grand total: 1056944 bytes in 2 blocks; 3296 free (1 chunks); 1053648 used

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

RE: Can you please tell us how set this prefetch attribute in following lines.

[M Tarkeshwar Rao]

Hi all,



How to fetch certain number of tuples from a postgres table.



Same I am doing in oracle using following lines by setting prefetch attribute.



For oracle
// Prepare query
if( OCIStmtPrepare( myOciStatement, myOciError, (text *)aSqlStatement,
// Get statement type
 OCIAttrGet( (void *)myOciStatement, OCI_HTYPE_STMT, _type, 0, 
OCI_ATTR_STMT_TYPE, myOciError );
// Set prefetch count
  OCIAttrSet( myOciStatement, OCI_HTYPE_STMT, , 0, 
OCI_ATTR_PREFETCH_ROWS, myOciError );
// Execute query
status = OCIStmtExecute( myOciServerCtx, myOciStatement, myOciError, iters, 0, 
NULL, NULL, OCI_DEFAULT );


For Postgres



Can you please tell us how set this prefetch attribute in following lines. Is 
PQexec returns all the rows from the table?


mySqlResultsPG = PQexec(connection, aSqlStatement);
if((PQresultStatus(mySqlResultsPG) == PGRES_FATAL_ERROR ) || 
(PQstatus(connection) != CONNECTION_OK)){}
if ((PQresultStatus(mySqlResultsPG) == PGRES_COMMAND_OK) || 
(PQresultStatus(mySqlResultsPG) == PGRES_TUPLES_OK))
{
myNumColumns = PQnfields(mySqlResultsPG);
myTotalNumberOfRowsInQueryResult = PQntuples(mySqlResultsPG);
myCurrentRowNum = 0 ;
}



Regards

Tarkeshwar

Re: maintenance_work_mem used by Vacuum

[Masahiko Sawada]

On Fri, Oct 18, 2019, 11:43 Amit Kapila  wrote:

> On Thu, Oct 17, 2019 at 3:10 PM Tom Lane  wrote:
> >
> > Amit Kapila  writes:
> > > Another idea could be each index AM tell whether it uses
> > > maintainence_work_mem or not and based on that we can do the
> > > computation (divide the maintainence_work_mem by the number of such
> > > indexes during parallel vacuum).
> >
> > FWIW, that seems like a perfectly reasonable API addition to me.
> >
>
> Thanks,  Sawada-san, if you also think this API makes sense, then we
> can try to write a patch and see how it turns out?
>
>
 Yeah agreed. I can write this patch next week and will
share it.

Regards,

Re: [HACKERS] Block level parallel vacuum

[Dilip Kumar]

On Thu, Oct 17, 2019 at 4:00 PM Amit Kapila  wrote:
>
> On Thu, Oct 17, 2019 at 3:25 PM Dilip Kumar  wrote:
> >
> > On Thu, Oct 17, 2019 at 2:12 PM Masahiko Sawada  
> > wrote:
> > >
> > > On Thu, Oct 17, 2019 at 5:30 PM Amit Kapila  
> > > wrote:
> > > >
> > > > Another point in this regard is that the user anyway has an option to
> > > > turn off the cost-based vacuum.  By default, it is anyway disabled.
> > > > So, if the user enables it we have to provide some sensible behavior.
> > > > If we can't come up with anything, then, in the end, we might want to
> > > > turn it off for a parallel vacuum and mention the same in docs, but I
> > > > think we should try to come up with a solution for it.
> > >
> > > I finally got your point and now understood the need. And the idea I
> > > proposed doesn't work fine.
> > >
> > > So you meant that all workers share the cost count and if a parallel
> > > vacuum worker increase the cost and it reaches the limit, does the
> > > only one worker sleep? Is that okay even though other parallel workers
> > > are still running and then the sleep might not help?
> > >
>
> Remember that the other running workers will also increase
> VacuumCostBalance and whichever worker finds that it becomes greater
> than VacuumCostLimit will reset its value and sleep.  So, won't this
> make sure that overall throttling works the same?
>
> > I agree with this point.  There is a possibility that some of the
> > workers who are doing heavy I/O continue to work and OTOH other
> > workers who are doing very less I/O might become the victim and
> > unnecessarily delay its operation.
> >
>
> Sure, but will it impact the overall I/O?  I mean to say the rate
> limit we want to provide for overall vacuum operation will still be
> the same.  Also, isn't a similar thing happens now also where heap
> might have done a major portion of I/O but soon after we start
> vacuuming the index, we will hit the limit and will sleep.

Actually, What I meant is that the worker who performing actual I/O
might not go for the delay and another worker which has done only CPU
operation might pay the penalty?  So basically the worker who is doing
CPU intensive operation might go for the delay and pay the penalty and
the worker who is performing actual I/O continues to work and do
further I/O.  Do you think this is not a practical problem?

Stepping back a bit,  OTOH, I think that we can not guarantee that the
one worker who has done more I/O will continue to do further I/O and
the one which has not done much I/O will not perform more I/O in
future.  So it might not be too bad if we compute shared costs as you
suggested above.

>
> I think this might not be the perfect solution and we should try to
> come up with something else if this doesn't seem to be working.  Have
> you guys thought about the second solution I mentioned in email [1]
> (Before launching workers, we need to compute the remaining I/O )?
>  Any other better ideas?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: libpq: Fix wrong connection status on invalid "connect_timeout"

[Michael Paquier]

On Thu, Oct 17, 2019 at 10:10:17PM +0200, Lars Kanis wrote:
> That's why I changed connectDBComplete() only, instead of setting the
> status directly in parse_int_param().

Yes, you shouldn't do that as the keepalive parameters and
tcp_user_timeout have some specific handling when it comes to defaults
depending on the platform and we have some retry logic when specifying
multiple hosts.

Now, there is actually more to it than it looks at first glance.  Your
patch is pointing out at a failure within the regression tests of the
ECPG driver, as any parameters part of a connection string may have
trailing spaces which are considered as incorrect by the patch,
causing the connection to fail.

In short, on HEAD this succeeds but would fail with your patch:
$ psql 'postgresql:///postgres?host=/tmp_timeout=14 =5432'
psql: error: could not connect to server: invalid integer value "14 "
for connection option "connect_timeout"

Parameter names are more restrictive, as URLs don't allow leading or
trailing spaces for them.  On HEAD, we allow leading spaces for
integer parameters as the parsing uses strtol(3), but not for the
trailing spaces, which is a bit crazy and I think that we had better
not break that if the parameter value correctly defines a proper
integer.  So attached is a patch to skip trailing whitespaces as well,
which also fixes the issue with ECPG.  I have refactored the parsing
logic a bit while on it.  The comment at the top of parse_int_param()
needs to be reworked a bit more.

We could add some TAP tests for that, but I don't see a good area to
check after connection parameters.  We have tests for multi-host
strings in 001_stream_rep.pl but that already feels misplaced as those
tests are for recovery.  Perhaps we could add directly regression
tests for libpq.  I'll start a new thread about that once we are done
here, the topic is larger.

(Note to self: Ed Morley needs to be credited for the report as well.)
--
Michael
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f91f0f2efe..9af830321c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1687,7 +1687,7 @@ useKeepalives(PGconn *conn)
 /*
  * Parse and try to interpret "value" as an integer value, and if successful,
  * store it in *result, complaining if there is any trailing garbage or an
- * overflow.
+ * overflow.  This allows any number of leading and trailing whitespaces.
  */
 static bool
 parse_int_param(const char *value, int *result, PGconn *conn,
@@ -1698,14 +1698,31 @@ parse_int_param(const char *value, int *result, PGconn *conn,
 
 	*result = 0;
 
+	/* strtol(3) skips leading whitespaces */
 	errno = 0;
 	numval = strtol(value, , 10);
-	if (errno == 0 && *end == '\0' && numval == (int) numval)
-	{
-		*result = numval;
-		return true;
-	}
 
+	/*
+	 * If no progress was done during the parsing or an error happened, fail.
+	 * This tests properly for overflows of the result.
+	 */
+	if (value == end || errno != 0 || numval != (int) numval)
+		goto error;
+
+	/*
+	 * Skip any trailing whitespace; if anything but whitespace remains before
+	 * the terminating character, fail
+	 */
+	while (*end && *end != '\0' && isspace((unsigned char) *end))
+		end++;
+
+	if (*end && *end != '\0')
+		goto error;
+
+	*result = numval;
+	return true;
+
+error:
 	appendPQExpBuffer(>errorMessage,
 	  libpq_gettext("invalid integer value \"%s\" for connection option \"%s\"\n"),
 	  value, context);
@@ -2008,7 +2025,11 @@ connectDBComplete(PGconn *conn)
 	{
 		if (!parse_int_param(conn->connect_timeout, , conn,
 			 "connect_timeout"))
+		{
+			/* mark the connection as bad to report the parsing failure */
+			conn->status = CONNECTION_BAD;
 			return 0;
+		}
 
 		if (timeout > 0)
 		{


signature.asc
Description: PGP signature

Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays

[Amit Kapila]

On Thu, Oct 17, 2019 at 4:58 PM Kyotaro Horiguchi
 wrote:
>
> At Thu, 17 Oct 2019 16:30:02 +0900, Michael Paquier  
> wrote in
> > On Thu, Oct 17, 2019 at 06:37:11PM +1300, Thomas Munro wrote:
> > > -1 for these macros.
> > >
> > > These are basic facts about the C language.  I hope C eventually
> > > supports {} like C++, so that you don't have to think hard about
> > > whether the first member is another struct, and recursively so … but
> > > since the macros can't help with that problem, what is the point?
> >
> > FWIW, I am not convinced that those macros are an improvement either.
>
> FWIW agreed. I might have put +1 if it had multpile definitions
> according to platforms, though.
>

Thanks, Thomas, Michael, and Horiguchi-San.  I think there are enough
votes on not using a macro that we can proceed with that approach.
This takes us back to what Smith, Peter has initially proposed [1].
I shall wait for a couple of days to see if someone would like to
argue otherwise and then review the proposed patch.

[1] - 
https://www.postgresql.org/message-id/201DD0641B056142AC8C6645EC1B5F62014B919631%40SYD1217

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

Re: maintenance_work_mem used by Vacuum

[Amit Kapila]

On Thu, Oct 17, 2019 at 3:10 PM Tom Lane  wrote:
>
> Amit Kapila  writes:
> > Another idea could be each index AM tell whether it uses
> > maintainence_work_mem or not and based on that we can do the
> > computation (divide the maintainence_work_mem by the number of such
> > indexes during parallel vacuum).
>
> FWIW, that seems like a perfectly reasonable API addition to me.
>

Thanks,  Sawada-san, if you also think this API makes sense, then we
can try to write a patch and see how it turns out?

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

Re: maintenance_work_mem used by Vacuum

[Amit Kapila]

On Thu, Oct 17, 2019 at 6:05 PM Masahiko Sawada  wrote:
>
> On Thu, Oct 17, 2019 at 6:13 PM Amit Kapila  wrote:
> >
> > On Wed, Oct 16, 2019 at 5:35 PM Masahiko Sawada  
> > wrote:
> > >
> > > On Wed, Oct 16, 2019 at 3:48 PM Amit Kapila  
> > > wrote:
> > > >
> > > >
> > > > It is not that currently, other indexes don't use any additional
> > > > memory (except for maintainence_work_mem).  For example, Gist index
> > > > can use memory for collecting empty leaf pages and internal pages.  I
> > > > am not sure if we can do anything for such cases.  The case for Gin
> > > > index seems to be clear and it seems to be having the risk of using
> > > > much more memory, so why not try to do something for it?
> > >
> > > Yeah gin indexes is clear now and I agree that we need to do something
> > > for it. But I'm also concerned third party index AMs. Similar to the
> > > problem related to IndexBulkDeleteResult structure that we're
> > > discussing on another thread I thought that we have the same problem
> > > on this.
> > >
> >
> > I understand your concern, but I am not sure what is a good way to
> > deal with it.  I think we can do something generic like divide the
> > maintainence_work_mem equally among workers, but then the indexes that
> > use maintainence_work_mem will suffer if the number of such indexes is
> > much less than the indexes that don't use maintainence_work_mem.
> > Another idea could be each index AM tell whether it uses
> > maintainence_work_mem or not and based on that we can do the
> > computation (divide the maintainence_work_mem by the number of such
> > indexes during parallel vacuum).  Do you have any other ideas for
> > this?
> >
>
> I was thinking the similar idea to the latter idea: ask index AM the
> amount of memory (that should be part of maintenance_work_mem) it will
> consume and then compute the new limit for both heap scan and index
> vacuuming based on that.
>

Oh, that would be tricky as compared to what I am proposing because it
might not be easy for indexAM to tell upfront the amount of memory it
needs.  I think we can keep it simple for now.

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

Re: "pg_ctl: the PID file ... is empty" at end of make check

[Thomas Munro]

On Fri, Oct 18, 2019 at 1:21 AM Tom Lane  wrote:
> Thomas Munro  writes:
> > On Tue, Oct 15, 2019 at 1:55 PM Tom Lane  wrote:
> >> and now prairiedog has shown it too:
> >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog=2019-10-14%2021%3A45%3A47
> >> which is positively fascinating, because prairiedog is running a
> >> bronze-age version of macOS that surely never heard of APFS.
> >> So this makes it look like this is a basic macOS bug that's not
> >> as filesystem-dependent as one might think.
>
> > Does https://github.com/macdice/unlinktest show the problem on that system?
>
> It does, though with a very low frequency:
>
> $ ./unlinktest
> $ ./unlinktest 1
> read 0 bytes, unexpected
> $ ./unlinktest 1
> read 0 bytes, unexpected
> read 0 bytes, unexpected
> $ ./unlinktest 1
> $
>
> The failure rate on my recent-vintage laptop is more like one
> failure every five loops.

Wow.  Ok, I'll add a note to the bug report to say it's reproducible
on "Darwin Kernel Version 8.11.0: Wed Oct 10 18:26:00 PDT 2007;
root:xnu-792.24.17~1/RELEASE_PPC" next time I'm near an Apple device
that will let me log into the bug thing.  On the off-chance that
someone from Apple stumbles on this and is interested, the Radar
number is rdar://46318934 and the title is "unlink(2) is not atomic
(kernel/file system bug)".

Re: v12.0: segfault in reindex CONCURRENTLY

[Michael Paquier]

On Thu, Oct 17, 2019 at 06:56:48AM -0300, Alvaro Herrera wrote:
> On 2019-Oct-17, Michael Paquier wrote:
>> pgstat_progress_end_command() is done for REINDEX CONCURRENTLY after
>> the concurrent drop, so it made sense to me to still report any PID
>> REINDEX CONC is waiting for at this stage.
> 
> Yeah, okay.  So let's talk about your proposed new comment.  First,
> there are two spots where WaitForLockers is called in index_drop and
> you're proposing to patch the second one.  I think we should patch the
> first one and reference that one from the second one.  I propose
> something like this (sorry for crude pasting):
>
> 

What you are proposing here sounds fine to me.  Perhaps you would
prefer to adjust the code yourself?
--
Michael


signature.asc
Description: PGP signature

RE: extension patch of CREATE OR REPLACE TRIGGER

[osumi.takami...@fujitsu.com]

Dear Tom Lane

Thank you so much for your comment.

> * Upthread you asked about changing the lock level to be AccessExclusiveLock 
> if
> the trigger already exists, but the patch doesn't actually do that.  Which is 
> fine by
> me, because that sounds like a perfectly bad idea. 

Why I suggested a discussion 
to make the lock level of C.O.R.T. stronger above comes from my concern.

I've worried about a case that
C.O.R.T. weak lock like ShareRowExclusiveLock allows 
one session to replace other session's trigger for new trigger by COMMIT;
As a result, the session is made to use the new one unintentionally.

As you can see below, the previous trigger is replaced by Session2 after 
applying this patch.
This seems to conflict with user's expectation to data consistency between 
sessions or 
to identify C.O.R.T with DROP TRIGGER (AcessExclusive) + CREATE TRIGGER in 
terms of lock level.

-- Preparation
create table my_table1 (id integer, name text);
create table my_table2 (id integer, name text);
CREATE OR REPLACE FUNCTION public.my_updateproc1() RETURNS trigger LANGUAGE 
plpgsql
 AS $function$
  begin
UPDATE my_table2 SET name = 'new ' WHERE id=OLD.id;
RETURN NULL;
  end;$function$;

CREATE OR REPLACE FUNCTION public.my_updateproc2() RETURNS trigger LANGUAGE 
plpgsql
 AS $function$
  begin
UPDATE my_table2 SET name = 'replace' WHERE id=OLD.id;
RETURN NULL;
  end;$function$;

CREATE OR REPLACE TRIGGER my_regular_trigger AFTER UPDATE
   ON my_table1 FOR EACH ROW EXECUTE PROCEDURE my_updateproc1();

--Session 1---
BEGIN;
select * from my_table1; -- Cause AccessShareLock here by referring to 
my_table1;

--Session 2---
BEGIN;
CREATE OR REPLACE TRIGGER my_regular_trigger
   AFTER UPDATE ON my_table1 FOR EACH ROW
 EXECUTE PROCEDURE my_updateproc2();
COMMIT;

--Session 1---
select pg_get_triggerdef(oid, true) from pg_trigger where tgrelid = 
'my_table1'::regclass AND tgname = 'my_regular_trigger'; 

 CREATE TRIGGER my_regular_trigger AFTER UPDATE ON my_table1 FOR EACH ROW 
EXECUTE FUNCTION my_updateproc2()
(1 row)

By the way, I've fixed other points of my previous patch.
> * I wouldn't recommend adding CreateConstraintEntry's new argument at the end.
I changed the order of CreateConstraintEntry function and its header comment.

Besides that,
> I'm not on board with the idea that testing trigger_exists three separate 
> times, in
> three randomly-different-looking ways, makes things more readable.  
I did code refactoring of the redundant and confusing part.

> We do not test \h output in any existing regression test
And off course, I deleted the \h test you mentioned above.  

Regards,
   Takamichi Osumi



CREATE_OR_REPLACE_TRIGGER_v04.patch
Description: CREATE_OR_REPLACE_TRIGGER_v04.patch

Re: Remaining calls of heap_close/heap_open in the tree

[Michael Paquier]

On Thu, Oct 17, 2019 at 12:34:44PM +0100, Dagfinn Ilmari Mannsåker wrote:
> Would it be possible to wrap them in some #if(n)def guard so that
> they're available when building out-of-tree extensions, but not when
> building postgres itself?

Not sure that's worth the trouble.  If there are no objections, I will
remove the compatibility macros.
--
Michael


signature.asc
Description: PGP signature

Re: v12 and pg_restore -f-

[Stephen Frost]

Greetings,

* Justin Pryzby (pry...@telsasoft.com) wrote:
> On Thu, Oct 17, 2019 at 12:24:10PM +0200, Tom Lane wrote:
> > Alternatively, we could revoke the requirement to use "-f -" in 12,
> > and wait a couple releases before enforcing it.  The fundamental
> > problem here is that we tried to go from "-f - doesn't work" to
> > "you must use -f -" with no grace period where "-f - is optional".
> > In hindsight that was a bad idea.
> 
> I'm going to make an argument in favour of keeping the enforcement of -f- in
> v12.
> 
> If there's no enforcement, I don't know if many people would naturally start 
> to
> use -f-, which means that tools which need to work across a wide range of
> (minor) versions may never confront this until it's enforced in v14/v15, at
> which point we probably end up revisiting the whole thing again.
> 
> Failing pg_restore forces people to confront the new/different behavior.  If 
> we
> defer failing for 2 years, it probably just means it'll be an issue again 2
> years from now.

Absolutely agreed on this- deferring the pain doesn't really change
things here.

> However, it's still an issue if (old) back branches (like 11.5) don't support
> -f-, and I think the differing behavior should be called out in the v12 
> release
> notes, as succinctly as possible.

I agree that we should call it out in the release notes, of course, and
that, in this case, it's alright to fix the '-f-' bug that exists in the
back branches as a bug and not something else.

> Also, I'm taking the opportunity to correct myself, before someone else does:
> 
> On Wed, Oct 16, 2019 at 02:28:40PM -0500, Justin Pryzby wrote:
> > And vendors (something like pgadmin) will end up "having to" write to a file
> > to be portable, or else check the full version, not just the major version.
> 
> I take back that part .. before v12, they'd get stdout by not specifying -f,
> and since 12.0 they'd get stdout with -f-.  No need to check the minor 
> version,
> since the "need to" specify -f- wouldn't be backpatched, of course.

Ah, yes, that's true.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: ICU for global collation

[Thomas Munro]

On Thu, Oct 17, 2019 at 3:52 PM Thomas Munro  wrote:
> I rebased this patch, and tweaked get_collation_action_version() very
> slightly so that you get collation version change detection (of the
> ersatz kind provided by commit d5ac14f9) for the default collation
> even when not using ICU.  Please see attached.

It should also remove the sentence I recently added to
alter_collation.sgml to say that the default collation doesn't have
version tracking.  Rereading that section, it's also clear that the
query introduced with:

   "The following query can be used to identify all collations in the current
   database that need to be refreshed and the objects that depend on them:"

… is wrong with this patch applied.  The right query is quite hard to
come up with, since we don't explicitly track dependencies on the
default collation.  That is, there is no pg_depend entry pointing from
the index to the collation when you write CREATE INDEX ON t(x) for a
text column using the default collation, but there is one when you
write CREATE INDEX ON t(x COLLATE "fr_FR"), or when you write CREATE
INDEX ON t(x) for a text column that was explicitly defined to use
COLLATE "fr_FR".  One solution is that we could start tracking those
dependencies explicitly too.

A preexisting problem with that query is that it doesn't report
transitive dependencies.  An index on t(x) of a user defined type
defined with CREATE TYPE my_type AS (x text COLLATE "fr_FR") doesn't
result in a pg_depend row from index to collation, so the query fails
to report that as an index needing to be rebuilt.  You could fix that
with a sprinkle of recursive magic, but you'd need a different kind of
magic to deal with transitive dependencies on the default collation
unless we start listing such dependencies explicitly.  In that
example, my_type would need to depend on collation "default".  You
can't just do some kind of search for transitive dependencies on type
"text", because they aren't tracked either.

In my longer term proposal to track per-dependency versions, either by
adding refobjversion to pg_depend or by creating another
pg_depend-like catalog, you'd almost certainly need to add an explicit
record for dependencies on the default collation.

Re: libpq: Fix wrong connection status on invalid "connect_timeout"

[Lars Kanis]

I verified that all other integer parameters properly set CONNECTION_BAD
in case of invalid values. These are:

* port
* keepalives_idle
* keepalives_interval
* keepalives_count
* tcp_user_timeout

That's why I changed connectDBComplete() only, instead of setting the
status directly in parse_int_param().

--

Kind Regards,
Lars Kanis


Am 17.10.19 um 20:04 schrieb Lars Kanis:
> Greetings,
>
> libpq since PostgreSQL-12 has stricter checks for integer values in
> connection parameters. They were introduced by commit
> https://github.com/postgres/postgres/commit/e7a2217978d9cbb2149bfcb4ef1e45716cfcbefb
> .
>
> However in case of "connect_timeout" such an invalid integer value leads
> to a connection status other than CONNECTION_OK or CONNECTION_BAD. The
> wrong parameter is therefore not properly reported to user space. This
> patch fixes this by explicit setting CONNECTION_BAD.
>
> The issue was raised on ruby-pg: https://github.com/ged/ruby-pg/issues/302
>
> It originally came up at Heroku:
> https://github.com/heroku/stack-images/issues/147
>
-- 
--
Kind Regards,
Lars Kanis

libpq: Fix wrong connection status on invalid "connect_timeout"

[Lars Kanis]

Greetings,

libpq since PostgreSQL-12 has stricter checks for integer values in
connection parameters. They were introduced by commit
https://github.com/postgres/postgres/commit/e7a2217978d9cbb2149bfcb4ef1e45716cfcbefb
.

However in case of "connect_timeout" such an invalid integer value leads
to a connection status other than CONNECTION_OK or CONNECTION_BAD. The
wrong parameter is therefore not properly reported to user space. This
patch fixes this by explicit setting CONNECTION_BAD.

The issue was raised on ruby-pg: https://github.com/ged/ruby-pg/issues/302

It originally came up at Heroku:
https://github.com/heroku/stack-images/issues/147

-- 

Kind Regards,
Lars Kanis

From f0c0dc3511c66a1e0fe0e4cc7ad2995f5f40bd7c Mon Sep 17 00:00:00 2001
From: Lars Kanis 
Date: Thu, 17 Oct 2019 19:37:51 +0200
Subject: [PATCH] Fix wrong connection status on invalid "connect_timeout"

Commit e7a2217978d9cbb2149bfcb4ef1e45716cfcbefb introduced
stricter checks for integer values in connection parameters.
However in case of "connect_timeout" such invalid integer
values leaded to a connection status other than CONNECTION_OK
or CONNECTION_BAD. This patch explicit sets CONNECTION_BAD.

Signed-off-by: Lars Kanis 
---
 src/interfaces/libpq/fe-connect.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f91f0f2..44d927e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2008,7 +2008,11 @@ connectDBComplete(PGconn *conn)
 	{
 		if (!parse_int_param(conn->connect_timeout, , conn,
 			 "connect_timeout"))
+		{
+			/* invalid integer */
+			conn->status = CONNECTION_BAD;
 			return 0;
+		}
 
 		if (timeout > 0)
 		{
-- 
2.17.1

Non working timeout detection in logical worker

[Julien Rouhaud]

Hello,

Jehan-Guillaume (in Cc) reported me today a problem with logical
replication, where in case of network issue the walsender is correctly
terminating at the given wal_sender_timeout but the logical worker
kept waiting indefinitely.

The issue is apparently a simple thinko, the timestamp of the last
received activity being unconditionally set at the beginning of the
main processing loop, making any reasonable timeout setting
ineffective.  Trivial patch to fix the problem attached.


fix_logical_worker_timeout-v1.diff
Description: Binary data

UPSERT on view does not find constraint by name

[Jeremy Finzel]

I'm not sure if this can be considered a bug or not, but it is perhaps
unexpected.  I found that when using a view that is simply select * from
table, then doing INSERT ... ON CONFLICT ON CONSTRAINT constraint_name on
that view, it does not find the constraint and errors out.  But it does
find the constraint if one lists the columns instead.

I did not find any mention of this specifically in the docs, or any
discussion on this topic after a brief search, and I have already asked my
stakeholder to change to using the column list as better practice anyway.
But in any case, I wanted to know if this is a known issue or not.

Thanks!
Jeremy

Re: Compressed pluggable storage experiments

[Alvaro Herrera]

On 2019-Oct-10, Ildar Musin wrote:

> 1. Unlike FDW API, in pluggable storage API there are no routines like
> "begin modify table" and "end modify table" and there is no shared
> state between insert/update/delete calls.

Hmm.  I think adding a begin/end to modifytable is a reasonable thing to
do (it'd be a no-op for heap and zheap I guess).

> 2. It looks like I cannot implement custom storage options. E.g. for
> compressed storage it makes sense to implement different compression
> methods (lz4, zstd etc.) and corresponding options (like compression
> level). But as i can see storage options (like fillfactor etc) are
> hardcoded and are not extensible. Possible solution is to use GUCs
> which would work but is not extremely convinient.

Yeah, the reloptions module is undergoing some changes.  I expect that
there will be a way to extend reloptions from an extension, at the end
of that set of patches.

> 3. A bit surprising limitation that in order to use bitmap scan the
> maximum number of tuples per page must not exceed 291 due to
> MAX_TUPLES_PER_PAGE macro in tidbitmap.c which is calculated based on
> 8kb page size. In case of 1mb page this restriction feels really
> limiting.

I suppose this is a hardcoded limit that needs to be fixed by patching
core as we make table AM more pervasive.

> 4. In order to use WAL-logging each page must start with a standard 24
> byte PageHeaderData even if it is needless for storage itself. Not a
> big deal though. Another (acutally documented) WAL-related limitation
> is that only generic WAL can be used within extension. So unless
> inserts are made in bulks it's going to require a lot of disk space to
> accomodate logs and wide bandwith for replication.

Not sure what to suggest.  Either you should ignore this problem, or
you should fix it.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: v12 and pg_restore -f-

[Justin Pryzby]

On Thu, Oct 17, 2019 at 12:24:10PM +0200, Tom Lane wrote:
> Alternatively, we could revoke the requirement to use "-f -" in 12,
> and wait a couple releases before enforcing it.  The fundamental
> problem here is that we tried to go from "-f - doesn't work" to
> "you must use -f -" with no grace period where "-f - is optional".
> In hindsight that was a bad idea.

I'm going to make an argument in favour of keeping the enforcement of -f- in
v12.

If there's no enforcement, I don't know if many people would naturally start to
use -f-, which means that tools which need to work across a wide range of
(minor) versions may never confront this until it's enforced in v14/v15, at
which point we probably end up revisiting the whole thing again.

Failing pg_restore forces people to confront the new/different behavior.  If we
defer failing for 2 years, it probably just means it'll be an issue again 2
years from now.

However, it's still an issue if (old) back branches (like 11.5) don't support
-f-, and I think the differing behavior should be called out in the v12 release
notes, as succinctly as possible.

Also, I'm taking the opportunity to correct myself, before someone else does:

On Wed, Oct 16, 2019 at 02:28:40PM -0500, Justin Pryzby wrote:
> And vendors (something like pgadmin) will end up "having to" write to a file
> to be portable, or else check the full version, not just the major version.

I take back that part .. before v12, they'd get stdout by not specifying -f,
and since 12.0 they'd get stdout with -f-.  No need to check the minor version,
since the "need to" specify -f- wouldn't be backpatched, of course.

Justin

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2019年10月11日 下午9:50，Konstantin Knizhnik  写道：
> 
> 
> 
> On 11.10.2019 15:15, 曾文旌(义从) wrote:
>> Dear Hackers,
>> 
>> This propose a way to develop global temporary tables in PostgreSQL.
>> 
>> I noticed that there is an "Allow temporary tables to exist as empty by 
>> default in all sessions" in the postgresql todolist.
>> https://wiki.postgresql.org/wiki/Todo 
>> 
>> In recent years, PG community had many discussions about global temp table 
>> (GTT) support. Previous discussion covered the following topics: 
>> (1)  The main benefit or function: GTT offers features like “persistent 
>> schema, ephemeral data”, which avoids catalog bloat and reduces catalog 
>> vacuum. 
>> (2)  Whether follows ANSI concept of temporary tables
>> (3)  How to deal with statistics, single copy of schema definition, relcache
>> (4)  More can be seen in 
>> https://www.postgresql.org/message-id/73954ab7-44d3-b37b-81a3-69bdcbb446f7%40postgrespro.ru
>>  
>> 
>> (5)  A recent implementation and design from Konstantin Knizhnik covered 
>> many functions of GTT: 
>> https://www.postgresql.org/message-id/attachment/103265/global_private_temp-1.patch
>>  
>> 
>> 
>> However, as pointed by Konstantin himself, the implementation still needs 
>> functions related to CLOG, vacuum, and MVCC visibility.
>> 
> 
> Just to clarify.
> I have now proposed several different solutions for GTT:
> 
> Shared vs. private buffers for GTT:
> 1. Private buffers. This is least invasive patch, requiring no changes in 
> relfilenodes.
> 2. Shared buffers. Requires changing relfilenode but supports parallel query 
> execution for GTT.
> 
> Access to GTT at replica:
> 1. Access is prohibited (as for original temp tables). No changes at all.
> 2. Tuples of temp tables are marked with forzen XID.  Minimal changes, 
> rollbacks are not possible.
> 3. Providing special XIDs for GTT at replica. No changes in CLOG are 
> required, but special MVCC visibility rules are used for GTT. Current 
> limitation: number of transactions accessing GTT at replica is limited by 2^32
> and bitmap of correspondent size has to be maintained (tuples of GTT are not 
> proceeded by vacuum and not frozen, so XID horizon never moved).
> 
> So except the limitation mentioned above (which I do not consider as 
> critical) there is only one problem which was not addressed: maintaining 
> statistics for GTT. 
> If all of the following conditions are true:
> 
> 1) GTT are used in joins
> 2) There are indexes defined for GTT
> 3) Size and histogram of GTT in different backends can significantly vary. 
> 4) ANALYZE was explicitly called for GTT
> 
> then query execution plan built in one backend will be also used for other 
> backends where it can be inefficient.
> I also do not consider this problem as "show stopper" for adding GTT to 
> Postgres.
When session A writes 1000 rows of data to gtt X, session B also uses X at 
the same time and it has 100 rows of different data. If B uses analyze to count 
the statistics of 10 rows of data and updates it to catalog.
Obviously, session A will get inaccurate query plan based on misaligned 
statistics when calculating the query plan for X related queries. Session A may 
think that table X is too small to be worth using index scan, but it is not. 
Each session needs to get the statistics of the self data to make the query 
plan.


> I still do not understand the opinion of community which functionality of GTT 
> is considered to be most important.
> But the patch with local buffers and no replica support is small enough to 
> become good starting point.
Yes ,the first step, we focus on complete basic functions of gtt (dml ddl index 
on gtt (MVCC visibility rules) storage).
Abnormal statistics can cause problems with index selection on gtt, so index on 
gtt and accurate statistical information is necessary.


> 
> -- 
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com 
> 
> The Russian Postgres Company 

Memory leak reported by address sanitizer in ECPGconnect/CRYPTO_zalloc

[Madars Vitolins]

Hi Folks,


I am doing some development for Enduro/X distributed transaction 
middleware XA driver emulation, so that it would be suitable for ecpg 
apps too, and so the program opens the connection with help of 
ECPGconnect() with assigned connection id, .e.g:


/**
 * Perform connect
 * @param conndata parsed connection data
 * @param connname connection name
 * @return NULL (connection failed) or connection object
 */
expublic PGconn * ndrx_pg_connect(ndrx_pgconnect_t *conndata, char 
*connname)

{
    PGconn *ret = NULL;

    NDRX_LOG(log_debug, "Establishing ECPG connection: [%s]", conndata);

    /* OK, try to open, with out autocommit please!
 */
    if (!ECPGconnect (__LINE__, conndata->c, conndata->url, conndata->user,
    conndata->password, connname, EXFALSE))
    {
    NDRX_LOG(log_error, "ECPGconnect failed, code %ld state: [%s]: %s",
    (long)sqlca.sqlcode, sqlca.sqlstate, 
sqlca.sqlerrm.sqlerrmc);

    ret = NULL;
    goto out;
    }

    ret = ECPGget_PGconn(connname);
    if (NULL==ret)
    {
    NDRX_LOG(log_error, "Postgres error: failed to get PQ 
connection!");

    ret = NULL;
    goto out;
    }

out:
  ...


And connection at the end of the program are closed by:


/**
 * disconnect from postgres
 * @param conn current connection object
 * @param connname connection name
 * @return EXSUCCEED/EXFAIL
 */
expublic int ndrx_pg_disconnect(PGconn *conn, char *connname)
{
    int ret = EXSUCCEED;

    NDRX_LOG(log_debug, "Closing ECPG connection: [%s]", connname);

    if (!ECPGdisconnect(__LINE__, connname))
    {
    NDRX_LOG(log_error, "ECPGdisconnect failed: %s",
    PQerrorMessage(conn));
    EXFAIL_OUT(ret);
    }
out:
    return ret;
}


I logs I have:

N:NDRX:5:b86e6a53: 
3940:7f4ccc218300:012:20191017:091547294:a_open_entry:tmi/xa.c:0365:atmi_xa_open_entry 
RMID=1
N:NDRX:5:b86e6a53: 
3940:7f4ccc218300:012:20191017:091547294:a_open_entry:switch.c:0295:Connection 
name: [20191017-91547294-11]
N:NDRX:5:b86e6a53: 
3940:7f4ccc218300:012:20191017:091547294:x_pg_connect:s/ecpg.c:0067:Establishing 
ECPG connection: []
N:NDRX:5:b86e6a53: 3940:7f4cdab329c0:001:20191017:091547296:_tpcontinue 
:return.c:0631:Long jumping to continue!


...

N:NDRX:5:b86e6a53: 
3940:7f4cdab329c0:001:20191017:091548579:_close_entry:tmi/xa.c:0404:atmi_xa_close_entry
N:NDRX:5:b86e6a53: 
3940:7f4cdab329c0:001:20191017:091548579:g_disconnect:s/ecpg.c:0102:Closing 
ECPG connection: [20191017-91546256-0]
N:NDRX:4:b86e6a53: 
3940:7f4cdab329c0:001:20191017:091548580:_close_entry:switch.c:0341:Connection 
closed



But the problem is that at times I get following leaks:

==3940==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 716 byte(s) in 3 object(s) allocated from:
    #0 0x7f4cd9a42b50 in __interceptor_malloc 
(/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
    #1 0x7f4cd43a4e58 in CRYPTO_zalloc 
(/usr/lib/x86_64-linux-gnu/libcrypto.so.1.1+0x17ae58)


Direct leak of 584 byte(s) in 1 object(s) allocated from:
    #0 0x7f4cd9a42b50 in __interceptor_malloc 
(/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
    #1 0x7f4cd43a4e58 in CRYPTO_zalloc 
(/usr/lib/x86_64-linux-gnu/libcrypto.so.1.1+0x17ae58)
    #2 0x7f4cd4ba1b5f in PQconnectPoll 
(/usr/lib/x86_64-linux-gnu/libpq.so.5+0xeb5f)

    #3 0x7f4cd4ba29de (/usr/lib/x86_64-linux-gnu/libpq.so.5+0xf9de)
    #4 0x7f4cd4ba5666 in PQconnectdbParams 
(/usr/lib/x86_64-linux-gnu/libpq.so.5+0x12666)
    #5 0x7f4cd4de89c2 in ECPGconnect 
(/usr/lib/x86_64-linux-gnu/libecpg.so.6+0xc9c2)
    #6 0x7f4cd50f9d73 in ndrx_pg_connect 
/home/user1/endurox/xadrv/postgres/ecpg.c:71
    #7 0x7f4cd50f6ce7 in xa_open_entry 
/home/user1/endurox/xadrv/postgres/pgswitch.c:297
    #8 0x7f4cd50f6ce7 in xa_open_entry_stat 
/home/user1/endurox/xadrv/postgres/pgswitch.c:785
    #9 0x7f4cd9403ef0 in atmi_xa_open_entry 
/home/user1/endurox/libatmi/xa.c:373

    #10 0x7f4cd940bcce in ndrx_tpopen /home/user1/endurox/libatmi/xa.c:1335
    #11 0x7f4cd93c27e2 in tpopen /home/user1/endurox/libatmi/atmi.c:468
    #12 0x55ae20f4e0f9 in tm_thread_init 
/home/user1/endurox/tmsrv/tmsrv.c:115

    #13 0x55ae20f4f3b8 in TPTMSRV_TH /home/user1/endurox/tmsrv/tmsrv.c:161
    #14 0x55ae20f6ca63 in poolthread_do 
/home/user1/endurox/libexthpool/thpool.c:370
    #15 0x7f4cd8bf36da in start_thread 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)


Indirect leak of 1728 byte(s) in 8 object(s) allocated from:
    #0 0x7f4cd9a42b50 in __interceptor_malloc 
(/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
    #1 0x7f4cd43a4e58 in CRYPTO_zalloc 
(/usr/lib/x86_64-linux-gnu/libcrypto.so.1.1+0x17ae58)


Can anybody give a hint where could be a problem?


program links libecpg and libpq.

$ psql --version
psql (PostgreSQL) 10.10 (Ubuntu 10.10-0ubuntu0.18.04.1)


Thanks a lot in advance,

Madars

Re: [PATCH] Race condition in logical walsender causes long postgresql shutdown delay

[Alvaro Herrera]

On 2019-Sep-26, Alvaro Herrera wrote:

> On 2019-Sep-26, Jeff Janes wrote:

> > Hi Alvaro, does this count as a review?
> 
> Well, I'm already a second pair of eyes for Craig's code, so I think it
> does :-)  I would have liked confirmation from Craig that my change
> looks okay to him too, but maybe we'll have to go without that.

There not being a third pair of eyes, I have pushed this.

Thanks!

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Questions/Observations related to Gist vacuum

[Dilip Kumar]

On Thu, 17 Oct 2019, 14:59 Amit Kapila,  wrote:

> On Thu, Oct 17, 2019 at 1:47 PM Dilip Kumar  wrote:
> >
> > On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas 
> wrote:
> > >
> > > On 17/10/2019 05:31, Amit Kapila wrote:
> > > >
> > > > The patch looks good to me.  I have slightly modified the comments
> and
> > > > removed unnecessary initialization.
> > > >
> > > > Heikki, are you fine me committing and backpatching this to 12?  Let
> > > > me know if you have a different idea to fix.
> > >
> > > Thanks! Looks good to me. Did either of you test it, though, with a
> > > multi-pass vacuum?
> >
> > From my side, I have tested it with the multi-pass vacuum using the
> > gist index and after the fix, it's using expected memory context.
> >
>
> I have also verified that, but I think what additionally we can test
> here is that without the patch it will leak the memory in
> TopTransactionContext (CurrentMemoryContext), but after patch it
> shouldn't leak it during multi-pass vacuum.
>
> Make sense to me, I will test that by tomorrow.

Re: maintenance_work_mem used by Vacuum

[Masahiko Sawada]

On Thu, Oct 17, 2019 at 6:13 PM Amit Kapila  wrote:
>
> On Wed, Oct 16, 2019 at 5:35 PM Masahiko Sawada  wrote:
> >
> > On Wed, Oct 16, 2019 at 3:48 PM Amit Kapila  wrote:
> > >
> > >
> > > It is not that currently, other indexes don't use any additional
> > > memory (except for maintainence_work_mem).  For example, Gist index
> > > can use memory for collecting empty leaf pages and internal pages.  I
> > > am not sure if we can do anything for such cases.  The case for Gin
> > > index seems to be clear and it seems to be having the risk of using
> > > much more memory, so why not try to do something for it?
> >
> > Yeah gin indexes is clear now and I agree that we need to do something
> > for it. But I'm also concerned third party index AMs. Similar to the
> > problem related to IndexBulkDeleteResult structure that we're
> > discussing on another thread I thought that we have the same problem
> > on this.
> >
>
> I understand your concern, but I am not sure what is a good way to
> deal with it.  I think we can do something generic like divide the
> maintainence_work_mem equally among workers, but then the indexes that
> use maintainence_work_mem will suffer if the number of such indexes is
> much less than the indexes that don't use maintainence_work_mem.
> Another idea could be each index AM tell whether it uses
> maintainence_work_mem or not and based on that we can do the
> computation (divide the maintainence_work_mem by the number of such
> indexes during parallel vacuum).  Do you have any other ideas for
> this?
>

I was thinking the similar idea to the latter idea: ask index AM the
amount of memory (that should be part of maintenance_work_mem) it will
consume and then compute the new limit for both heap scan and index
vacuuming based on that.

Regards,

--
Masahiko Sawada

Re: "pg_ctl: the PID file ... is empty" at end of make check

[Tom Lane]

Thomas Munro  writes:
> On Tue, Oct 15, 2019 at 1:55 PM Tom Lane  wrote:
>> and now prairiedog has shown it too:
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog=2019-10-14%2021%3A45%3A47
>> which is positively fascinating, because prairiedog is running a
>> bronze-age version of macOS that surely never heard of APFS.
>> So this makes it look like this is a basic macOS bug that's not
>> as filesystem-dependent as one might think.

> Does https://github.com/macdice/unlinktest show the problem on that system?

It does, though with a very low frequency:

$ ./unlinktest 
$ ./unlinktest 1
read 0 bytes, unexpected
$ ./unlinktest 1
read 0 bytes, unexpected
read 0 bytes, unexpected
$ ./unlinktest 1
$ 

The failure rate on my recent-vintage laptop is more like one
failure every five loops.

regards, tom lane

Re: Remaining calls of heap_close/heap_open in the tree

[Dagfinn Ilmari Mannsåker]

Andres Freund  writes:

> Hi,
>
> On 2019-10-17 06:58:27 -0300, Alvaro Herrera wrote:
>> On 2019-Oct-17, Michael Paquier wrote:
>> 
>> > On Thu, Oct 17, 2019 at 01:04:50AM -0700, Andres Freund wrote:
>> > > Wonder if it's worth removing the backward compat ones from master? I
>> > > don't quite think so, but...
>> > 
>> > I would vote for the removal so as we'll never see that again in
>> > core.  Let's see what others think here.
>> 
>> Agreed.  There are enough other API changes that if an external
>> extension wants to keep using heap_* in their code, they can add their
>> own defines anyway.
>
> There's plenty extensions that essentially only need to change
> heap_open/close to table_open/close between 11 and 12. And it's
> especially the simpler ones where that's the case.

Would it be possible to wrap them in some #if(n)def guard so that
they're available when building out-of-tree extensions, but not when
building postgres itself?

- ilmari
-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl

Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays

[Kyotaro Horiguchi]

At Thu, 17 Oct 2019 16:30:02 +0900, Michael Paquier  wrote 
in 
> On Thu, Oct 17, 2019 at 06:37:11PM +1300, Thomas Munro wrote:
> > -1 for these macros.
> > 
> > These are basic facts about the C language.  I hope C eventually
> > supports {} like C++, so that you don't have to think hard about
> > whether the first member is another struct, and recursively so … but
> > since the macros can't help with that problem, what is the point?
> 
> FWIW, I am not convinced that those macros are an improvement either.

FWIW agreed. I might have put +1 if it had multpile definitions
according to platforms, though.

> > I am reminded of an (apocryphal?) complaint from an old C FAQ about
> > people using #define BEGIN {.
> 
> This one?  Wow.
> http://c-faq.com/cpp/slm.html

I remember this.

Though the new macro proposed here doesn't completely seems to be a
so-called nonsyntactic macro, but the syntax using the macro looks
somewhat broken since it lacks {}, which should be there.

bool nulls[Natts_pg_collection] = INIT_ALL_ELEMS_ZERO;

We could abuse the macro for structs.

pgstattuple_type stat = INIT_ALL_ELEMS_ZERO;

This is correct in syntax, but seems completely broken.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: pgbench - extend initialization phase control

[Fabien COELHO]

Hello,

Failed regression test. It's necessary to change the first a in “allowed 
step characters are” to uppercase A in the regression test of 
002_pgbench_no_server.pl.


Argh. I think I ran the test, then stupidly updated the message afterwards 
to better match best practices, without rechecking:-(


The behavior of "g" is different between v12 and the patche, and 
backward compatibility is lost. In v12, BEGIN and COMMIT are specified 
only by choosing "g". It's a problem that backward compatibility is 
lost.


Somehow yes, but I do not see this as an actual problem from a functional 
point of view: it just means that if you use a 'dtgvp' with the newer 
version and if the inserts were to fail, then they are not under an 
explicit transaction, so previous inserts are not cleaned up. However, 
this is a pretty unlikely case, and anyway the error is reported, so any 
user would be expected not to go on after the initialization phase.


So basically I do not see the very small regression for an unlikely corner 
case to induce any problem in practice.


The benefit of controlling where begin/end actually occur is that it may 
have an impact on performance, and it allows to check that.


When using ( and ) with the -I, the documentation should indicate that double 
quotes are required,


Or single quotes, or backslash, if launch from the command line. I added a 
mention of escaping or protection in the doc in that case.



and  "v" not be able to enclose in ( and ).


That is a postgresql limitation, which may evolve. There could be others. 
I updated the doc to say that some commands may not work inside an 
explicit transaction.


When g is specified, null is inserted in the filler column of 
pgbentch_tellrs, acounts, branches. But when G is specified, empty 
string is inserted.


Indeed there is a small diff. ISTM that the actual filling with the 
initial client version is NULL for branches and tellers, and a 
blank-padded string for accounts.


I fixed the patch so that the end-result is the same with both g and G.


Do you have any intention of this difference?


Yes and no.

I intended that tellers & branches filler are filled, but I did not really 
notice that the client side was implicitely using NULL, although it says 
so in a comment. Although I'm not happy with the fact because it cheats 
with the benchmark design which requires the filler columns to be really 
filled and stored as is, it is indeed the place to change this (bad) 
behavior.


Attached a v4 with the updates described above.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index e3a0abb4c7..c5e4d17fd5 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -167,7 +167,7 @@ pgbench  options  d
 init_steps specifies the
 initialization steps to be performed, using one character per step.
 Each step is invoked in the specified order.
-The default is dtgvp.
+The default is dt(g)vp.
 The available steps are:
 
 
@@ -193,12 +193,40 @@ pgbench  options  d
   
  
  
-  g (Generate data)
+  ( (begin transaction)
+  
+   
+Emit a BEGIN.
+   
+   
+Beware that some steps may not work when called within an explicit transaction.
+   
+   
+Beware that using ( on the command line requires some protection or escaping.
+   
+  
+ 
+ 
+  g or G (Generate data, client or server side)
   

 Generate data and load it into the standard tables,
 replacing any data already present.

+   
+When data is generated server side, there is no progress output.
+   
+  
+ 
+ 
+  ) (commit transaction)
+  
+   
+Emit a COMMIT.
+   
+   
+Beware that using ) on the command line requires some protection or escaping.
+   
   
  
  
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e72ad0036e..597562248a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -130,7 +130,13 @@ static int	pthread_join(pthread_t th, void **thread_return);
 /
  * some configurable parameters */
 
-#define DEFAULT_INIT_STEPS "dtgvp"	/* default -I setting */
+/*
+ * we do all data generation in one transaction to enable the backend's
+ * data-loading optimizations
+ */
+#define DEFAULT_INIT_STEPS "dt(g)vp"	/* default -I setting */
+
+#define ALL_INIT_STEPS "dtgGvpf()"		/* all possible steps */
 
 #define LOG_STEP_SECONDS	5	/* seconds between log messages */
 #define DEFAULT_NXACTS	10		/* default nxacts */
@@ -626,7 +632,7 @@ usage(void)
 		   "  %s [OPTION]... [DBNAME]\n"
 		   "\nInitialization 

Re: Remaining calls of heap_close/heap_open in the tree

[Andres Freund]

Hi,

On 2019-10-17 06:58:27 -0300, Alvaro Herrera wrote:
> On 2019-Oct-17, Michael Paquier wrote:
> 
> > On Thu, Oct 17, 2019 at 01:04:50AM -0700, Andres Freund wrote:
> > > Wonder if it's worth removing the backward compat ones from master? I
> > > don't quite think so, but...
> > 
> > I would vote for the removal so as we'll never see that again in
> > core.  Let's see what others think here.
> 
> Agreed.  There are enough other API changes that if an external
> extension wants to keep using heap_* in their code, they can add their
> own defines anyway.

There's plenty extensions that essentially only need to change
heap_open/close to table_open/close between 11 and 12. And it's
especially the simpler ones where that's the case.

Greetings,

Andres Freund

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Thu, Oct 17, 2019 at 3:25 PM Dilip Kumar  wrote:
>
> On Thu, Oct 17, 2019 at 2:12 PM Masahiko Sawada  wrote:
> >
> > On Thu, Oct 17, 2019 at 5:30 PM Amit Kapila  wrote:
> > >
> > > Another point in this regard is that the user anyway has an option to
> > > turn off the cost-based vacuum.  By default, it is anyway disabled.
> > > So, if the user enables it we have to provide some sensible behavior.
> > > If we can't come up with anything, then, in the end, we might want to
> > > turn it off for a parallel vacuum and mention the same in docs, but I
> > > think we should try to come up with a solution for it.
> >
> > I finally got your point and now understood the need. And the idea I
> > proposed doesn't work fine.
> >
> > So you meant that all workers share the cost count and if a parallel
> > vacuum worker increase the cost and it reaches the limit, does the
> > only one worker sleep? Is that okay even though other parallel workers
> > are still running and then the sleep might not help?
> >

Remember that the other running workers will also increase
VacuumCostBalance and whichever worker finds that it becomes greater
than VacuumCostLimit will reset its value and sleep.  So, won't this
make sure that overall throttling works the same?

> I agree with this point.  There is a possibility that some of the
> workers who are doing heavy I/O continue to work and OTOH other
> workers who are doing very less I/O might become the victim and
> unnecessarily delay its operation.
>

Sure, but will it impact the overall I/O?  I mean to say the rate
limit we want to provide for overall vacuum operation will still be
the same.  Also, isn't a similar thing happens now also where heap
might have done a major portion of I/O but soon after we start
vacuuming the index, we will hit the limit and will sleep.

I think this might not be the perfect solution and we should try to
come up with something else if this doesn't seem to be working.  Have
you guys thought about the second solution I mentioned in email [1]
(Before launching workers, we need to compute the remaining I/O )?
 Any other better ideas?

[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2BySETHCaCnAsEC-dC4GSXaE2sNGMOgD6J%3DX%2BN43bBqJQ%40mail.gmail.com

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

Re: Remaining calls of heap_close/heap_open in the tree

[Alvaro Herrera]

On 2019-Oct-17, Michael Paquier wrote:

> On Thu, Oct 17, 2019 at 01:04:50AM -0700, Andres Freund wrote:
> > Wonder if it's worth removing the backward compat ones from master? I
> > don't quite think so, but...
> 
> I would vote for the removal so as we'll never see that again in
> core.  Let's see what others think here.

Agreed.  There are enough other API changes that if an external
extension wants to keep using heap_* in their code, they can add their
own defines anyway.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: v12.0: segfault in reindex CONCURRENTLY

[Alvaro Herrera]

On 2019-Oct-17, Michael Paquier wrote:

> On Thu, Oct 17, 2019 at 05:33:22AM -0300, Alvaro Herrera wrote:
> > Hmm, I wonder if it isn't the right solution to set 'progress' to false
> > in that spot, instead.  index_drop says it must only be called by the
> > dependency machinery; are we depending on that to pass-through the need
> > to update progress status?  I'm going over that code now.
> 
> pgstat_progress_end_command() is done for REINDEX CONCURRENTLY after
> the concurrent drop, so it made sense to me to still report any PID
> REINDEX CONC is waiting for at this stage.

Yeah, okay.  So let's talk about your proposed new comment.  First,
there are two spots where WaitForLockers is called in index_drop and
you're proposing to patch the second one.  I think we should patch the
first one and reference that one from the second one.  I propose
something like this (sorry for crude pasting):

 * Note: the reason we use actual lock acquisition here, rather than
 * just checking the ProcArray and sleeping, is that deadlock is
 * possible if one of the transactions in question is blocked trying
 * to acquire an exclusive lock on our table.  The lock code will
 * detect deadlock and error out properly.
 * 
 * Note: we report progress through WaitForLockers() unconditionally
 * here, even though it will only be used by REINDEX CONCURRENTLY and
 * not DROP INDEX CONCURRENTLY.
 */

and then

/*
 * Wait till every transaction that saw the old index state has
-* finished.
+* finished.  See above about progress reporting.
 */

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Block level parallel vacuum

[Dilip Kumar]

On Thu, Oct 17, 2019 at 2:12 PM Masahiko Sawada  wrote:
>
> On Thu, Oct 17, 2019 at 5:30 PM Amit Kapila  wrote:
> >
> > On Thu, Oct 17, 2019 at 12:21 PM Amit Kapila  
> > wrote:
> > >
> > > On Thu, Oct 17, 2019 at 10:56 AM Masahiko Sawada  
> > > wrote:
> > > >
> > > > I guess that the concepts of vacuum delay contradicts the concepts of
> > > > parallel vacuum. The concepts of parallel vacuum would be to use more
> > > > resource to make vacuum faster. Vacuum delays balances I/O during
> > > > vacuum in order to avoid I/O spikes by vacuum but parallel vacuum
> > > > rather concentrates I/O in shorter duration.
> > > >
> > >
> > > You have a point, but the way it is currently working in the patch
> > > doesn't make much sense.
> > >
> >
> > Another point in this regard is that the user anyway has an option to
> > turn off the cost-based vacuum.  By default, it is anyway disabled.
> > So, if the user enables it we have to provide some sensible behavior.
> > If we can't come up with anything, then, in the end, we might want to
> > turn it off for a parallel vacuum and mention the same in docs, but I
> > think we should try to come up with a solution for it.
>
> I finally got your point and now understood the need. And the idea I
> proposed doesn't work fine.
>
> So you meant that all workers share the cost count and if a parallel
> vacuum worker increase the cost and it reaches the limit, does the
> only one worker sleep? Is that okay even though other parallel workers
> are still running and then the sleep might not help?
>
I agree with this point.  There is a possibility that some of the
workers who are doing heavy I/O continue to work and OTOH other
workers who are doing very less I/O might become the victim and
unnecessarily delay its operation.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: maintenance_work_mem used by Vacuum

[Tom Lane]

Amit Kapila  writes:
> Another idea could be each index AM tell whether it uses
> maintainence_work_mem or not and based on that we can do the
> computation (divide the maintainence_work_mem by the number of such
> indexes during parallel vacuum).

FWIW, that seems like a perfectly reasonable API addition to me.

regards, tom lane

Re: Questions/Observations related to Gist vacuum

[Amit Kapila]

On Thu, Oct 17, 2019 at 1:47 PM Dilip Kumar  wrote:
>
> On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas  wrote:
> >
> > On 17/10/2019 05:31, Amit Kapila wrote:
> > >
> > > The patch looks good to me.  I have slightly modified the comments and
> > > removed unnecessary initialization.
> > >
> > > Heikki, are you fine me committing and backpatching this to 12?  Let
> > > me know if you have a different idea to fix.
> >
> > Thanks! Looks good to me. Did either of you test it, though, with a
> > multi-pass vacuum?
>
> From my side, I have tested it with the multi-pass vacuum using the
> gist index and after the fix, it's using expected memory context.
>

I have also verified that, but I think what additionally we can test
here is that without the patch it will leak the memory in
TopTransactionContext (CurrentMemoryContext), but after patch it
shouldn't leak it during multi-pass vacuum.

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

Re: maintenance_work_mem used by Vacuum

[Amit Kapila]

On Wed, Oct 16, 2019 at 5:35 PM Masahiko Sawada  wrote:
>
> On Wed, Oct 16, 2019 at 3:48 PM Amit Kapila  wrote:
> >
> >
> > It is not that currently, other indexes don't use any additional
> > memory (except for maintainence_work_mem).  For example, Gist index
> > can use memory for collecting empty leaf pages and internal pages.  I
> > am not sure if we can do anything for such cases.  The case for Gin
> > index seems to be clear and it seems to be having the risk of using
> > much more memory, so why not try to do something for it?
>
> Yeah gin indexes is clear now and I agree that we need to do something
> for it. But I'm also concerned third party index AMs. Similar to the
> problem related to IndexBulkDeleteResult structure that we're
> discussing on another thread I thought that we have the same problem
> on this.
>

I understand your concern, but I am not sure what is a good way to
deal with it.  I think we can do something generic like divide the
maintainence_work_mem equally among workers, but then the indexes that
use maintainence_work_mem will suffer if the number of such indexes is
much less than the indexes that don't use maintainence_work_mem.
Another idea could be each index AM tell whether it uses
maintainence_work_mem or not and based on that we can do the
computation (divide the maintainence_work_mem by the number of such
indexes during parallel vacuum).  Do you have any other ideas for
this?

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

Re: Change atoi to strtol in same place

[Kyotaro Horiguchi]

At Fri, 11 Oct 2019 23:27:54 -0500, Joe Nelson  wrote in 
> Here's v6 of the patch.
> 
> [x] Rebase on 20961ceaf0
> [x] Don't call exit(1) after pg_fatal()
> [x] Use Tom Lane's suggestion for %lld in _() string
> [x] Allow full unsigned 16-bit range for ports (don't disallow ports 0-1023)
> [x] Explicitly cast parsed values to smaller integers

Thank you for the new version.

By the way in the upthread,

At Tue, 8 Oct 2019 01:46:51 -0500, Joe Nelson  wrote in 
> > 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?

I agree to David in that it's better to avoid that kind of complexity
if possible. The significant point of separating them was that you
don't want to suggest a false value range for non-integer inputs.

Looking the latest patch, the wrong suggestions and the complexity
introduced by the %lld alternative are already gone. So I think we're
reaching the simple solution where pg_strtoint64_range doesn't need to
be involved in message building.

" must be an integer in the range (mm .. xx)"

Doesn't the generic message work for all kinds of failure here?

# It is also easy for transators than the split message case.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: v12.0: segfault in reindex CONCURRENTLY

[Michael Paquier]

On Thu, Oct 17, 2019 at 05:33:22AM -0300, Alvaro Herrera wrote:
> Hmm, I wonder if it isn't the right solution to set 'progress' to false
> in that spot, instead.  index_drop says it must only be called by the
> dependency machinery; are we depending on that to pass-through the need
> to update progress status?  I'm going over that code now.

pgstat_progress_end_command() is done for REINDEX CONCURRENTLY after
the concurrent drop, so it made sense to me to still report any PID
REINDEX CONC is waiting for at this stage.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Thu, Oct 17, 2019 at 5:30 PM Amit Kapila  wrote:
>
> On Thu, Oct 17, 2019 at 12:21 PM Amit Kapila  wrote:
> >
> > On Thu, Oct 17, 2019 at 10:56 AM Masahiko Sawada  
> > wrote:
> > >
> > > I guess that the concepts of vacuum delay contradicts the concepts of
> > > parallel vacuum. The concepts of parallel vacuum would be to use more
> > > resource to make vacuum faster. Vacuum delays balances I/O during
> > > vacuum in order to avoid I/O spikes by vacuum but parallel vacuum
> > > rather concentrates I/O in shorter duration.
> > >
> >
> > You have a point, but the way it is currently working in the patch
> > doesn't make much sense.
> >
>
> Another point in this regard is that the user anyway has an option to
> turn off the cost-based vacuum.  By default, it is anyway disabled.
> So, if the user enables it we have to provide some sensible behavior.
> If we can't come up with anything, then, in the end, we might want to
> turn it off for a parallel vacuum and mention the same in docs, but I
> think we should try to come up with a solution for it.

I finally got your point and now understood the need. And the idea I
proposed doesn't work fine.

So you meant that all workers share the cost count and if a parallel
vacuum worker increase the cost and it reaches the limit, does the
only one worker sleep? Is that okay even though other parallel workers
are still running and then the sleep might not help?

Regards,

--
Masahiko Sawada

Re: Zedstore - compressed in-core columnar storage

[Heikki Linnakangas]

On 15/10/2019 13:49, Ashutosh Sharma wrote:

Hi,

I got chance to spend some time looking into the recent changes done
in the zedstore code, basically the functions for packing datums into
the attribute streams and handling attribute leaf pages. I didn't find
any issues but there are some minor comments that I found when
reviewing. I have worked on those and attached is the patch with the
changes. See if the changes looks meaningful to you.


Thanks for looking! Applied to the development repository 
(https://github.com/greenplum-db/postgres/tree/zedstore/)


- Heikki

Re: pgbench - extend initialization phase control

[btendouan]

Hi,

When g is specified, null is inserted in the filler column of 
pgbentch_tellrs, acounts, branches.

But when G is specified, empty string is inserted.

Do you have any intention of this difference?

--
Anna

Re: v12.0: segfault in reindex CONCURRENTLY

[Alvaro Herrera]

On 2019-Oct-17, Michael Paquier wrote:

> You may not have a backtrace, but I think that you are right:
> WaitForLockers() gets called in index_drop() with progress reporting
> enabled.  index_drop() would also be taken by REINDEX CONCURRENTLY
> through performMultipleDeletions() but we cannot know if it gets used
> for REINDEX CONCURRENTLY or for DROP INDEX CONCURRENTLY as it goes
> through the central deletion machinery, so we have to mark progress
> reporting as true anyway.  Maybe that's worth a comment in index_drop
> when calling WaitForLockers() because it is not actually that obvious,
> say like that:

Hmm, I wonder if it isn't the right solution to set 'progress' to false
in that spot, instead.  index_drop says it must only be called by the
dependency machinery; are we depending on that to pass-through the need
to update progress status?  I'm going over that code now.


-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Thu, Oct 17, 2019 at 12:21 PM Amit Kapila  wrote:
>
> On Thu, Oct 17, 2019 at 10:56 AM Masahiko Sawada  
> wrote:
> >
> > I guess that the concepts of vacuum delay contradicts the concepts of
> > parallel vacuum. The concepts of parallel vacuum would be to use more
> > resource to make vacuum faster. Vacuum delays balances I/O during
> > vacuum in order to avoid I/O spikes by vacuum but parallel vacuum
> > rather concentrates I/O in shorter duration.
> >
>
> You have a point, but the way it is currently working in the patch
> doesn't make much sense.
>

Another point in this regard is that the user anyway has an option to
turn off the cost-based vacuum.  By default, it is anyway disabled.
So, if the user enables it we have to provide some sensible behavior.
If we can't come up with anything, then, in the end, we might want to
turn it off for a parallel vacuum and mention the same in docs, but I
think we should try to come up with a solution for it.

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

Re: Remaining calls of heap_close/heap_open in the tree

[Michael Paquier]

On Thu, Oct 17, 2019 at 01:04:50AM -0700, Andres Freund wrote:
> Wonder if it's worth removing the backward compat ones from master? I
> don't quite think so, but...

I would vote for the removal so as we'll never see that again in
core.  Let's see what others think here.
--
Michael


signature.asc
Description: PGP signature

Re: parallel restore sometimes fails for FKs to partitioned tables

[Alvaro Herrera]

On 2019-Oct-05, Alvaro Herrera wrote:

> 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.

Pushed with some additional tweaks.

Parallel restore of partitioned tables containing data and unique keys
still fails with some deadlock errors, though :-(

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Questions/Observations related to Gist vacuum

[Dilip Kumar]

On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas  wrote:
>
> On 17/10/2019 05:31, Amit Kapila wrote:
> > On Wed, Oct 16, 2019 at 11:20 AM Dilip Kumar  wrote:
> >>
> >> On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas  wrote:
> >>>
> >>> On 15/10/2019 09:37, Amit Kapila wrote:
>  While reviewing a parallel vacuum patch [1], we noticed a few things
>  about $SUBJECT implemented in commit -
>  7df159a620b760e289f1795b13542ed1b3e13b87.
> 
>  1. A new memory context GistBulkDeleteResult->page_set_context has
>  been introduced, but it doesn't seem to be used.
> >>>
> >>> Oops. internal_page_set and empty_leaf_set were supposed to be allocated
> >>> in that memory context. As things stand, we leak them until end of
> >>> vacuum, in a multi-pass vacuum.
> >>
> >> Here is a patch to fix this issue.
> >
> > The patch looks good to me.  I have slightly modified the comments and
> > removed unnecessary initialization.
> >
> > Heikki, are you fine me committing and backpatching this to 12?  Let
> > me know if you have a different idea to fix.
>
> Thanks! Looks good to me. Did either of you test it, though, with a
> multi-pass vacuum?

>From my side, I have tested it with the multi-pass vacuum using the
gist index and after the fix, it's using expected memory context.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Remaining calls of heap_close/heap_open in the tree

[Tom Lane]

Andres Freund  writes:
> On 2019-10-17 10:47:06 +0900, Michael Paquier wrote:
>> I have just bumped into $subject, and we now use the table_*
>> equivalents in the code.  Any objections to the simple patch attached
>> to clean up that?

> They're not really "remaining", as much as having been introduced after
> the introduction of table_open()/close()...

> Wonder if it's worth removing the backward compat ones from master? I
> don't quite think so, but...

If we don't remove 'em, we'll keep getting new calls from patches that
haven't been updated.

regards, tom lane

Re: Remaining calls of heap_close/heap_open in the tree

[Andres Freund]

Hi,

On 2019-10-17 10:47:06 +0900, Michael Paquier wrote:
> I have just bumped into $subject, and we now use the table_*
> equivalents in the code.  Any objections to the simple patch attached
> to clean up that?

They're not really "remaining", as much as having been introduced after
the introduction of table_open()/close()...

Wonder if it's worth removing the backward compat ones from master? I
don't quite think so, but...

Greetings,

Andres Freund

Re: Remove obsolete information schema tables

[Michael Paquier]

On Mon, Oct 14, 2019 at 10:27:14AM +0200, Peter Eisentraut wrote:
> I propose this patch to remove the information schema tables
> SQL_LANGUAGES, which was eliminated in SQL:2008, and SQL_PACKAGES, which
> was eliminated in SQL:2011.  Since they were dropped by the SQL
> standard, the information in them was no longer updated and therefore no
> longer useful.

The cleanup looks right.  I cannot grep missing references FWIW.

> This also removes the feature-package association information in
> sql_feature_packages.txt, but for the time begin we are keeping the
> information which features are in the Core package (that is, mandatory
> SQL features).  Maybe at some point someone wants to invent a way to
> store that that does not involve using the "package" mechanism
> anymore.

I have a question here.  Per the notes in information_schema.sql,
SQL_SIZING_PROFILES has been removed in SQL:2011,
attributes.isnullable and DOMAIN_UDT_USAGE in SQL:2003~.  Would it
make sense to cleanup those ones?
--
Michael


signature.asc
Description: PGP signature

Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays

[Michael Paquier]

On Thu, Oct 17, 2019 at 06:37:11PM +1300, Thomas Munro wrote:
> -1 for these macros.
> 
> These are basic facts about the C language.  I hope C eventually
> supports {} like C++, so that you don't have to think hard about
> whether the first member is another struct, and recursively so … but
> since the macros can't help with that problem, what is the point?

FWIW, I am not convinced that those macros are an improvement either.

> I am reminded of an (apocryphal?) complaint from an old C FAQ about
> people using #define BEGIN {.

This one?  Wow.
http://c-faq.com/cpp/slm.html
--
Michael


signature.asc
Description: PGP signature

Re: Clean up MinGW def file generation

[Michael Paquier]

On Tue, Oct 15, 2019 at 09:00:23AM +0200, Peter Eisentraut wrote:
> This doesn't make much sense (anymore?) since MinGW surely has sed and
> MSVC doesn't use this (and has Perl).  I think this is a leftover from
> various ancient client-only ad-hoc Windows build provisions (those
> win32.mak files we used to have around).  Also, the ddll.def (debug
> build) isn't used by anything anymore AFAICT.

sed is present in MinGW for some time, at least 2009 if you look here:
https://sourceforge.net/projects/mingw/files/MSYS/Base/sed/
Cygwin also includes sed, so this cleanup makes sense.

> I think we can clean this up and just have the regular ddl.def built
> normally at build time if required.
> 
> Does anyone know more about this?

This comes from here, but I cannot see a thread about this topic
around this date:
commit: a1d5d8574751d62a039d8ceb44329ee7c637196a
author: Peter Eisentraut 
date: Tue, 26 Feb 2008 06:41:24 +
Refactor the code that creates the shared library export files to appear
only once in Makefile.shlib and not in four copies.
--
Michael


signature.asc
Description: PGP signature

Re: Questions/Observations related to Gist vacuum

[Heikki Linnakangas]

On 17/10/2019 05:31, Amit Kapila wrote:

On Wed, Oct 16, 2019 at 11:20 AM Dilip Kumar  wrote:


On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas  wrote:


On 15/10/2019 09:37, Amit Kapila wrote:

While reviewing a parallel vacuum patch [1], we noticed a few things
about $SUBJECT implemented in commit -
7df159a620b760e289f1795b13542ed1b3e13b87.

1. A new memory context GistBulkDeleteResult->page_set_context has
been introduced, but it doesn't seem to be used.


Oops. internal_page_set and empty_leaf_set were supposed to be allocated
in that memory context. As things stand, we leak them until end of
vacuum, in a multi-pass vacuum.


Here is a patch to fix this issue.


The patch looks good to me.  I have slightly modified the comments and
removed unnecessary initialization.

Heikki, are you fine me committing and backpatching this to 12?  Let
me know if you have a different idea to fix.


Thanks! Looks good to me. Did either of you test it, though, with a 
multi-pass vacuum?


- Heikki

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Thu, Oct 17, 2019 at 10:56 AM Masahiko Sawada  wrote:
>
> On Wed, Oct 16, 2019 at 3:02 PM Amit Kapila  wrote:
> >
> > On Wed, Oct 16, 2019 at 6:50 AM Masahiko Sawada  
> > wrote:
> > >
> > > On Tue, Oct 15, 2019 at 6:33 PM Amit Kapila  
> > > wrote:
> > > >
> > >
> > > Attached updated patch set. 0001 patch introduces new index AM field
> > > amcanparallelvacuum. All index AMs except for gist sets true for now.
> > > 0002 patch incorporated the all comments I got so far.
> > >
> >
> > I haven't studied the latest patch in detail, but it seems you are
> > still assuming that all indexes will have the same amount of shared
> > memory for index stats and copying it in the same way.
>
> Yeah I thought we agreed at least to have canparallelvacuum and if an
> index AM cannot support parallel index vacuuming like gist, it returns
> false.
>
> > I thought we
> > agreed that each index AM should do this on its own.  The basic
> > problem is as of now we see this problem only with the Gist index, but
> > some other index AM's could also have a similar problem.
>
> Okay. I'm thinking we're going to have a new callback to ack index AMs
> the size of the structure using within both ambulkdelete and
> amvacuumcleanup. But copying it to DSM can be done by the core because
> it knows how many bytes need to be copied to DSM. Is that okay?
>

That sounds okay.

> >
> > Another major problem with previous and this patch version is that the
> > cost-based vacuum concept seems to be entirely broken.  Basically,
> > each parallel vacuum worker operates independently w.r.t vacuum delay
> > and cost.  Assume that the overall I/O allowed for vacuum operation is
> > X after which it will sleep for some time, reset the balance and
> > continue.  In the patch, each worker will be allowed to perform X
> > before which it can sleep and also there is no coordination for the
> > same with master backend.  This is somewhat similar to memory usage
> > problem, but a bit more tricky because here we can't easily split the
> > I/O for each of the worker.
> >
> > One idea could be that we somehow map vacuum costing related
> > parameters to the shared memory (dsm) which the vacuum operation is
> > using and then allow workers to coordinate.  This way master and
> > worker processes will have the same view of balance cost and can act
> > accordingly.
> >
> > The other idea could be that we come up with some smart way to split
> > the I/O among workers.  Initially, I thought we could try something as
> > we do for autovacuum workers (see autovac_balance_cost), but I think
> > that will require much more math.  Before launching workers, we need
> > to compute the remaining I/O (heap operation would have used
> > something) after which we need to sleep and continue the operation and
> > then somehow split it equally across workers.  Once the workers are
> > finished, then need to let master backend know how much I/O they have
> > consumed and then master backend can add it to it's current I/O
> > consumed.
> >
> > I think this problem matters because the vacuum delay is useful for
> > large vacuums and this patch is trying to exactly solve that problem,
> > so we can't ignore this problem.  I am not yet sure what is the best
> > solution to this problem, but I think we need to do something for it.
> >
>
> I guess that the concepts of vacuum delay contradicts the concepts of
> parallel vacuum. The concepts of parallel vacuum would be to use more
> resource to make vacuum faster. Vacuum delays balances I/O during
> vacuum in order to avoid I/O spikes by vacuum but parallel vacuum
> rather concentrates I/O in shorter duration.
>

You have a point, but the way it is currently working in the patch
doesn't make much sense.  Basically, each of the parallel workers will
be allowed to use a complete I/O limit which is actually a limit for
the entire vacuum operation.  It doesn't give any consideration to the
work done for the heap.

> Since we need to share
> the memory in entire system we need to deal with the memory issue but
> disks are different.
>
> If we need to deal with this problem how about just dividing
> vacuum_cost_limit by the parallel degree and setting it to worker's
> vacuum_cost_limit?
>

How will we take the I/O done by heap into consideration?  The
vacuum_cost_limit is the cost for the entire vacuum operation not
separately for heap and indexes.  What makes you think that
considering the limit for heap and index separately is not
problematic?


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

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Thu, Oct 17, 2019 at 3:18 PM Mahendra Singh  wrote:
>
> Hi
> I applied all 3 patches and ran regression test. I was getting one regression 
> failure.
>
>> diff -U3 
>> /home/mahendra/postgres_base_rp/postgres/src/test/regress/expected/vacuum.out
>>  /home/mahendra/postgres_base_rp/postgres/src/test/regress/results/vacuum.out
>> --- 
>> /home/mahendra/postgres_base_rp/postgres/src/test/regress/expected/vacuum.out
>>  2019-10-17 10:01:58.138863802 +0530
>> +++ 
>> /home/mahendra/postgres_base_rp/postgres/src/test/regress/results/vacuum.out 
>> 2019-10-17 11:41:20.930699926 +0530
>> @@ -105,7 +105,7 @@
>>  CREATE TEMPORARY TABLE tmp (a int PRIMARY KEY);
>>  CREATE INDEX tmp_idx1 ON tmp (a);
>>  VACUUM (PARALLEL 1) tmp; -- error, cannot parallel vacuum temporary tables
>> -WARNING:  skipping "tmp" --- cannot parallel vacuum temporary tables
>> +WARNING:  skipping vacuum on "tmp" --- cannot vacuum temporary tables in 
>> parallel
>>  -- INDEX_CLEANUP option
>>  CREATE TABLE no_index_cleanup (i INT PRIMARY KEY, t TEXT);
>>  -- Use uncompressed data stored in toast.
>
>
> It look likes that you changed warning message for temp table, but haven't 
> updated expected out file.
>

Thank you!
I forgot to change the expected file. I'll fix it in the next version patch.

Regards,

--
Masahiko Sawada

Re: [HACKERS] Block level parallel vacuum

[Mahendra Singh]

Hi
I applied all 3 patches and ran regression test. I was getting one
regression failure.

diff -U3
> /home/mahendra/postgres_base_rp/postgres/src/test/regress/expected/vacuum.out
> /home/mahendra/postgres_base_rp/postgres/src/test/regress/results/vacuum.out
> ---
> /home/mahendra/postgres_base_rp/postgres/src/test/regress/expected/vacuum.out
> 2019-10-17 10:01:58.138863802 +0530
> +++
> /home/mahendra/postgres_base_rp/postgres/src/test/regress/results/vacuum.out
> 2019-10-17 11:41:20.930699926 +0530
> @@ -105,7 +105,7 @@
>  CREATE TEMPORARY TABLE tmp (a int PRIMARY KEY);
>  CREATE INDEX tmp_idx1 ON tmp (a);
>  VACUUM (PARALLEL 1) tmp; -- error, cannot parallel vacuum temporary tables
> -WARNING:  skipping "tmp" --- cannot parallel vacuum temporary tables
> +WARNING:  skipping vacuum on "tmp" --- cannot vacuum temporary tables in
> parallel
>  -- INDEX_CLEANUP option
>  CREATE TABLE no_index_cleanup (i INT PRIMARY KEY, t TEXT);
>  -- Use uncompressed data stored in toast.
>

It look likes that you changed warning message for temp table, but haven't
updated expected out file.

Thanks and Regards
Mahendra Thalor
EnterpriseDB: http://www.enterprisedb.com

On Wed, 16 Oct 2019 at 06:50, Masahiko Sawada  wrote:

> On Tue, Oct 15, 2019 at 6:33 PM Amit Kapila 
> wrote:
> >
> > On Tue, Oct 15, 2019 at 1:26 PM Masahiko Sawada 
> wrote:
> > >
> > > On Tue, Oct 15, 2019 at 4:15 PM Masahiko Sawada 
> wrote:
> > > >
> > > > > > > If we avoid postponing deleting empty pages till the cleanup
> phase,
> > > > > > > then we don't have the problem for gist indexes.
> > > > > >
> > > > > > Yes. But considering your pointing out I guess that there might
> be
> > > > > > other index AMs use the stats returned from bulkdelete in the
> similar
> > > > > > way to gist index (i.e. using more larger structure of which
> > > > > > IndexBulkDeleteResult is just the first field). If we have the
> same
> > > > > > concern the parallel vacuum still needs to deal with that as you
> > > > > > mentioned.
> > > > > >
> > > > >
> > > > > Right, apart from some functions for memory allocation/estimation
> and
> > > > > stats copy, we might need something like amcanparallelvacuum, so
> that
> > > > > index methods can have the option to not participate in parallel
> > > > > vacuum due to reasons similar to gist or something else.  I think
> we
> > > > > can work towards this direction as this anyway seems to be required
> > > > > and till we reach any conclusion for gist indexes, you can mark
> > > > > amcanparallelvacuum for gist indexes as false.
> > > >
> > > > Agreed. I'll create a separate patch to add this callback and change
> > > > parallel vacuum patch so that it checks the participation of indexes
> > > > and then vacuums on un-participated indexes after parallel vacuum.
> > >
> > > amcanparallelvacuum is not necessary to be a callback, it can be a
> > > boolean field of IndexAmRoutine.
> > >
> >
> > Yes, it will be a boolean.  Note that for parallel-index scans, we
> > already have amcanparallel.
> >
>
> Attached updated patch set. 0001 patch introduces new index AM field
> amcanparallelvacuum. All index AMs except for gist sets true for now.
> 0002 patch incorporated the all comments I got so far.
>
> Regards,
>
> --
> Masahiko Sawada
>