Thread: New gist vacuum.
<p>Hello. I am student from gsoc programm.<br />My project is sequantial access in vacuum of gist.<br /><br />New vacuumhas 2 big step:<br />physical order scan pages and cleaning after 1 step.<br /><br /><br />1 scan - scan all pagesand create information map(hashmap) and add information to rescan stack( stack of pages that needed to rescanning<br/><br />second step is work only with page(from rescan stack) where there is a changes. In new version of vacuumbesides increased speed also there is a deleting of pages. Only leaf pages can be deleted. The process of deleteingpages is (1. delete link to page. 2. change rightlinks (if needed) 3. set deleted). I added 2 action in wal (wheni set delete flag and when i change rightlinks). When i delete links to leaf pages from inner page i always save 1 linkto leaf(avoiding situations with empty inner pages).<p>I attach some speed benchmarks.<p>i compare old and new versionon my laptop(without ssd). the test: table "point_tbl" from regression database. i insert about 200 millions rows.after that i delete 33 million and run vacuum.<p>size of index is about 18 gb.<p>old version:<p>INFO: vacuuming "public.point_tbl"<br/>INFO: scanned index "gpointind" to remove 11184520 row versions<br />DETAIL: CPU 84.70s/72.26u secelapsed 27007.14 sec.<br />INFO: "point_tbl": removed 11184520 row versions in 400715 pages<br />DETAIL: CPU 3.96s/3.10usec elapsed 233.12 sec.<br />INFO: scanned index "gpointind" to remove 11184523 row versions<br />DETAIL: CPU87.10s/69.05u sec elapsed 26410.44 sec.<br />INFO: "point_tbl": removed 11184523 row versions in 400715 pages<br />DETAIL:CPU 4.23s/3.36u sec elapsed 331.43 sec.<br />INFO: scanned index "gpointind" to remove 11184523 row versions<br/>DETAIL: CPU 87.65s/65.73u sec elapsed 26230.35 sec.<br />INFO: "point_tbl": removed 11184523 row versions in400715 pages<br />DETAIL: CPU 4.47s/3.41u sec elapsed 342.93 sec.<br />INFO: scanned index "gpointind" to remove 866 rowversions<br />DETAIL: CPU 79.97s/39.64u sec elapsed 23341.88 sec.<br />INFO: "point_tbl": removed 866 row versions in31 pages<br />DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.<br />INFO: index "gpointind" now contains 201326592 row versionsin 2336441 pages<br />DETAIL: 33554432 index row versions were removed.<br />0 index pages have been deleted, 0 arecurrently reusable.<p> <p> <p>new vacuum is about<p> <p>INFO: vacuuming "public.point_tbl"<br />INFO: scanned index "gpointind"to remove 11184520 row versions<br />DETAIL: CPU 13.00s/27.57u sec elapsed 1864.22 sec.<br />INFO: "point_tbl":removed 11184520 row versions in 400715 pages<br />DETAIL: CPU 3.46s/2.86u sec elapsed 214.04 sec.<br />INFO:scanned index "gpointind" to remove 11184523 row versions<br />DETAIL: CPU 14.17s/27.02u sec elapsed 2163.67 sec.<br/>INFO: "point_tbl": removed 11184523 row versions in 400715 pages<br />DETAIL: CPU 3.33s/2.99u sec elapsed 222.60sec.<br />INFO: scanned index "gpointind" to remove 11184523 row versions<br />DETAIL: CPU 11.84s/25.23u sec elapsed1828.71 sec.<br />INFO: "point_tbl": removed 11184523 row versions in 400715 pages<br />DETAIL: CPU 3.44s/2.81u secelapsed 215.06 sec.<br />INFO: scanned index "gpointind" to remove 866 row versions<br />DETAIL: CPU 5.62s/6.68u sec elapsed176.67 sec.<br />INFO: "point_tbl": removed 866 row versions in 31 pages<br />DETAIL: CPU 0.00s/0.00u sec elapsed0.01 sec.<br />INFO: index "gpointind" now contains 201326592 row versions in 2336360 pages<br />DETAIL: 33554432index row versions were removed.<br />150833 index pages have been deleted, 150833 are currently reusable.<br />CPU5.54s/2.08u sec elapsed 165.61 sec.<br />INFO: "point_tbl": found 33554432 removable, 201326592 nonremovable row versionsin 1202176 out of 1202176 pages<br />DETAIL: 0 dead row versions cannot be removed yet.<br />There were 0 unuseditem pointers.<br />Skipped 0 pages due to buffer pins.<br />0 pages are entirely empty.<br />CPU 73.50s/116.82u secelapsed 8300.73 sec.<br />INFO: analyzing "public.point_tbl"<br />INFO: "point_tbl": scanned 100 of 1202176 pages, containing16756 live rows and 0 dead rows; 100 rows in sample, 201326601 estimated total rows<br />VACUUM<p> <p>There isa big speed up + we can reuse some pages.<p>Thanks.
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
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
<div>Thank you, Jeff.</div><div>I reworking patch now. All // warning will be deleted.</div><div>About memory consumptionnew version will control size of stack and will operate with map of little size because i want delete old stylevacuum(now if maintenance_work_mem less than needed to build info map we use old-style vacuum with logical order).</div>
Костя Кузнецов wrote: > <div>Thank you, Jeff.</div><div>I reworking patch now. All // warning will be deleted.</div><div>About memory consumptionnew version will control size of stack and will operate with map of little size because i want delete old stylevacuum(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
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 consumptionnew version will control size of stack and will operate with map of little size because i want delete old stylevacuum(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 filltable for every page. If author and community do not object, I want to continue work on Konstantin's patch. Best regards, Andrey Borodin. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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 hadthe 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.
Attachment
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.
Attachment
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 shouldnot. If gistvacuumcleanup() is not given a statistics gathered by gistbulkdelete() it returns incorrect tuples count for partialindex. 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 solvesa bit different problem. Best regards, Andrey Borodin.
Attachment
> 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
Attachment
Hi!
On Thu, Dec 28, 2017 at 2:37 PM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
I'm very glad this patch isn't forgotten. I've assigned to review this patch.
> 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.
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
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Hi, Alexander!
Many thanks for looking into patches!
A little bit later I will provide answer in other branch of discussion.
Ok, done. https://commitfest.postgresql.org/17/148315 янв. 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.
Great, thanks!I've made small improvements to this patch: variable naming, formatting, comments.
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.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;
Best regards, Andrey Borodin.
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 complexconcurrency 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.
Attachment
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 timewith questions. Best regards, Andrey Borodin.
Attachment
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 timewith 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
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 anytime 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.
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 anytime 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
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 numberof 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
> I'll attach patch file next message.
Updated patch is attached.
Attachment
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
numberof 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
> 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 numberof 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 mainvacuum 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.
Attachment
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
> 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 convenientway. I've learned no to do it this way anymore :) Best regards, Andrey Borodin.