public inbox for [email protected]
help / color / mirror / Atom feedRe: Fix gistkillitems & add regression test to microvacuum
6+ messages / 3 participants
[nested] [flat]
* Re: Fix gistkillitems & add regression test to microvacuum
@ 2026-01-20 10:30 Andrey Borodin <[email protected]>
2026-01-23 11:03 ` Re: Fix gistkillitems & add regression test to microvacuum Kirill Reshke <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andrey Borodin @ 2026-01-20 10:30 UTC (permalink / raw)
To: Kirill Reshke <[email protected]>; +Cc: pgsql-hackers
> On 15 Jan 2026, at 22:59, Kirill Reshke <[email protected]> wrote:
>
> PFA v2 which leaves the test in-place.
>
> Also commit message improvements.
Yeah, killtuples for GiST root page is broken. Your patch is fixing it.
I don't think we should backpatch this, the bug is harmless, but for master the patch LGTM.
It would be good to assign so->curBlkno and so->curBlkno together. But gistScanPage() is the only place with access to the block number.
+# Test gist, but with fewer rows - that killitems used to be buggy.
Probably, in this comment we can explicitly say that killitems was buggy, but now is fixed.
Thanks!
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum
2026-01-20 10:30 Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
@ 2026-01-23 11:03 ` Kirill Reshke <[email protected]>
2026-01-27 07:26 ` Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Reshke @ 2026-01-23 11:03 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: pgsql-hackers
On Tue, 20 Jan 2026 at 15:30, Andrey Borodin <[email protected]> wrote:
>
>
>
> > On 15 Jan 2026, at 22:59, Kirill Reshke <[email protected]> wrote:
> >
> > PFA v2 which leaves the test in-place.
> >
> > Also commit message improvements.
>
> Yeah, killtuples for GiST root page is broken. Your patch is fixing it.
> I don't think we should backpatch this, the bug is harmless, but for master the patch LGTM.
Thank you
> It would be good to assign so->curBlkno and so->curBlkno together. But gistScanPage() is the only place with access to the block number.
Sorry, didnt get this take.
> +# Test gist, but with fewer rows - that killitems used to be buggy.
>
> Probably, in this comment we can explicitly say that killitems was buggy, but now is fixed.
>
Hmm, what would be a good wording here?
--
Best regards,
Kirill Reshke
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum
2026-01-20 10:30 Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
2026-01-23 11:03 ` Re: Fix gistkillitems & add regression test to microvacuum Kirill Reshke <[email protected]>
@ 2026-01-27 07:26 ` Andrey Borodin <[email protected]>
2026-02-11 12:18 ` Re: Fix gistkillitems & add regression test to microvacuum Soumya S Murali <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andrey Borodin @ 2026-01-27 07:26 UTC (permalink / raw)
To: Kirill Reshke <[email protected]>; +Cc: pgsql-hackers
> On 23 Jan 2026, at 16:03, Kirill Reshke <[email protected]> wrote:
>
> On Tue, 20 Jan 2026 at 15:30, Andrey Borodin <[email protected]> wrote:
>>
>>
>>
>>> On 15 Jan 2026, at 22:59, Kirill Reshke <[email protected]> wrote:
>>>
>>> PFA v2 which leaves the test in-place.
>>>
>>> Also commit message improvements.
>>
>> Yeah, killtuples for GiST root page is broken. Your patch is fixing it.
>> I don't think we should backpatch this, the bug is harmless, but for master the patch LGTM.
>
> Thank you
>
>> It would be good to assign so->curBlkno and so->curBlkno together. But gistScanPage() is the only place with access to the block number.
>
> Sorry, didnt get this take.
Sorry, I meant so->curBlkno and so->numKilled are semantically correlated. But it's difficult to assign them together and this does not worth refactoring.
>
>> +# Test gist, but with fewer rows - that killitems used to be buggy.
>>
>> Probably, in this comment we can explicitly say that killitems was buggy, but now is fixed.
>>
>
> Hmm, what would be a good wording here?
I've opened an English dictionary and it says "used to" can be used with a meaning "bug was there but it's not anymore".
So the patch is RfC IMO.
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum
2026-01-20 10:30 Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
2026-01-23 11:03 ` Re: Fix gistkillitems & add regression test to microvacuum Kirill Reshke <[email protected]>
2026-01-27 07:26 ` Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
@ 2026-02-11 12:18 ` Soumya S Murali <[email protected]>
2026-03-18 05:42 ` Re: Fix gistkillitems & add regression test to microvacuum Kirill Reshke <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Soumya S Murali @ 2026-02-11 12:18 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Kirill Reshke <[email protected]>; pgsql-hackers
Hi all,
Thank you for the updated patch.
On Wed, Feb 11, 2026 at 2:52 PM Andrey Borodin <[email protected]> wrote:
>
>
>
> > On 23 Jan 2026, at 16:03, Kirill Reshke <[email protected]> wrote:
> >
> > On Tue, 20 Jan 2026 at 15:30, Andrey Borodin <[email protected]> wrote:
> >>
> >>
> >>
> >>> On 15 Jan 2026, at 22:59, Kirill Reshke <[email protected]> wrote:
> >>>
> >>> PFA v2 which leaves the test in-place.
> >>>
> >>> Also commit message improvements.
> >>
> >> Yeah, killtuples for GiST root page is broken. Your patch is fixing it.
> >> I don't think we should backpatch this, the bug is harmless, but for master the patch LGTM.
> >
> > Thank you
> >
> >> It would be good to assign so->curBlkno and so->curBlkno together. But gistScanPage() is the only place with access to the block number.
> >
> > Sorry, didnt get this take.
>
> Sorry, I meant so->curBlkno and so->numKilled are semantically correlated. But it's difficult to assign them together and this does not worth refactoring.
>
> >
> >> +# Test gist, but with fewer rows - that killitems used to be buggy.
> >>
> >> Probably, in this comment we can explicitly say that killitems was buggy, but now is fixed.
> >>
> >
> > Hmm, what would be a good wording here?
>
> I've opened an English dictionary and it says "used to" can be used with a meaning "bug was there but it's not anymore".
>
> So the patch is RfC IMO.
I reproduced the issue locally by filling a GiST root page, deleting
all tuples, and triggering a microvacuum by an index-only scan.
With the patch, The root page is now handled consistently with other
leaf pages. so->curBlkno and so->curPageLSN are properly set during
scan, and gistkillitems() operates safely and correctly on the root
page.
Based on runtime validation and code inspection, the fix LGTM.
Thanks,
Soumya
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum
2026-01-20 10:30 Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
2026-01-23 11:03 ` Re: Fix gistkillitems & add regression test to microvacuum Kirill Reshke <[email protected]>
2026-01-27 07:26 ` Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
2026-02-11 12:18 ` Re: Fix gistkillitems & add regression test to microvacuum Soumya S Murali <[email protected]>
@ 2026-03-18 05:42 ` Kirill Reshke <[email protected]>
2026-03-20 06:17 ` Re: Fix gistkillitems & add regression test to microvacuum Soumya S Murali <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Reshke @ 2026-03-18 05:42 UTC (permalink / raw)
To: Soumya S Murali <[email protected]>; +Cc: Andrey Borodin <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>
On Wed, 11 Feb 2026 at 17:18, Soumya S Murali
<[email protected]> wrote:
>
> Hi all,
>
> Thank you for the updated patch.
> I reproduced the issue locally by filling a GiST root page, deleting
> all tuples, and triggering a microvacuum by an index-only scan.
> With the patch, The root page is now handled consistently with other
> leaf pages. so->curBlkno and so->curPageLSN are properly set during
> scan, and gistkillitems() operates safely and correctly on the root
> page.
> Based on runtime validation and code inspection, the fix LGTM.
>
Rebased due to f5eb854ab6d[0]
I have added a [0]committer to CC.
Andres, can you please take a look at this thread, if by chance you
have spare time for it? You have changed kill tuples-related logic
frequently, so you might be interested...
[0] https://git.postgresql.org/cgit/postgresql.git/commit/?id=f5eb854ab6d6281ec2d3143657944bdda6676341
--
Best regards,
Kirill Reshke
Attachments:
[application/octet-stream] v3-0001-Fix-gistkillitems-for-GiST-ROOT-page.patch (3.1K, 2-v3-0001-Fix-gistkillitems-for-GiST-ROOT-page.patch)
download | inline diff:
From e796b25bbd0765231307ec4c757f67bfdef9e363 Mon Sep 17 00:00:00 2001
From: reshke <[email protected]>
Date: Thu, 15 Jan 2026 06:34:42 +0000
Subject: [PATCH v3] 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).
Reviewed-by: Andrey Borodin <[email protected]>
Reviewed-by: Soumya S Murali <[email protected]>
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 4d7c100d737..851d3679ce4 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -415,6 +415,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
*/
@@ -730,9 +733,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 a3db2c40936..1d944b493c2 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) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple';
has_new_heap_accesses
---------------------
-t
+f
(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 3b98ff9f76d..bd923c8bb00 100644
--- a/src/test/modules/index/specs/killtuples.spec
+++ b/src/test/modules/index/specs/killtuples.spec
@@ -96,7 +96,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.25.1
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix gistkillitems & add regression test to microvacuum
2026-01-20 10:30 Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
2026-01-23 11:03 ` Re: Fix gistkillitems & add regression test to microvacuum Kirill Reshke <[email protected]>
2026-01-27 07:26 ` Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
2026-02-11 12:18 ` Re: Fix gistkillitems & add regression test to microvacuum Soumya S Murali <[email protected]>
2026-03-18 05:42 ` Re: Fix gistkillitems & add regression test to microvacuum Kirill Reshke <[email protected]>
@ 2026-03-20 06:17 ` Soumya S Murali <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Soumya S Murali @ 2026-03-20 06:17 UTC (permalink / raw)
To: Kirill Reshke <[email protected]>; +Cc: Andrey Borodin <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>
Hi all,
On Wed, Mar 18, 2026 at 11:12 AM Kirill Reshke <[email protected]> wrote:
>
> On Wed, 11 Feb 2026 at 17:18, Soumya S Murali
> <[email protected]> wrote:
> >
> > Hi all,
> >
> > Thank you for the updated patch.
> > I reproduced the issue locally by filling a GiST root page, deleting
> > all tuples, and triggering a microvacuum by an index-only scan.
> > With the patch, The root page is now handled consistently with other
> > leaf pages. so->curBlkno and so->curPageLSN are properly set during
> > scan, and gistkillitems() operates safely and correctly on the root
> > page.
> > Based on runtime validation and code inspection, the fix LGTM.
> >
>
>
>
> Rebased due to f5eb854ab6d[0]
>
> I have added a [0]committer to CC.
>
> Andres, can you please take a look at this thread, if by chance you
> have spare time for it? You have changed kill tuples-related logic
> frequently, so you might be interested...
>
> [0] https://git.postgresql.org/cgit/postgresql.git/commit/?id=f5eb854ab6d6281ec2d3143657944bdda6676341
>
I performed a follow-up test on top of the v3 patch with a strictly
controlled setup to ensure a single-page GiST index (confirming that
block 1 is out of range). To force the cleanup path, I executed an
index-only scan after deleting all tuples and used a narrow WAL window
to isolate the relevant records. The WAL output shows GiST PAGE_UPDATE
on block 0, confirming that the root page is modified after the scan.
However even in this setup, I do not observe any XLOG_GIST_DELETE
records. This seems while the patch correctly initializes curBlkno and
enables proper handling of the root page, the DELETE WAL emission
depends on whether killitems actually marks tuples as DEAD under the
given visible conditions.
Overall, the fix looks correct from a code perspective, but in this
scenario the cleanup does not result in a DELETE WAL record.
Regards,
Soumya
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-03-20 06:17 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-01-20 10:30 Re: Fix gistkillitems & add regression test to microvacuum Andrey Borodin <[email protected]>
2026-01-23 11:03 ` Kirill Reshke <[email protected]>
2026-01-27 07:26 ` Andrey Borodin <[email protected]>
2026-02-11 12:18 ` Soumya S Murali <[email protected]>
2026-03-18 05:42 ` Kirill Reshke <[email protected]>
2026-03-20 06:17 ` Soumya S Murali <[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