Thread: Questions/Observations related to Gist vacuum

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.
2. Right now, in gistbulkdelete we make a note of empty leaf pages and
internals pages and then in the second pass during gistvacuumcleanup,
we delete all the empty leaf pages.  I was thinking why unlike nbtree,
we have delayed the deletion of empty pages till gistvacuumcleanup.  I
don't see any problem if we do this during gistbulkdelete itself
similar to nbtree, also I think there is some advantage in marking the
pages as deleted as early as possible.  Basically, if the vacuum
operation is canceled or errored out between gistbulkdelete and
gistvacuumcleanup, then I think the deleted pages could be marked as
recyclable very early in next vacuum operation.  The other advantage
of doing this during gistbulkdelete is we can avoid sharing
information between gistbulkdelete and gistvacuumcleanup which is
quite helpful for a parallel vacuum as the information is not trivial
(it is internally stored as in-memory Btree).   OTOH, there might be
some advantage for delaying the deletion of pages especially in the
case of multiple scans during a single VACUUM command.  We can
probably delete all empty leaf pages in one go which could in some
cases lead to fewer internal page reads.  However, I am not sure if it
is really advantageous to postpone the deletion as there seem to be
some downsides to it as well. I don't see it documented why unlike
nbtree we consider delaying deletion of empty pages till
gistvacuumcleanup, but I might be missing something.

Thoughts?

[1] - https://www.postgresql.org/message-id/CAA4eK1JEQ2y3uNucNopDjK8pse6xSe5%3D_oknoWfRQvAF%3DVqsBA%40mail.gmail.com

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

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.

> 2. Right now, in gistbulkdelete we make a note of empty leaf pages and
> internals pages and then in the second pass during gistvacuumcleanup,
> we delete all the empty leaf pages.  I was thinking why unlike nbtree,
> we have delayed the deletion of empty pages till gistvacuumcleanup.  I
> don't see any problem if we do this during gistbulkdelete itself
> similar to nbtree, also I think there is some advantage in marking the
> pages as deleted as early as possible.  Basically, if the vacuum
> operation is canceled or errored out between gistbulkdelete and
> gistvacuumcleanup, then I think the deleted pages could be marked as
> recyclable very early in next vacuum operation.  The other advantage
> of doing this during gistbulkdelete is we can avoid sharing
> information between gistbulkdelete and gistvacuumcleanup which is
> quite helpful for a parallel vacuum as the information is not trivial
> (it is internally stored as in-memory Btree).   OTOH, there might be
> some advantage for delaying the deletion of pages especially in the
> case of multiple scans during a single VACUUM command.  We can
> probably delete all empty leaf pages in one go which could in some
> cases lead to fewer internal page reads.  However, I am not sure if it
> is really advantageous to postpone the deletion as there seem to be
> some downsides to it as well. I don't see it documented why unlike
> nbtree we consider delaying deletion of empty pages till
> gistvacuumcleanup, but I might be missing something.

Hmm. The thinking is/was that removing the empty pages is somewhat 
expensive, because it has to scan all the internal nodes to find the 
downlinks to the to-be-deleted pages. Furthermore, it needs to scan all 
the internal pages (or at least until it has found all the downlinks), 
regardless of how many empty pages are being deleted. So it makes sense 
to do it only once, for all the empty pages. You're right though, that 
there would be advantages, too, in doing it after each pass. All things 
considered, I'm not sure which is better.

- Heikki

On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi> 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.

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

On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 15/10/2019 09:37, Amit Kapila wrote:
> > 2. Right now, in gistbulkdelete we make a note of empty leaf pages and
> > internals pages and then in the second pass during gistvacuumcleanup,
> > we delete all the empty leaf pages.  I was thinking why unlike nbtree,
> > we have delayed the deletion of empty pages till gistvacuumcleanup.  I
> > don't see any problem if we do this during gistbulkdelete itself
> > similar to nbtree, also I think there is some advantage in marking the
> > pages as deleted as early as possible.  Basically, if the vacuum
> > operation is canceled or errored out between gistbulkdelete and
> > gistvacuumcleanup, then I think the deleted pages could be marked as
> > recyclable very early in next vacuum operation.  The other advantage
> > of doing this during gistbulkdelete is we can avoid sharing
> > information between gistbulkdelete and gistvacuumcleanup which is
> > quite helpful for a parallel vacuum as the information is not trivial
> > (it is internally stored as in-memory Btree).   OTOH, there might be
> > some advantage for delaying the deletion of pages especially in the
> > case of multiple scans during a single VACUUM command.  We can
> > probably delete all empty leaf pages in one go which could in some
> > cases lead to fewer internal page reads.  However, I am not sure if it
> > is really advantageous to postpone the deletion as there seem to be
> > some downsides to it as well. I don't see it documented why unlike
> > nbtree we consider delaying deletion of empty pages till
> > gistvacuumcleanup, but I might be missing something.
>
> Hmm. The thinking is/was that removing the empty pages is somewhat
> expensive, because it has to scan all the internal nodes to find the
> downlinks to the to-be-deleted pages. Furthermore, it needs to scan all
> the internal pages (or at least until it has found all the downlinks),
> regardless of how many empty pages are being deleted. So it makes sense
> to do it only once, for all the empty pages. You're right though, that
> there would be advantages, too, in doing it after each pass.
>

I was thinking more about this and it seems that there could be more
cases where delaying the delete mark for pages can further delay the
recycling of pages.  It is quite possible that immediately after bulk
delete the value of nextFullXid (used as deleteXid) is X and during
vacuum clean up it can be X + N where the chances of N being large is
more when there are multiple passes of vacuum scan.  Now, if we would
have set the value of deleteXid as X, then there are more chances for
the next vacuum to recycle it.  I am not sure but it might be that in
the future, we could come up with something (say if we can recompute
RecentGlobalXmin again) where we can recycle pages of first index scan
in the next scan of the index during single vacuum operation.

This is just to emphasize the point that doing the delete marking of
pages in the same pass has advantages, otherwise, I understand that
there are advantages in delaying it as well.

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

I think we have below option w.r.t Gist indexes for parallel vacuum
a. don't allow Gist Index to participate in parallel vacuum
b. allow delete of leaf pages in bulkdelete for parallel worker
c. always allow deleting leaf pages in bulkdelete
d. Invent some mechanism to share all the Gist stats via shared memory

(a) is not a very good option, but it is a safe option as we can
extend it in the future and we might decide to go with it especially
if we can't decide among any other options. (b) would serve the need
but would add some additional checks in gistbulkdelete and will look
more like a hack.  (c) would be best, but I think it will be difficult
to be sure that is a good decision for all type of cases. (d) can
require a lot of effort and I am not sure if it is worth adding
complexity in the proposed patch.

Do you have any thoughts on this?

Just to give you an idea of the current parallel vacuum patch, the
master backend scans the heap and forms the dead tuple array in dsm
and then we launch one worker for each index based on the availability
of workers and share the dead tuple array with each worker.  Each
worker performs bulkdelete for the index.  In the end, we perform
cleanup of all the indexes either via worker or master backend based
on some conditions.

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

On 16 October 2019 12:57:03 CEST, Amit Kapila <amit.kapila16@gmail.com> wrote:
>On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi>
>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.

Hopefully, multi-pass vacuums are rare in practice. And we should lift the current 1 GB limit on the dead TID array,
replacingit with something more compact and expandable, to make multi-pass vacuums even more rare. So I don't think we
needto jump through many hoops to optimize the multi-pass case. 

- Heikki

On Wed, Oct 16, 2019 at 11:20 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi> 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.

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

On Wed, Oct 16, 2019 at 7:21 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 16 October 2019 12:57:03 CEST, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi>
> >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.
>

makes sense.  I think we can write a patch for it and prepare the
parallel vacuum patch on top of it.  Once the parallel vacuum is in a
committable shape, we can commit the gist-index related patch first
followed by parallel vacuum patch.

> Hopefully, multi-pass vacuums are rare in practice. And we should lift the current 1 GB limit on the dead TID array,
replacingit with something more compact and expandable, to make multi-pass vacuums even more rare. So I don't think we
needto jump through many hoops to optimize the multi-pass case. 
>

Yeah, that will be a good improvement.

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

On Thu, Oct 17, 2019 at 9:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Oct 16, 2019 at 7:21 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >
> > On 16 October 2019 12:57:03 CEST, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi>
> > >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.
> >
>
> makes sense.  I think we can write a patch for it and prepare the
> parallel vacuum patch on top of it.  Once the parallel vacuum is in a
> committable shape, we can commit the gist-index related patch first
> followed by parallel vacuum patch.

+1
I can write a patch for the same.

> > Hopefully, multi-pass vacuums are rare in practice. And we should lift the current 1 GB limit on the dead TID
array,replacing it with something more compact and expandable, to make multi-pass vacuums even more rare. So I don't
thinkwe need to jump through many hoops to optimize the multi-pass case. 
> >
>
> Yeah, that will be a good improvement.
+1

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

On 17/10/2019 05:31, Amit Kapila wrote:
> On Wed, Oct 16, 2019 at 11:20 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>
>> On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi> 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

On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 17/10/2019 05:31, Amit Kapila wrote:
> > On Wed, Oct 16, 2019 at 11:20 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >>
> >> On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi> 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

On Thu, Oct 17, 2019 at 1:47 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas <hlinnaka@iki.fi> 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

On Thu, Oct 17, 2019 at 6:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, 17 Oct 2019, 14:59 Amit Kapila, <amit.kapila16@gmail.com> wrote:
>>
>> On Thu, Oct 17, 2019 at 1:47 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> >
>> > On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas <hlinnaka@iki.fi> 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

On Wed, Oct 16, 2019 at 7:22 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 16 October 2019 12:57:03 CEST, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi>
> >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

On Fri, Oct 18, 2019 at 9:34 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Oct 17, 2019 at 6:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, 17 Oct 2019, 14:59 Amit Kapila, <amit.kapila16@gmail.com> wrote:
> >>
> >> On Thu, Oct 17, 2019 at 1:47 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >> >
> >> > On Thu, Oct 17, 2019 at 12:27 PM Heikki Linnakangas <hlinnaka@iki.fi> 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

On Fri, Oct 18, 2019 at 9:41 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Oct 16, 2019 at 7:22 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >
> > On 16 October 2019 12:57:03 CEST, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi>
> > >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

On Fri, Oct 18, 2019 at 10:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 18, 2019 at 9:41 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Oct 16, 2019 at 7:22 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > >
> > > On 16 October 2019 12:57:03 CEST, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >On Tue, Oct 15, 2019 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi>
> > > >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.
I agree with that point.
 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.

I have prepared a first version of the patch.  Currently, I am
performing an empty page deletion for all the cases.

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

On Fri, Oct 18, 2019 at 10:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
>

Pushed.

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

On Mon, Oct 21, 2019 at 11:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 18, 2019 at 10:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 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.
> >
>
> Pushed.
Thanks.

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

Hi!

> 18 окт. 2019 г., в 13:21, Dilip Kumar <dilipbalaut@gmail.com> написал(а):
>
> On Fri, Oct 18, 2019 at 10:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>
>> I think we can do it in general as adding some check for parallel
>> vacuum there will look bit hackish.
> I agree with that point.
> 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.
>
> I have prepared a first version of the patch.  Currently, I am
> performing an empty page deletion for all the cases.

I've took a look into the patch, and cannot understand one simple thing...
We should not call gistvacuum_delete_empty_pages() for same gist_stats twice.
Another way once the function is called we should somehow update or zero empty_leaf_set.
Does this invariant hold in your patch?

Best regards, Andrey Borodin.

On Mon, Oct 21, 2019 at 2:30 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> Hi!
>
> > 18 окт. 2019 г., в 13:21, Dilip Kumar <dilipbalaut@gmail.com> написал(а):
> >
> > On Fri, Oct 18, 2019 at 10:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >>
> >> I think we can do it in general as adding some check for parallel
> >> vacuum there will look bit hackish.
> > I agree with that point.
> > 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.
> >
> > I have prepared a first version of the patch.  Currently, I am
> > performing an empty page deletion for all the cases.
>
> I've took a look into the patch, and cannot understand one simple thing...
> We should not call gistvacuum_delete_empty_pages() for same gist_stats twice.
> Another way once the function is called we should somehow update or zero empty_leaf_set.
> Does this invariant hold in your patch?
>
Thanks for looking into the patch.   With this patch now
GistBulkDeleteResult is local to single gistbulkdelete call or
gistvacuumcleanup.  So now we are not sharing GistBulkDeleteResult,
across the calls so I am not sure how it will be called twice for the
same gist_stats?  I might be missing something here?

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

> 21 окт. 2019 г., в 11:12, Dilip Kumar <dilipbalaut@gmail.com> написал(а):
>
> On Mon, Oct 21, 2019 at 2:30 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>>
>> I've took a look into the patch, and cannot understand one simple thing...
>> We should not call gistvacuum_delete_empty_pages() for same gist_stats twice.
>> Another way once the function is called we should somehow update or zero empty_leaf_set.
>> Does this invariant hold in your patch?
>>
> Thanks for looking into the patch.   With this patch now
> GistBulkDeleteResult is local to single gistbulkdelete call or
> gistvacuumcleanup.  So now we are not sharing GistBulkDeleteResult,
> across the calls so I am not sure how it will be called twice for the
> same gist_stats?  I might be missing something here?

Yes, you are right, sorry for the noise.
Currently we are doing both gistvacuumscan() and gistvacuum_delete_empty_pages() in both gistbulkdelete() and
gistvacuumcleanup().Is it supposed to be so? Functions gistbulkdelete() and gistvacuumcleanup() look very similar and
sharesome comments. This is what triggered my attention. 

Thanks!

--
Andrey Borodin
Open source RDBMS development team leader
Yandex.Cloud

On Mon, Oct 21, 2019 at 2:58 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
>
>
> > 21 окт. 2019 г., в 11:12, Dilip Kumar <dilipbalaut@gmail.com> написал(а):
> >
> > On Mon, Oct 21, 2019 at 2:30 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> >>
> >> I've took a look into the patch, and cannot understand one simple thing...
> >> We should not call gistvacuum_delete_empty_pages() for same gist_stats twice.
> >> Another way once the function is called we should somehow update or zero empty_leaf_set.
> >> Does this invariant hold in your patch?
> >>
> > Thanks for looking into the patch.   With this patch now
> > GistBulkDeleteResult is local to single gistbulkdelete call or
> > gistvacuumcleanup.  So now we are not sharing GistBulkDeleteResult,
> > across the calls so I am not sure how it will be called twice for the
> > same gist_stats?  I might be missing something here?
>
> Yes, you are right, sorry for the noise.
> Currently we are doing both gistvacuumscan() and gistvacuum_delete_empty_pages() in both gistbulkdelete() and
gistvacuumcleanup().Is it supposed to be so? 

There was an issue discussed in parallel vacuum thread[1], and for
solving that it has been discussed in this thread[2] that we can
delete empty pages in bulkdelete phase itself.  But, that does not
mean that we can remove that from the gistvacuumcleanup phase.
Because if the gistbulkdelete is not at all called in the vacuum pass
then gistvacuumcleanup, will perform both gistvacuumscan and
gistvacuum_delete_empty_pages.  In short, In whichever pass, we detect
the empty page in the same pass we delete the empty page.

Functions gistbulkdelete() and gistvacuumcleanup() look very similar
and share some comments. This is what triggered my attention.

[1] - https://www.postgresql.org/message-id/CAA4eK1JEQ2y3uNucNopDjK8pse6xSe5%3D_oknoWfRQvAF%3DVqsBA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/69EF7B88-F3E7-4E09-824D-694CF39E5683%40iki.fi

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

On Fri, Oct 18, 2019 at 4:51 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> I have prepared a first version of the patch.  Currently, I am
> performing an empty page deletion for all the cases.
>

Few comments:
----------------------
1.
-/*
- * State kept across vacuum stages.
- */
 typedef struct
 {
- IndexBulkDeleteResult stats; /* must be first */
+ IndexBulkDeleteResult *stats; /* kept across vacuum stages. */

  /*
- * These are used to memorize all internal and empty leaf pages in the 1st
- * vacuum stage.  They are used in the 2nd stage, to delete all the empty
- * pages.
+ * These are used to memorize all internal and empty leaf pages. They are
+ * used for deleting all the empty pages.
  */
  IntegerSet *internal_page_set;
  IntegerSet *empty_leaf_set;

Now, if we don't want to share the remaining stats across
gistbulkdelete and gistvacuumcleanup, isn't it better to keep the
information of internal and empty leaf pages as part of GistVacState?
Also, I think it is better to call gistvacuum_delete_empty_pages from
function gistvacuumscan as that will avoid it calling from multiple
places.

2.
- gist_stats->page_set_context = NULL;
- gist_stats->internal_page_set = NULL;
- gist_stats->empty_leaf_set = NULL;

Why have you removed this initialization?

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

On Tue, Oct 22, 2019 at 9:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 18, 2019 at 4:51 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > I have prepared a first version of the patch.  Currently, I am
> > performing an empty page deletion for all the cases.
> >
>
> Few comments:
> ----------------------
> 1.
> -/*
> - * State kept across vacuum stages.
> - */
>  typedef struct
>  {
> - IndexBulkDeleteResult stats; /* must be first */
> + IndexBulkDeleteResult *stats; /* kept across vacuum stages. */
>
>   /*
> - * These are used to memorize all internal and empty leaf pages in the 1st
> - * vacuum stage.  They are used in the 2nd stage, to delete all the empty
> - * pages.
> + * These are used to memorize all internal and empty leaf pages. They are
> + * used for deleting all the empty pages.
>   */
>   IntegerSet *internal_page_set;
>   IntegerSet *empty_leaf_set;
>
> Now, if we don't want to share the remaining stats across
> gistbulkdelete and gistvacuumcleanup, isn't it better to keep the
> information of internal and empty leaf pages as part of GistVacState?

Basically, only IndexBulkDeleteResult is now shared across the stage
so we can move all members to GistVacState and completely get rid of
GistBulkDeleteResult?

IndexBulkDeleteResult *stats
IntegerSet *internal_page_set;
IntegerSet *empty_leaf_set;
MemoryContext page_set_context;


> Also, I think it is better to call gistvacuum_delete_empty_pages from
> function gistvacuumscan as that will avoid it calling from multiple
> places.
Yeah we can do that.
>
> 2.
> - gist_stats->page_set_context = NULL;
> - gist_stats->internal_page_set = NULL;
> - gist_stats->empty_leaf_set = NULL;
>
> Why have you removed this initialization?
This was post-cleanup reset since we were returning the gist_stats so
it was better to clean up but now we are not returning it out so I
though we don't need to clean this.  But, I think now we can free the
memory gist_stats itself.

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

On Tue, Oct 22, 2019 at 10:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Oct 22, 2019 at 9:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Oct 18, 2019 at 4:51 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > I have prepared a first version of the patch.  Currently, I am
> > > performing an empty page deletion for all the cases.
> > >
> >
> > Few comments:
> > ----------------------
> > 1.
> > -/*
> > - * State kept across vacuum stages.
> > - */
> >  typedef struct
> >  {
> > - IndexBulkDeleteResult stats; /* must be first */
> > + IndexBulkDeleteResult *stats; /* kept across vacuum stages. */
> >
> >   /*
> > - * These are used to memorize all internal and empty leaf pages in the 1st
> > - * vacuum stage.  They are used in the 2nd stage, to delete all the empty
> > - * pages.
> > + * These are used to memorize all internal and empty leaf pages. They are
> > + * used for deleting all the empty pages.
> >   */
> >   IntegerSet *internal_page_set;
> >   IntegerSet *empty_leaf_set;
> >
> > Now, if we don't want to share the remaining stats across
> > gistbulkdelete and gistvacuumcleanup, isn't it better to keep the
> > information of internal and empty leaf pages as part of GistVacState?
>
> Basically, only IndexBulkDeleteResult is now shared across the stage
> so we can move all members to GistVacState and completely get rid of
> GistBulkDeleteResult?
>

Yes, something like that would be better.  Let's try and see how it comes out.

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

On Tue, Oct 22, 2019 at 10:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 22, 2019 at 10:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, Oct 22, 2019 at 9:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Oct 18, 2019 at 4:51 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > I have prepared a first version of the patch.  Currently, I am
> > > > performing an empty page deletion for all the cases.
> > > >
> > >
> > > Few comments:
> > > ----------------------
> > > 1.
> > > -/*
> > > - * State kept across vacuum stages.
> > > - */
> > >  typedef struct
> > >  {
> > > - IndexBulkDeleteResult stats; /* must be first */
> > > + IndexBulkDeleteResult *stats; /* kept across vacuum stages. */
> > >
> > >   /*
> > > - * These are used to memorize all internal and empty leaf pages in the 1st
> > > - * vacuum stage.  They are used in the 2nd stage, to delete all the empty
> > > - * pages.
> > > + * These are used to memorize all internal and empty leaf pages. They are
> > > + * used for deleting all the empty pages.
> > >   */
> > >   IntegerSet *internal_page_set;
> > >   IntegerSet *empty_leaf_set;
> > >
> > > Now, if we don't want to share the remaining stats across
> > > gistbulkdelete and gistvacuumcleanup, isn't it better to keep the
> > > information of internal and empty leaf pages as part of GistVacState?
> >
> > Basically, only IndexBulkDeleteResult is now shared across the stage
> > so we can move all members to GistVacState and completely get rid of
> > GistBulkDeleteResult?
> >
>
> Yes, something like that would be better.  Let's try and see how it comes out.
I have modified as we discussed.  Please take a look.

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

On Tue, Oct 22, 2019 at 2:17 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Oct 22, 2019 at 10:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > > Basically, only IndexBulkDeleteResult is now shared across the stage
> > > so we can move all members to GistVacState and completely get rid of
> > > GistBulkDeleteResult?
> > >
> >
> > Yes, something like that would be better.  Let's try and see how it comes out.
> I have modified as we discussed.  Please take a look.
>

Thanks, I haven't reviewed this yet, but it seems to be on the right
lines.  Sawada-San, can you please prepare the next version of the
parallel vacuum patch on top of this patch and enable parallel vacuum
for Gist indexes?  We can do the review of this patch in detail once
the parallel vacuum patch is in better shape as without that it
wouldn't make sense to commit this patch.

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

On Wed, Oct 23, 2019 at 8:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 22, 2019 at 2:17 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, Oct 22, 2019 at 10:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > > Basically, only IndexBulkDeleteResult is now shared across the stage
> > > > so we can move all members to GistVacState and completely get rid of
> > > > GistBulkDeleteResult?
> > > >
> > >
> > > Yes, something like that would be better.  Let's try and see how it comes out.
> > I have modified as we discussed.  Please take a look.
> >
>
> Thanks, I haven't reviewed this yet, but it seems to be on the right
> lines.  Sawada-San, can you please prepare the next version of the
> parallel vacuum patch on top of this patch and enable parallel vacuum
> for Gist indexes?

Yeah I've sent the latest patch set that is built on top of this
patch[1]. BTW I looked at this patch briefly but it looks good to me.

[1] https://www.postgresql.org/message-id/CAD21AoBMo9dr_QmhT%3DdKh7fmiq7tpx%2ByLHR8nw9i5NZ-SgtaVg%40mail.gmail.com

Regards,

--
Masahiko Sawada

On Fri, Oct 25, 2019 at 9:22 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Oct 23, 2019 at 8:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Oct 22, 2019 at 2:17 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Tue, Oct 22, 2019 at 10:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > I have modified as we discussed.  Please take a look.
> > >
> >
> > Thanks, I haven't reviewed this yet, but it seems to be on the right
> > lines.  Sawada-San, can you please prepare the next version of the
> > parallel vacuum patch on top of this patch and enable parallel vacuum
> > for Gist indexes?
>
> Yeah I've sent the latest patch set that is built on top of this
> patch[1]. BTW I looked at this patch briefly but it looks good to me.
>

Today, I have looked at this patch and found a few things that need to
be changed:

1.
 static void gistvacuum_delete_empty_pages(IndexVacuumInfo *info,
-   GistBulkDeleteResult *stats);
-static bool gistdeletepage(IndexVacuumInfo *info, GistBulkDeleteResult *stats,
+   GistVacState *stats);

I think stats is not a good name for GistVacState.  How about vstate?

2.
+ /* we don't need the internal and empty page sets anymore */
+ MemoryContextDelete(vstate.page_set_context);

After memory context delete, we can reset this and other related
variables as we were doing without the patch.

3.  There are a couple of places in code (like comments, README) that
mentions the deletion of empty pages in the second stage of the
vacuum.  We should change all such places.

I have modified the patch for the above points and additionally ran
pgindent.  Let me know what you think about the attached patch?

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

On Mon, Dec 9, 2019 at 2:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> I have modified the patch for the above points and additionally ran
> pgindent.  Let me know what you think about the attached patch?
>

A new version with a slightly modified commit message.

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

On Mon, Dec 9, 2019 at 2:37 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 9, 2019 at 2:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > I have modified the patch for the above points and additionally ran
> > pgindent.  Let me know what you think about the attached patch?
> >
>
> A new version with a slightly modified commit message.

Your changes look fine to me.  Thanks!

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

On Thu, Jan 9, 2020 at 4:41 PM Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Mon, 9 Dec 2019 at 14:37, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Dec 9, 2019 at 2:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > I have modified the patch for the above points and additionally ran
> > > pgindent.  Let me know what you think about the attached patch?
> > >
> >
> > A new version with a slightly modified commit message.
>
> I reviewed v4 patch and below is the one review comment:
>
> +     * These are used to memorize all internal and empty leaf pages. They are
> +     * used for deleting all the empty pages.
>       */
> After dot, there should be 2 spaces. Earlier, there was 2 spaces.
>
> Other than that patch looks fine to me.
>
Thanks for the comment. Amit has already taken care of this before pushing it.

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