gistkillitems can LP_DEAD-mark the wrong tuples on a rescan
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253424psql -h localhost -U postgresBuilt from patchset v7 (message #7), August 18, 2026 at 08:45 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253424_7 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253424_7 && git checkout t253424_7Patchset v7 (message #7) is on t253424_7
Attached patch adds a failing test case that shows wrong answers,
authored by Opus 5. The failures stem from GiST not invalidating
killedItems/numKilled across rescans: it's possible for the wrong
index tuples to have their LP_DEAD bits set due to state from a
previous rescan (namely killedItems and numKilled) getting mixed
together with state from the current rescan ( namely curBlkno and
curPageLSN). At a minimum, we should invalidate the
gistkillitems-related state across rescans.
Apparently the test case makes GiST mark index tuples that contain
downlinks in internal pages LP_DEAD, which is enough to make
gistScanPage() completely ignore whole subtrees. This is possible even
though such an index tuple should never be LP_DEAD marked in general
(obviously only tuples that point to heap TIDs should ever get
LP_DEAD-marked). gistScanPage has fairly uniform handling for leaf
pages and internal pages, which is what makes the "ignore whole
subtree" misbehavior possible.
This is *probably* only a bug on master, since only master has commit
9c9ddf109, which taught gistkillitems to run against a single-page
GiST index. However, we don't correctly reset the state on a rescan in
any version, which seems dicey even without commit 9c9ddf109.
I think that the proper fix here is have GiST deal with gistkillitems
in the same way that nbtree deals with _bt_killitems: gistkillitems
should be called in gistrescan and in gistendscan (not just in
gistgettuple), so that we correctly invalidate the state, *without*
neglecting to LP_DEAD-mark index tuples that really are dead-to-all.
--
Peter Geoghegan
On Sat, Aug 15, 2026 at 06:06:25PM -0400, Peter Geoghegan wrote:
This is *probably* only a bug on master, since only master has commit
9c9ddf109, which taught gistkillitems to run against a single-page
GiST index. However, we don't correctly reset the state on a rescan in
any version, which seems dicey even without commit 9c9ddf109.
My buildfarm member batta is running recovery test 027 with
wal_consistency_checking, with:
PG_TEST_EXTRA => "regress_dump_restore wal_consistency_checking saslprep",
Based on the last reports, we have no failures:
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=batta&br=master
If this was not caught, we have either:
- A problem in 027 itself, with coverage in the regression tests but
fail to detect the failure in the test. I recall that we are careful
about checking the standby state in 027 as an inconsistency would be
the same as a FATAL in a standby.
- Lack of coverage in the main regression test suite.
So, which one is it? Perhaps number 2), because such a test would be
expensive to run in the main regression test suite? It sounds to me
that we need a bit more than just your patch here.
--
Michael
On Sat, Aug 15, 2026 at 7:38 PM Michael Paquier <michael@paquier.xyz> wrote:
On Sat, Aug 15, 2026 at 06:06:25PM -0400, Peter Geoghegan wrote:
This is *probably* only a bug on master, since only master has commit
9c9ddf109, which taught gistkillitems to run against a single-page
GiST index. However, we don't correctly reset the state on a rescan in
any version, which seems dicey even without commit 9c9ddf109.My buildfarm member batta is running recovery test 027 with
wal_consistency_checking, with:
PG_TEST_EXTRA => "regress_dump_restore wal_consistency_checking saslprep",
Did you intend to respond to the nearby thread about
wal_consistency_checking? This thread is about LP_DEAD marking going
wrong in gist, but technically it's unrelated to the
wal_consistency_checking thing.
So, which one is it? Perhaps number 2), because such a test would be
expensive to run in the main regression test suite? It sounds to me
that we need a bit more than just your patch here.
We do have test coverage that might have caught the F_TUPLES_DELETED
issue that I complained about on the other thread, but it's not part
of the main regression tests. It's in
src/test/modules/index/specs/killtuples.spec. Could that be why?
--
Peter Geoghegan
On Sat, Aug 15, 2026 at 07:43:02PM -0400, Peter Geoghegan wrote:
On Sat, Aug 15, 2026 at 7:38 PM Michael Paquier <michael@paquier.xyz> wrote:
My buildfarm member batta is running recovery test 027 with
wal_consistency_checking, with:
PG_TEST_EXTRA => "regress_dump_restore wal_consistency_checking saslprep",Did you intend to respond to the nearby thread about
wal_consistency_checking? This thread is about LP_DEAD marking going
wrong in gist, but technically it's unrelated to the
wal_consistency_checking thing.
Sorry about that, wrong thread. :)
--
Michael
FWIW my buildfarm animal beetle also runs with wal_consistency_checking, so
batta is not the only one. Ofc, beetle did not find this issue either.
I think Peter's description of the inconsistency is correct.
This change also overlaps almost exactly with Kirill's earlier "GiST
nitpicks I want to discuss (and maybe eventually fix)" thread [0]/messages/by-id/CALdSSPgMYKpqkZAx32B2orx0Whev8-ik44oHtVkeDQz+hP=neA@mail.gmail.com. His
0001 stopped setting GistTuplesDeleted while retaining F_TUPLES_DELETED for
pageinspect and existing on-disk data. Peter's v2 test seems to supply the
important missing part.
There is also Alexander Lakhin's earlier work on GiST and hash regression
coverage [1]/messages/by-id/69ee05f7-8168-cce5-7f2d-d887f0aaf8bd@gmail.com, including the killtuples tests and WAL coverage. It seems
useful to cross-link these discussions. I'm interested in fixing those
items.
Best regards, Andrey Borodin.
[0]: /messages/by-id/CALdSSPgMYKpqkZAx32B2orx0Whev8-ik44oHtVkeDQz+hP=neA@mail.gmail.com
[1]: /messages/by-id/69ee05f7-8168-cce5-7f2d-d887f0aaf8bd@gmail.com
On 16 Aug 2026, at 15:00, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
wal_consistency_checking
Sorry, the message I just sent was intended for the nearby
"GiST wal_consistency_checking issue" thread. Since I have already bumped
this thread, let me reply to Peter's report too.
Looking back at Kirill's thread that led to 9c9ddf109 [0]/messages/by-id/CALdSSPgZWX_D8+Fx4YQqRN5eW5iSx_rJdqQhCfdWTvqKXVfJ4w@mail.gmail.com, I wrote:
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.
That did not age well. This bug is exactly the failure mode that observation
anticipated, so I no longer think that these fields should remain
independent.
Calling gistkillitems() from gistrescan() and gistendscan() looks like the
right fix. Longer term, I think the block number, page LSN and
killed offsets should form one kill-state object, initialized only for a
leaf page and invalidated as a unit. Visiting an internal page should never
replace the identity of the page to which pending kill offsets belong.
It also seems worth making the leaf-page check effective in non-assert
builds.
Best regards, Andrey Borodin.
[0]: /messages/by-id/CALdSSPgZWX_D8+Fx4YQqRN5eW5iSx_rJdqQhCfdWTvqKXVfJ4w@mail.gmail.com
[1]: /messages/by-id/lxzj26ga6ippdeunz6kuncectr5gfuugmm2ry22qu6hcx6oid6@lzx3sjsqhmt6
On Sun, Aug 16, 2026 at 8:24 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Calling gistkillitems() from gistrescan() and gistendscan() looks like the
right fix.
Attached patch does it that way. I'm not planning on committing my
original isolation test.
In the back branches, we should add hardening that defensively resets
numKilled on a rescan. This hardening might not be necessary, but
including it is easier than proving we don't need it. I've attached a
second very simple patch showing how I plan to do this on the
backbranches.
I'll commit these patches in the next couple of days, barring any
objections. For release management purposes these patches will be
treated as a single item.
Longer term, I think the block number, page LSN and
killed offsets should form one kill-state object, initialized only for a
leaf page and invalidated as a unit. Visiting an internal page should never
replace the identity of the page to which pending kill offsets belong.
The ongoing amgetbatch work (which enables index prefetching) forces
index AMs to do things that way: the equivalent of gistkillitems is
passed a batch that describes the items returned to the core executor
from a given leaf page earlier on. This batch data structure contains
a deadItems[] array that indicates which specific tuples from that
same page should be LP_DEAD-marked now. These dead item arrays are
tied to and accessed through one particular batch, so there's almost
no way for an index AM to misuse it; the index AM's opaque state
doesn't need to be kept in sync with the batch information passed
through by the core executor.
--
Peter Geoghegan