public inbox for [email protected]help / color / mirror / Atom feed
Fix gistkillitems & add regression test to microvacuum 5+ messages / 1 participants [nested] [flat]
* Fix gistkillitems & add regression test to microvacuum @ 2026-01-15 07:00 Kirill Reshke <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Kirill Reshke @ 2026-01-15 07:00 UTC (permalink / raw) To: pgsql-hackers Hi hackers. While looking at [0] I noticed that XLOG_GIST_DELETE & XLOG_GIST_PAGE_DELETE records are not covered. This thread addresses XLOG_GIST_DELETE, which is also known as a microvacuum feature. test.sql contains regression test that trigger this code to be exercised in stream_regress.pl TAP test. Test is as follows: we create a gist index on the table, then we insert exactly 407 records, making the root page full (next insert will trigger page split). Then I delete all tuples from relation and trigger Index Only scan to do kill-on-select (killtuples). It marks gist 0 page (which is root and is leaf) as has_garbage. Then, the next insertion triggers xlog_gist_delete record. To verify this I use pageinspect and pg_waldimp (locally). Also this test is dependent on block size being 8192 which is not good. And all of this does not work actually without v1-0001, because there is a bug in GiST which does not call gistkillitmes for the very first (root) page. There is also test2.sql which inserts a single tuple, not 407. It can be used to verify v1-0001. [0] coverage.postgresql.org/src/backend/access/gist/gistxlog.c.gcov.html -- Best regards, Kirill Reshke Attachments: [application/octet-stream] test2.sql (501B, 2-test2.sql) download [application/octet-stream] test.sql (529B, 3-test.sql) download [application/octet-stream] v1-0001-Fix-gistkillitems-for-GiST-ROOT-page.patch (1.6K, 4-v1-0001-Fix-gistkillitems-for-GiST-ROOT-page.patch) download | inline diff: From 84b0fb6e1ce58c7b917fbcc32e73cadcd265d535 Mon Sep 17 00:00:00 2001 From: reshke <[email protected]> Date: Thu, 15 Jan 2026 06:34:42 +0000 Subject: [PATCH v1] Fix gistkillitems for GiST ROOT page. --- src/backend/access/gist/gist.c | 1 + src/backend/access/gist/gistget.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c index 9c219cadf07..2f341df9058 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -1681,6 +1681,7 @@ gistprunepage(Relation rel, Page page, Buffer buffer, Relation heapRel) maxoff; Assert(GistPageIsLeaf(page)); + Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE)); /* * Scan over all items to see which ones need to be deleted according to diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c index 11b214eb99b..7dc34680e2d 100644 --- a/src/backend/access/gist/gistget.c +++ b/src/backend/access/gist/gistget.c @@ -407,6 +407,9 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, */ so->curPageLSN = BufferGetLSNAtomic(buffer); + /* save current item BlockNumber for gistkillitems() call */ + so->curBlkno = pageItem->blkno; + /* * check all tuples on page */ @@ -722,9 +725,6 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir) CHECK_FOR_INTERRUPTS(); - /* save current item BlockNumber for next gistkillitems() call */ - so->curBlkno = item->blkno; - /* * While scanning a leaf page, ItemPointers of matching heap * tuples are stored in so->pageData. If there are any on -- 2.43.0 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum @ 2026-01-15 07:46 Kirill Reshke <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Kirill Reshke @ 2026-01-15 07:46 UTC (permalink / raw) To: pgsql-hackers On Thu, 15 Jan 2026 at 12:00, Kirill Reshke <[email protected]> wrote: > > Hi hackers. > > While looking at [0] I noticed that XLOG_GIST_DELETE & XLOG_GIST_PAGE_DELETE > records are not covered. > > This thread addresses XLOG_GIST_DELETE, which is also known as a > microvacuum feature. > > test.sql contains regression test that trigger this code to be > exercised in stream_regress.pl TAP test. > > Test is as follows: we create a gist index on the table, then we > insert exactly 407 records, making the root page full (next insert > will trigger page split). Then I delete all tuples from relation and > trigger Index Only scan to do kill-on-select (killtuples). It marks > gist 0 page (which is root and is leaf) as has_garbage. Then, the next > insertion triggers xlog_gist_delete record. > > To verify this I use pageinspect and pg_waldimp (locally). Also this > test is dependent on block size being 8192 which is not good. > > > And all of this does not work actually without v1-0001, because there > is a bug in GiST which does not call gistkillitmes for the very first > (root) page. > > There is also test2.sql which inserts a single tuple, not 407. It can > be used to verify v1-0001. > > [0] coverage.postgresql.org/src/backend/access/gist/gistxlog.c.gcov.html > > > -- > Best regards, > Kirill Reshke ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum @ 2026-01-15 08:21 Kirill Reshke <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Kirill Reshke @ 2026-01-15 08:21 UTC (permalink / raw) To: pgsql-hackers On Thu, 15 Jan 2026 at 12:46, Kirill Reshke <[email protected]> wrote: > > On Thu, 15 Jan 2026 at 12:00, Kirill Reshke <[email protected]> wrote: > > > > Hi hackers. > > > > While looking at [0] I noticed that XLOG_GIST_DELETE & XLOG_GIST_PAGE_DELETE > > records are not covered. > > > > This thread addresses XLOG_GIST_DELETE, which is also known as a > > microvacuum feature. > > > > test.sql contains regression test that trigger this code to be > > exercised in stream_regress.pl TAP test. > > > > Test is as follows: we create a gist index on the table, then we > > insert exactly 407 records, making the root page full (next insert > > will trigger page split). Then I delete all tuples from relation and > > trigger Index Only scan to do kill-on-select (killtuples). It marks > > gist 0 page (which is root and is leaf) as has_garbage. Then, the next > > insertion triggers xlog_gist_delete record. > > > > To verify this I use pageinspect and pg_waldimp (locally). Also this > > test is dependent on block size being 8192 which is not good. > > > > > > And all of this does not work actually without v1-0001, because there > > is a bug in GiST which does not call gistkillitmes for the very first > > (root) page. > > > > There is also test2.sql which inserts a single tuple, not 407. It can > > be used to verify v1-0001. > > > > [0] coverage.postgresql.org/src/backend/access/gist/gistxlog.c.gcov.html > > > > > > -- > > Best regards, > > Kirill Reshke > > > From cf feedback it turns out we already have an isolation test for > this, and it does almost exactly the same. > And more, it fails. > Will try to fix > > > -- > Best regards, > Kirill Reshke This looks like gist does not work for small indexes and this is explicitly tested after [0] [0] https://www.postgresql.org/message-id/lxzj26ga6ippdeunz6kuncectr5gfuugmm2ry22qu6hcx6oid6%40lzx3sjsqh... -- Best regards, Kirill Reshke ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum @ 2026-01-15 08:35 Kirill Reshke <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Kirill Reshke @ 2026-01-15 08:35 UTC (permalink / raw) To: pgsql-hackers On Thu, 15 Jan 2026 at 13:21, Kirill Reshke <[email protected]> wrote: > > On Thu, 15 Jan 2026 at 12:46, Kirill Reshke <[email protected]> wrote: > > > > On Thu, 15 Jan 2026 at 12:00, Kirill Reshke <[email protected]> wrote: > > > > > > Hi hackers. > > > > > > While looking at [0] I noticed that XLOG_GIST_DELETE & XLOG_GIST_PAGE_DELETE > > > records are not covered. > > > > > > This thread addresses XLOG_GIST_DELETE, which is also known as a > > > microvacuum feature. > > > > > > test.sql contains regression test that trigger this code to be > > > exercised in stream_regress.pl TAP test. > > > > > > Test is as follows: we create a gist index on the table, then we > > > insert exactly 407 records, making the root page full (next insert > > > will trigger page split). Then I delete all tuples from relation and > > > trigger Index Only scan to do kill-on-select (killtuples). It marks > > > gist 0 page (which is root and is leaf) as has_garbage. Then, the next > > > insertion triggers xlog_gist_delete record. > > > > > > To verify this I use pageinspect and pg_waldimp (locally). Also this > > > test is dependent on block size being 8192 which is not good. > > > > > > > > > And all of this does not work actually without v1-0001, because there > > > is a bug in GiST which does not call gistkillitmes for the very first > > > (root) page. > > > > > > There is also test2.sql which inserts a single tuple, not 407. It can > > > be used to verify v1-0001. > > > > > > [0] coverage.postgresql.org/src/backend/access/gist/gistxlog.c.gcov.html > > > > > > > > > -- > > > Best regards, > > > Kirill Reshke > > > > > > From cf feedback it turns out we already have an isolation test for > > this, and it does almost exactly the same. > > And more, it fails. > > Will try to fix > > > > > > -- > > Best regards, > > Kirill Reshke > > This looks like gist does not work for small indexes and this is > explicitly tested after [0] > [0] https://www.postgresql.org/message-id/lxzj26ga6ippdeunz6kuncectr5gfuugmm2ry22qu6hcx6oid6%40lzx3sjsqh... > > > -- > Best regards, > Kirill Reshke I was right on commit message of 377b7ab """ For gist some related paths were reached, but gist's implementation seems to not work if all the dead tuples are on one page (or something like that). The coverage for other index types was rather incidental. """ It does not work if all the dead tuples are on one page and this page is ROOT. So, should we delete this ... # Test gist, but with fewer rows - shows that killitems doesn't work anymore! permutation create_table fill_10 create_ext_btree_gist create_gist flush disable_seq disable_bitmap ... from isolation spec? -- Best regards, Kirill Reshke ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum @ 2026-01-15 17:59 Kirill Reshke <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Kirill Reshke @ 2026-01-15 17:59 UTC (permalink / raw) To: pgsql-hackers On Thu, 15 Jan 2026 at 13:35, Kirill Reshke <[email protected]> wrote: > > On Thu, 15 Jan 2026 at 13:21, Kirill Reshke <[email protected]> wrote: > > > > On Thu, 15 Jan 2026 at 12:46, Kirill Reshke <[email protected]> wrote: > > > > > > On Thu, 15 Jan 2026 at 12:00, Kirill Reshke <[email protected]> wrote: > > > > > > > > Hi hackers. > > > > > > > > While looking at [0] I noticed that XLOG_GIST_DELETE & XLOG_GIST_PAGE_DELETE > > > > records are not covered. > > > > > > > > This thread addresses XLOG_GIST_DELETE, which is also known as a > > > > microvacuum feature. > > > > > > > > test.sql contains regression test that trigger this code to be > > > > exercised in stream_regress.pl TAP test. > > > > > > > > Test is as follows: we create a gist index on the table, then we > > > > insert exactly 407 records, making the root page full (next insert > > > > will trigger page split). Then I delete all tuples from relation and > > > > trigger Index Only scan to do kill-on-select (killtuples). It marks > > > > gist 0 page (which is root and is leaf) as has_garbage. Then, the next > > > > insertion triggers xlog_gist_delete record. > > > > > > > > To verify this I use pageinspect and pg_waldimp (locally). Also this > > > > test is dependent on block size being 8192 which is not good. > > > > > > > > > > > > And all of this does not work actually without v1-0001, because there > > > > is a bug in GiST which does not call gistkillitmes for the very first > > > > (root) page. > > > > > > > > There is also test2.sql which inserts a single tuple, not 407. It can > > > > be used to verify v1-0001. > > > > > > > > [0] coverage.postgresql.org/src/backend/access/gist/gistxlog.c.gcov.html > > > > > > > > > > > > -- > > > > Best regards, > > > > Kirill Reshke > > > > > > > > > From cf feedback it turns out we already have an isolation test for > > > this, and it does almost exactly the same. > > > And more, it fails. > > > Will try to fix > > > > > > > > > -- > > > Best regards, > > > Kirill Reshke > > > > This looks like gist does not work for small indexes and this is > > explicitly tested after [0] > > [0] https://www.postgresql.org/message-id/lxzj26ga6ippdeunz6kuncectr5gfuugmm2ry22qu6hcx6oid6%40lzx3sjsqh... > > > > > > -- > > Best regards, > > Kirill Reshke > > I was right on commit message of 377b7ab > > """ > For gist some related paths were reached, but gist's implementation > seems to not work if all the dead tuples are on one page (or something > like that). The coverage for other index types was rather incidental. > """ > > It does not work if all the dead tuples are on one page and this page is ROOT. > > So, should we delete this > > ... > # Test gist, but with fewer rows - shows that killitems doesn't work anymore! > permutation > create_table fill_10 create_ext_btree_gist create_gist flush > disable_seq disable_bitmap > > ... > > from isolation spec? > > > -- > Best regards, > Kirill Reshke PFA v2 which leaves the test in-place. Also commit message improvements. -- Best regards, Kirill Reshke Attachments: [application/octet-stream] v2-0001-Fix-gistkillitems-for-GiST-ROOT-page.patch (3.0K, 2-v2-0001-Fix-gistkillitems-for-GiST-ROOT-page.patch) download | inline diff: From c6d89d6bdcda3bd53beb120be77f24838e4b5b3d Mon Sep 17 00:00:00 2001 From: reshke <[email protected]> Date: Thu, 15 Jan 2026 06:34:42 +0000 Subject: [PATCH v2] Fix gistkillitems for GiST ROOT page. GiST index killitems feature misbehaves for single-page (ROOT-only) GiST index. This is caused by GiST scan "current block" variable not beign initialized for very first-to-scan page (which is ROOT page). Fix this by moving variable initialization in read pgae utility function. Also adjust test output that used to test exactly this bug (starting 377b7ab). Discussion: https://postgr.es/m/CALdSSPgZWX_D8%2BFx4YQqRN5eW5iSx_rJdqQhCfdWTvqKXVfJ4w%40mail.gmail.com Discussion: https://postgr.es/m/lxzj26ga6ippdeunz6kuncectr5gfuugmm2ry22qu6hcx6oid6@lzx3sjsqhmt6 --- src/backend/access/gist/gistget.c | 6 +++--- src/test/modules/index/expected/killtuples.out | 2 +- src/test/modules/index/specs/killtuples.spec | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c index 11b214eb99b..7dc34680e2d 100644 --- a/src/backend/access/gist/gistget.c +++ b/src/backend/access/gist/gistget.c @@ -407,6 +407,9 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, */ so->curPageLSN = BufferGetLSNAtomic(buffer); + /* save current item BlockNumber for gistkillitems() call */ + so->curBlkno = pageItem->blkno; + /* * check all tuples on page */ @@ -722,9 +725,6 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir) CHECK_FOR_INTERRUPTS(); - /* save current item BlockNumber for next gistkillitems() call */ - so->curBlkno = item->blkno; - /* * While scanning a leaf page, ItemPointers of matching heap * tuples are stored in so->pageData. If there are any on diff --git a/src/test/modules/index/expected/killtuples.out b/src/test/modules/index/expected/killtuples.out index be7ddd756ef..f504eca76df 100644 --- a/src/test/modules/index/expected/killtuples.out +++ b/src/test/modules/index/expected/killtuples.out @@ -223,7 +223,7 @@ step flush: SELECT FROM pg_stat_force_next_flush(); step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; new_heap_accesses ----------------- - 1 + 0 (1 row) step drop_table: DROP TABLE IF EXISTS kill_prior_tuple; diff --git a/src/test/modules/index/specs/killtuples.spec b/src/test/modules/index/specs/killtuples.spec index 77fe8c689a7..7f912a40f1f 100644 --- a/src/test/modules/index/specs/killtuples.spec +++ b/src/test/modules/index/specs/killtuples.spec @@ -94,7 +94,7 @@ permutation measure access flush result drop_table drop_ext_btree_gist -# Test gist, but with fewer rows - shows that killitems doesn't work anymore! +# Test gist, but with fewer rows - that killitems used to be buggy. permutation create_table fill_10 create_ext_btree_gist create_gist flush disable_seq disable_bitmap -- 2.43.0 ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-01-15 17:59 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-01-15 07:00 Fix gistkillitems & add regression test to microvacuum Kirill Reshke <[email protected]> 2026-01-15 07:46 ` Kirill Reshke <[email protected]> 2026-01-15 08:21 ` Kirill Reshke <[email protected]> 2026-01-15 08:35 ` Kirill Reshke <[email protected]> 2026-01-15 17:59 ` Kirill Reshke <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox