Thread

New gist vacuum.

Костя Кузнецов <chapaev28@ya.ru> — 2015-09-10T22:52:30Z

Re: New gist vacuum.

Michael Paquier <michael.paquier@gmail.com> — 2015-09-13T13:17:28Z

On Fri, Sep 11, 2015 at 7:52 AM, Костя Кузнецов <chapaev28@ya.ru> wrote:
> old version:
>
> INFO: vacuuming "public.point_tbl"
> INFO: scanned index "gpointind" to remove 11184520 row versions
> DETAIL: CPU 84.70s/72.26u sec elapsed 27007.14 sec.
> [...]
>
> new vacuum is about
> INFO: vacuuming "public.point_tbl"
> INFO: scanned index "gpointind" to remove 11184520 row versions
> DETAIL: CPU 13.00s/27.57u sec elapsed 1864.22 sec.
> [...]
> There is a big speed up + we can reuse some pages.

Indeed. Interesting. You should definitely add your patch to the next
commit fest:
https://commitfest.postgresql.org/7/
-- 
Michael

Re: New gist vacuum.

Jeff Janes <jeff.janes@gmail.com> — 2015-10-31T06:55:32Z

On Thu, Sep 10, 2015 at 3:52 PM, Костя Кузнецов <chapaev28@ya.ru> wrote:
> Hello. I am student from gsoc programm.
> My project is sequantial access in vacuum of gist.
>
> New vacuum has 2 big step:
> physical order scan pages and cleaning after 1 step.
>
>
> 1 scan - scan all pages and create information map(hashmap) and add
> information to rescan stack( stack of pages that needed to rescanning

This is interesting work.  I think the patch needs a rebase to the git
HEAD.  There is a minor conflict in gistRedoPageUpdateRecord.  It is a
little confusing  because your patch introduces new code and then
immediately comments it out (using //, which is not a comment style
allowed in our style guide) and that phantom code confuses the
conflict resolution process.

There are several other places throughout the patch that use //
comment style to comment out code which the patch itself added.  Those
should be removed, and the real comments should be converted to /*
this */ style.

I also got a compiler warning, it looks like a missing #include:

gistutil.c: In function 'gistNewBuffer':
gistutil.c:788:4: warning: implicit declaration of function
'TransactionIdPrecedes' [-Wimplicit-function-declaration]
    if (GistPageIsDeleted(page) &&
TransactionIdPrecedes(p->pd_prune_xid, RecentGlobalDataXmin)) {
    ^


Also, I didn't see a check on the size of the stack.  Is there an
argument that this stack will not be able to grow to be large enough
to cause trouble?

Thanks,

Jeff

Re: New gist vacuum.

Костя Кузнецов <chapaev28@ya.ru> — 2015-11-27T17:00:36Z

<div>Thank you, Jeff.</div><div>I reworking patch now. All // warning will be deleted.</div><div>About memory consumption new version will control size of stack and will operate with map of little size because i want delete old style vacuum(now if maintenance_work_mem less than needed to build info map we use old-style vacuum with logical order).</div>

Re: New gist vacuum.

Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-01-31T12:18:36Z

Костя Кузнецов wrote:
> <div>Thank you, Jeff.</div><div>I reworking patch now. All // warning will be deleted.</div><div>About memory consumption new version will control size of stack and will operate with map of little size because i want delete old style vacuum(now if maintenance_work_mem less than needed to build info map we use old-style vacuum with logical order).</div>
> 

You never got around to submitting the updated version of this patch,
and it's been a long time now, so I'm marking it as returned with
feedback for now.  Please do submit a new version once you have it,
since this seems to be very useful.

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

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2017-11-12T18:20:24Z

Hello!

> 31 янв. 2016 г., в 17:18, Alvaro Herrera <alvherre@2ndquadrant.com> написал(а):
> 
> Костя Кузнецов wrote:
>> <div>Thank you, Jeff.</div><div>I reworking patch now. All // warning will be deleted.</div><div>About memory consumption new version will control size of stack and will operate with map of little size because i want delete old style vacuum(now if maintenance_work_mem less than needed to build info map we use old-style vacuum with logical order).</div>
> 
> You never got around to submitting the updated version of this patch,
> and it's been a long time now, so I'm marking it as returned with
> feedback for now.  Please do submit a new version once you have it,
> since this seems to be very useful.

I've rebased patch (see attachment), it seems to work. It still requires a bit of styling, docs and tests, at least.
Also, I thinks that hash table is not very good option if we have all pages there: we should either use array or do not fill table for every page.

If author and community do not object, I want to continue work on Konstantin's patch.

Best regards, Andrey Borodin.

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2017-12-19T10:58:35Z

Hi hackers!

Here is the patch that deletes pages during GiST VACUUM.

> 12 нояб. 2017 г., в 23:20, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
> 
> If author and community do not object, I want to continue work on Konstantin's patch.


==Purpose==
Long story short, some time ago Konstantin Kuznetsov hacked out a patch that added GiST scan with physical order of scan.
This scan is using a lot of memory to build map of whole GiST graph. If there is not enough maintenance memory, patch had the fallback to old GiST VACUUM.
New behavior was deleting pages while old (still used) was not.

I've rebased patch, fixed some bugs and decided that it solves too much in a single step.

Here is the patch, which adds functionality of GiST page deletes.
If this is committed, porting physical scan code will be much easier.

==What is changed==
When GiST VACUUM scans graph for removed tuples, it remembers internal pages that are referencing completely empty leaf pages.
Then in additional step, these pages are rescanned to delete references and mark leaf pages as free.

==Limitations==
At least one reference on each internal pages is left undeleted to preserve balancing of the tree.
Pages that has FOLLOW-RIGHT flag also are not deleted, even if empty.


Thank you for your attention, any thoughts are welcome.

Best regards, Andrey Borodin.

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2017-12-28T11:37:48Z

Hi hackers!
> 19 дек. 2017 г., в 15:58, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
> Here is the patch that deletes pages during GiST VACUUM.

Here is new version of the patch for GiST VACUUM.
There are two main changes:
1. During rescan for page deletion only know to be recently empty pages are rescanned.
2. I've re-implemented physical scan with array instead of hash table.

Thanks! 
Merry Christmas and happy New Year.

Best regards, Andrey Borodin.

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2017-12-30T09:18:02Z

Hi!

> 28 дек. 2017 г., в 16:37, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
> Here is new version of the patch for GiST VACUUM.
> There are two main changes:
> 1. During rescan for page deletion only know to be recently empty pages are rescanned.
> 2. I've re-implemented physical scan with array instead of hash table.

There is one more minor spot in GiST VACUUM. It takes heap tuples count for statistics for partial indexes, while it should not. 

If gistvacuumcleanup() is not given a statistics gathered by gistbulkdelete() it returns incorrect tuples count for partial index.
Here's the micropatch, which fixes that corner case.
To reproduce this effect I used this query:
create table y as select cube(random()) c from generate_series(1,10000) y; create index on y using gist(c) where c~>1 > 0.5;
vacuum verbose y;
Before patch it will report 10000 tuples, with patch it will report different values around 5000.

I do not know, should I register separate commitfest entry? The code is very close to main GiST VACUUM patch, but solves a bit different problem.

Best regards, Andrey Borodin.

Re: New gist vacuum.

Alexander Korotkov <a.korotkov@postgrespro.ru> — 2018-01-15T18:34:35Z

Hi!

On Sat, Dec 30, 2017 at 12:18 PM, Andrey Borodin <x4mmm@yandex-team.ru>
wrote:

> > 28 дек. 2017 г., в 16:37, Andrey Borodin <x4mmm@yandex-team.ru>
> написал(а):
> > Here is new version of the patch for GiST VACUUM.
> > There are two main changes:
> > 1. During rescan for page deletion only know to be recently empty pages
> are rescanned.
> > 2. I've re-implemented physical scan with array instead of hash table.
>
> There is one more minor spot in GiST VACUUM. It takes heap tuples count
> for statistics for partial indexes, while it should not.
>
> If gistvacuumcleanup() is not given a statistics gathered by
> gistbulkdelete() it returns incorrect tuples count for partial index.
> Here's the micropatch, which fixes that corner case.
> To reproduce this effect I used this query:
> create table y as select cube(random()) c from generate_series(1,10000) y;
> create index on y using gist(c) where c~>1 > 0.5;
> vacuum verbose y;
> Before patch it will report 10000 tuples, with patch it will report
> different values around 5000.
>

It's very good that you've fixed that.

I do not know, should I register separate commitfest entry? The code is
> very close to main GiST VACUUM patch, but solves a bit different problem.
>

Yes, I think it deserves separate commitfest entry.  Despite it's related
to GiST VACUUM, it's a separate fix.
I've made small improvements to this patch: variable naming, formatting,
comments.
BTW, do we really need to set shouldCount depending on whether we receive
stats argument or not?  What if we always set shouldCount as in the first
branch of "if"?


> shouldCount = !heap_attisnull(rel->rd_indextuple, Anum_pg_index_indpred) ||
>    info->estimated_count;



------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: New gist vacuum.

Alexander Korotkov <a.korotkov@postgrespro.ru> — 2018-01-15T18:42:26Z

Hi!

On Thu, Dec 28, 2017 at 2:37 PM, Andrey Borodin <x4mmm@yandex-team.ru>
wrote:

> > 19 дек. 2017 г., в 15:58, Andrey Borodin <x4mmm@yandex-team.ru>
> написал(а):
> > Here is the patch that deletes pages during GiST VACUUM.
>
> Here is new version of the patch for GiST VACUUM.
> There are two main changes:
> 1. During rescan for page deletion only know to be recently empty pages
> are rescanned.
> 2. I've re-implemented physical scan with array instead of hash table.
>

I'm very glad this patch isn't forgotten.  I've assigned to review this
patch.
My first note on this patchset is following.  These patches touches
sensitive aspects or GiST and are related to complex concurrency issues.
Thus, I'm sure both of them deserve high-level description in
src/backend/access/gist/README.  Given this description, it would be much
easier to review the patches.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2018-01-16T05:24:31Z

Hi, Alexander!

Many thanks for looking into patches!
A little bit later I will provide answer in other branch of discussion.

> 15 янв. 2018 г., в 23:34, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):
> 
> I do not know, should I register separate commitfest entry? The code is very close to main GiST VACUUM patch, but solves a bit different problem.
> 
> Yes, I think it deserves separate commitfest entry.  Despite it's related to GiST VACUUM, it's a separate fix.
Ok, done. https://commitfest.postgresql.org/17/1483
> I've made small improvements to this patch: variable naming, formatting, comments.
Great, thanks!
> BTW, do we really need to set shouldCount depending on whether we receive stats argument or not?  What if we always set shouldCount as in the first branch of "if"?
>  
> 		shouldCount = !heap_attisnull(rel->rd_indextuple, Anum_pg_index_indpred) ||
> 					   info->estimated_count;
We do not need to count if we have exact count from heap and this index is not partial. ITSM that it is quite common case.

Best regards, Andrey Borodin.

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2018-01-16T16:42:51Z

Hi!
> 15 янв. 2018 г., в 23:42, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):
> 
> I'm very glad this patch isn't forgotten.  I've assigned to review this patch.
Cool, thanks!
> My first note on this patchset is following.  These patches touches sensitive aspects or GiST and are related to complex concurrency issues.
Indeed! That is why I've divided patches: first one implements very simple scan algorithm with concurrency and recovery, second replaces simple algorithm with two more tricky algorithms.
> Thus, I'm sure both of them deserve high-level description in src/backend/access/gist/README.  Given this description, it would be much easier to review the patches.
Yes, that's definitely true. Please find README patch attached.


Best regards, Andrey Borodin.

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2018-01-21T10:34:12Z

Hello, Alexander!
> 16 янв. 2018 г., в 21:42, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
> Please find README patch attached.

Here's v2 version. Same code, but x2 comments. Also fixed important typo in readme BFS->DFS. Feel free to ping me any time with questions.

Best regards, Andrey Borodin.

Re: Re: New gist vacuum.

David Steele <david@pgmasters.net> — 2018-02-07T15:39:49Z

Hi Andrey,

On 1/21/18 5:34 AM, Andrey Borodin wrote:
> Hello, Alexander!
>> 16 янв. 2018 г., в 21:42, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
>> Please find README patch attached.
> 
> Here's v2 version. Same code, but x2 comments. Also fixed important typo in readme BFS->DFS. Feel free to ping me any time with questions.

This patch has not gotten review and does not seem like a good fit for
the last PG11 CF so I am marking it Returned with Feedback.

Regards,
-- 
-David
david@pgmasters.net

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2018-02-07T15:46:35Z

Hi, David!

> 7 февр. 2018 г., в 18:39, David Steele <david@pgmasters.net> написал(а):
> 
> Hi Andrey,
> 
> On 1/21/18 5:34 AM, Andrey Borodin wrote:
>> Hello, Alexander!
>>> 16 янв. 2018 г., в 21:42, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
>>> Please find README patch attached.
>> 
>> Here's v2 version. Same code, but x2 comments. Also fixed important typo in readme BFS->DFS. Feel free to ping me any time with questions.
> 
> This patch has not gotten review and does not seem like a good fit for
> the last PG11 CF so I am marking it Returned with Feedback.

Why do you think this patch does not seem good fit for CF?

I've been talking with Alexander just yesterday at PgConf.Russia, and he was going to provide review.

Best regards, Andrey Borodin.

Re: New gist vacuum.

David Steele <david@pgmasters.net> — 2018-02-08T17:04:44Z

Hi Andrey,

On 2/7/18 10:46 AM, Andrey Borodin wrote:
>> 7 февр. 2018 г., в 18:39, David Steele <david@pgmasters.net> написал(а):
>>
>> Hi Andrey,
>>
>> On 1/21/18 5:34 AM, Andrey Borodin wrote:
>>> Hello, Alexander!
>>>> 16 янв. 2018 г., в 21:42, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
>>>> Please find README patch attached.
>>>
>>> Here's v2 version. Same code, but x2 comments. Also fixed important typo in readme BFS->DFS. Feel free to ping me any time with questions.
>>
>> This patch has not gotten review and does not seem like a good fit for
>> the last PG11 CF so I am marking it Returned with Feedback.
> 
> Why do you think this patch does not seem good fit for CF?

Apologies for the brevity.  I had about 40 patches to go through yesterday.

The reason it does not seem a good fit is that it's a new, possibly
invasive patch that has not gotten any review in the last three CFs
since it was reintroduced.  I'm not sure why that's the case and I have
no opinion about the patch itself, but there it is.

We try to avoid new patches in the last CF that could be destabilizing
and this patch appears to be in that category.  I know it has been
around for a while, but the lack of review makes it "new" in the context
of the last CF for PG11.

> I've been talking with Alexander just yesterday at PgConf.Russia, and he was going to provide review.

Great!  I'd suggest you submit this patch for the CF after 2018-03.

However, that's completely up to you.

Regards,
-- 
-David
david@pgmasters.net

Re: New gist vacuum.

Michail Nikolaev <michail.nikolaev@gmail.com> — 2018-02-25T13:49:02Z

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

Hello.

I have added small change to patch to allow it be compiled using msvc (uint64_t -> uint64).
Everything seems to work, check-world is passing.

Actually patch fixes two issues:
1) Partial GIST indexes now have corrent tuples count estimation.
2) Now subsequent calls to VACUUM on GIST index (like "VACCUM table_name") do not change tuples count to estimated number of tuples in table (which is changed even without any updates in table due current implementation).

I think it is fine to commit.

Patch is also availble on github:
https://github.com/michail-nikolaev/postgres/commit/ff5171b586e4eb60ea5d15a18055d8ea4e44d244?ts=4

I'll attach patch file next message.

The new status of this patch is: Ready for Committer

Re: New gist vacuum.

Michail Nikolaev <michail.nikolaev@gmail.com> — 2018-02-25T13:54:18Z

> I'll attach patch file next message.

Updated patch is attached.

Re: New gist vacuum.

Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-01T17:44:20Z

Michail Nikolaev <michail.nikolaev@gmail.com> writes:
> I have added small change to patch to allow it be compiled using msvc (uint64_t -> uint64).
> Everything seems to work, check-world is passing.

> Actually patch fixes two issues:
> 1) Partial GIST indexes now have corrent tuples count estimation.
> 2) Now subsequent calls to VACUUM on GIST index (like "VACCUM table_name") do not change tuples count to estimated number of tuples in table (which is changed even without any updates in table due current implementation).

> I think it is fine to commit.

I took a quick look at this.  I wonder what is the point of making
the counting conditional.  Since the function is visiting every
index page anyway, why not just always count and unconditionally
provide an exact answer?  The number of cycles saved by skipping
"tuplesCount += PageGetMaxOffsetNumber(page)" on each leaf page
is surely trivial.

			regards, tom lane

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2018-03-02T05:33:40Z

> 1 марта 2018 г., в 22:44, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
> 
> Michail Nikolaev <michail.nikolaev@gmail.com> writes:
>> I have added small change to patch to allow it be compiled using msvc (uint64_t -> uint64).
>> Everything seems to work, check-world is passing.
> 
>> Actually patch fixes two issues:
>> 1) Partial GIST indexes now have corrent tuples count estimation.
>> 2) Now subsequent calls to VACUUM on GIST index (like "VACCUM table_name") do not change tuples count to estimated number of tuples in table (which is changed even without any updates in table due current implementation).
> 
>> I think it is fine to commit.
> 
> I took a quick look at this.  I wonder what is the point of making
> the counting conditional.  Since the function is visiting every
> index page anyway, why not just always count and unconditionally
> provide an exact answer?  The number of cycles saved by skipping
> "tuplesCount += PageGetMaxOffsetNumber(page)" on each leaf page
> is surely trivial.

Thanks for looking into the patch, Tom!

I thought that it's a good idea to optimize out as many cycles as possible.
But, indeed, there are some reasons in favor of unconditional counting:
1. Code is cleaner, and this is not hot path
2. If we choose unconditional counting in gistvacuumcleanup() I'll remove those counting cycles in gistbulkdelete() in main vacuum patch (for v12). Both functions will have less code.

So, I agree, unconditional counting is a good idea. Here's the v3 patch.

Best regards, Andrey Borodin.

Re: New gist vacuum.

Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-02T16:25:35Z

Andrey Borodin <x4mmm@yandex-team.ru> writes:
> So, I agree, unconditional counting is a good idea. Here's the v3 patch.

Pushed with trivial cosmetic adjustments.

I've marked the CF entry as committed; please make a new CF entry in
2018-09 for the other patch.  I'd also suggest starting a new email
thread for that.  Linking the CF entry to a years-old thread doesn't
make it easy for people to find what's the current submission.

			regards, tom lane

Re: New gist vacuum.

x4mmm@yandex-team.ru — 2018-03-02T17:00:41Z

> 2 марта 2018 г., в 21:25, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
> 
> Andrey Borodin <x4mmm@yandex-team.ru> writes:
>> So, I agree, unconditional counting is a good idea. Here's the v3 patch.
> 
> Pushed with trivial cosmetic adjustments.
> 
> I've marked the CF entry as committed; please make a new CF entry in
> 2018-09 for the other patch.  I'd also suggest starting a new email
> thread for that.  Linking the CF entry to a years-old thread doesn't
> make it easy for people to find what's the current submission.

Thanks, Tom!

Yes, I'll definitely start new thread for that patch. This thread had split unexpectedly, and I see it's not a convenient way. I've learned no to do it this way anymore :)

Best regards, Andrey Borodin.