From 467751f76b65afcc63555812c0a9c57c3079c513 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Sat, 22 Aug 2026 11:23:12 +0300 Subject: [PATCH v2 2/3] Raise recovery conflicts when recycling GIN pages GIN decides that a deleted page is reusable from the primary's horizon alone and writes no WAL when it hands the page out, so nothing cancels a standby scan that still holds a reference to it. nbtree and GiST both log a page-reuse record for this. The test added by this commit shows how this oversight can lead to wrong answers on standbys. More generally, the scan can follow a stale block reference after replay has filled the block with an unrelated GIN page. What happens then depends on the new page type and contents, and is difficult to predict. Emit the existing GiST page-reuse record before GIN reuses a deleted page. Its layout and redo action are independent of the index AM, so using it preserves compatibility with older minor releases. Add a recovery test covering a standby scan concurrent with deletion and reuse on the primary. Discussion: https://postgr.es/m/CAH2-Wz=sKJcn+OtfVN9rdg+Ps9e4cuQWNP-9t12UE2d8nEG90Q@mail.gmail.com Backpatch-through: 14 --- src/backend/access/gin/ginget.c | 9 + src/backend/access/gin/ginutil.c | 40 +++++ src/test/recovery/Makefile | 3 +- src/test/recovery/meson.build | 1 + .../recovery/t/056_gin_page_reuse_conflict.pl | 164 ++++++++++++++++++ 5 files changed, 216 insertions(+), 1 deletion(-) create mode 100644 src/test/recovery/t/056_gin_page_reuse_conflict.pl diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c index 2bcb32ca3d0..89750bdf647 100644 --- a/src/backend/access/gin/ginget.c +++ b/src/backend/access/gin/ginget.c @@ -20,6 +20,7 @@ #include "miscadmin.h" #include "storage/predicate.h" #include "utils/datum.h" +#include "utils/injection_point.h" #include "utils/memutils.h" #include "utils/rel.h" @@ -787,6 +788,14 @@ entryLoadMoreItems(GinState *ginstate, GinScanEntry entry, } else LockBuffer(entry->buffer, GIN_UNLOCK); + + /* + * We return holding only a pin on entry->buffer. The next + * call reads this page's rightlink to step right, so the page + * has to still be the one we left behind. + */ + if (BufferIsValid(entry->buffer)) + INJECTION_POINT("gin-entry-load-more-items-pinned", NULL); return; } } diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c index e7cba81d477..434efe78de6 100644 --- a/src/backend/access/gin/ginutil.c +++ b/src/backend/access/gin/ginutil.c @@ -16,7 +16,9 @@ #include "access/gin_private.h" #include "access/ginxlog.h" +#include "access/gist_private.h" #include "access/reloptions.h" +#include "access/table.h" #include "access/xloginsert.h" #include "catalog/pg_collation.h" #include "catalog/pg_type.h" @@ -326,7 +328,45 @@ GinNewBuffer(Relation index) if (ConditionalLockBuffer(buffer)) { if (GinPageIsRecyclable(BufferGetPage(buffer))) + { + Page page = BufferGetPage(buffer); + + if (XLogStandbyInfoActive() && RelationNeedsWAL(index) && + GinPageGetDeleteXid(page) != InvalidTransactionId) + { + FullTransactionId nextfxid = ReadNextFullTransactionId(); + FullTransactionId deletefxid; + Relation heaprel; + + /* + * Legacy GIN pages only store the low 32 bits of the + * deletion XID. Interpret those as the latest occurrence + * not later than nextFullXid. This can give a conservative + * result for a page that survived an entire XID epoch, but + * avoids weakening recovery conflict handling. + */ + deletefxid = FullTransactionIdFromAllowableAt( + nextfxid, GinPageGetDeleteXid(page)); + + heaprel = table_open(index->rd_index->indrelid, NoLock); + + /* + * Use GiST's page-reuse record to make this backpatchable. + * The record is independent of the index AM and is already + * understood by older minor releases, so a patched primary + * can stream it to an unpatched standby. + * + * XXX Page reuse should eventually have a common WAL facility + * shared by nbtree, GiST, and GIN. + */ + gistXLogPageReuse(index, heaprel, + BufferGetBlockNumber(buffer), + deletefxid); + table_close(heaprel, NoLock); + } + return buffer; /* OK to use */ + } LockBuffer(buffer, GIN_UNLOCK); } diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile index 9c4102b6b2c..dfd3617c84c 100644 --- a/src/test/recovery/Makefile +++ b/src/test/recovery/Makefile @@ -9,7 +9,8 @@ # #------------------------------------------------------------------------- -EXTRA_INSTALL=contrib/pg_prewarm \ +EXTRA_INSTALL=contrib/pageinspect \ + contrib/pg_prewarm \ contrib/pg_stat_statements \ contrib/test_decoding \ src/test/modules/injection_points \ diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index 39ec8c4946d..681ac459027 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -64,6 +64,7 @@ tests += { 't/053_standby_login_event_trigger.pl', 't/054_unlogged_sequence_promotion.pl', 't/055_cascade_reconnect.pl', + 't/056_gin_page_reuse_conflict.pl', ], }, } diff --git a/src/test/recovery/t/056_gin_page_reuse_conflict.pl b/src/test/recovery/t/056_gin_page_reuse_conflict.pl new file mode 100644 index 00000000000..d02151da282 --- /dev/null +++ b/src/test/recovery/t/056_gin_page_reuse_conflict.pl @@ -0,0 +1,164 @@ +# Copyright (c) 2021-2026, PostgreSQL Global Development Group + +# Recycling a deleted GIN index page must raise a recovery conflict on a hot +# standby, as recycling a deleted nbtree or GiST page does. +# +# GIN decides a deleted page is reusable from the primary's horizon alone, and +# writes no WAL when it hands the page out. A standby snapshot is not in that +# horizon, and nothing tells the standby to cancel, so a standby scan holding a +# reference to the page goes on to read whatever the primary put there instead. +# +# Park a standby scan on a posting tree leaf it is holding, hand that page to +# another key on the primary, and ask the resumed scan and a sequential scan the +# same question under the same snapshot. They disagree. +# +# VACUUM prunes the deleted heap tuples in the same pass that empties the index +# pages, and that pruning would cancel the standby query on its own. So the +# snapshot is opened after the DELETE commits, past the pruning horizon. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node_primary = PostgreSQL::Test::Cluster->new('primary'); +$node_primary->init(allows_streaming => 1); +$node_primary->append_conf( + 'postgresql.conf', qq[ +autovacuum = off +# Cancel a conflicting standby query at once, so the test does not have to wait. +max_standby_streaming_delay = 0 +]); +$node_primary->start; + +# The modules may not be installed when this is run under installcheck. +if (!$node_primary->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} +if (!$node_primary->check_extension('pageinspect')) +{ + plan skip_all => 'Extension pageinspect not installed'; +} + +my $backup_name = 'my_backup'; +$node_primary->backup($backup_name); + +my $node_standby = PostgreSQL::Test::Cluster->new('standby'); +$node_standby->init_from_backup($node_primary, $backup_name, + has_streaming => 1); +$node_standby->append_conf( + 'postgresql.conf', qq[ +# The whole point is that the primary must not learn about standby snapshots. +hot_standby_feedback = off +]); +$node_standby->start; + +my $test_db = "test_gin_reuse"; +$node_primary->safe_psql('postgres', "CREATE DATABASE $test_db"); +$node_primary->safe_psql($test_db, "CREATE EXTENSION pageinspect"); +$node_primary->wait_for_replay_catchup($node_standby); + +my $point = 'gin-entry-load-more-items-pinned'; + +$node_primary->safe_psql( + $test_db, qq[ +CREATE EXTENSION injection_points; +CREATE TABLE w (id int, a int[]) WITH (autovacuum_enabled = off); +INSERT INTO w SELECT g, '{1}'::int[] FROM generate_series(1, 120000) g; +CREATE INDEX w_idx ON w USING gin (a) WITH (fastupdate = off); +]); + +# The scan pauses on the second leaf, so that is the page to delete and reuse. +# Empty it before the standby takes its snapshot, so pruning is not what +# cancels the standby query later. +$node_primary->safe_psql( + $test_db, qq[ +CREATE TABLE w_victim AS +SELECT blk FROM ( + SELECT b AS blk, + (SELECT min(t) + FROM gin_leafpage_items(get_raw_page('w_idx', b)) i, + LATERAL unnest(i.tids) t) AS lo + FROM generate_series(0, (pg_relation_size('w_idx') / + current_setting('block_size')::int)::int - 1) b + WHERE (gin_page_opaque_info(get_raw_page('w_idx', b))).flags + = '{data,leaf,compressed}' +) s +ORDER BY lo OFFSET 1 LIMIT 1; + +DELETE FROM w WHERE ctid IN ( + SELECT t + FROM w_victim v, + gin_leafpage_items(get_raw_page('w_idx', v.blk)) i, + LATERAL unnest(i.tids) t); +]); + +# Put the standby's snapshot clear of the pruning horizon. +$node_primary->safe_psql($test_db, "SELECT pg_current_xact_id()") for (1 .. 3); +$node_primary->wait_for_replay_catchup($node_standby); + +$node_standby->safe_psql($test_db, + "SELECT injection_points_attach('$point', 'wait')"); + +my $psql_w = $node_standby->background_psql($test_db, on_error_stop => 0); + +# Take the snapshot with a sequential scan, so it does not touch the GIN index. +$psql_w->query_safe( + "BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT count(*) FROM w;"); + +# Fire the index scan and return while it is still running. +$psql_w->query_until( + qr/scanning/, qq[ +\\echo scanning +SET enable_seqscan = off; +SELECT count(*) FROM w WHERE a \@> '{1}'; +]); + +$node_standby->poll_query_until($test_db, + "SELECT count(*) > 0 FROM pg_stat_activity WHERE wait_event = '$point'") + or die "timed out waiting for the standby scan to pause"; + +# Delete the pinned page and put it in the FSM. +$node_primary->safe_psql( + $test_db, qq[ +VACUUM (INDEX_CLEANUP ON) w; +SELECT pg_current_xact_id(); +VACUUM (INDEX_CLEANUP ON) w; +]); +my $deleted = $node_primary->safe_psql($test_db, + "SELECT (gin_page_opaque_info(get_raw_page('w_idx', v.blk))).flags \@> '{deleted}' FROM w_victim v" +); +is($deleted, 't', "the page the standby is pinning was deleted"); + +# Hand it to a different key, which takes it back out of the FSM silently. +$node_primary->safe_psql($test_db, + "INSERT INTO w SELECT 3000000 + g, '{2}'::int[] FROM generate_series(1, 120000) g" +); +my $reused = $node_primary->safe_psql($test_db, + "SELECT NOT ((gin_page_opaque_info(get_raw_page('w_idx', v.blk))).flags \@> '{deleted}') FROM w_victim v" +); +is($reused, 't', "the page was handed to another key while pinned"); + +$node_primary->wait_for_replay_catchup($node_standby); + +# Replaying page reuse must cancel the scan before it can read the new page. +$node_standby->safe_psql( + $test_db, "SELECT injection_points_detach('$point')"); + +$psql_w->query("ROLLBACK; SELECT 1;"); +like($psql_w->{stderr}, qr/canceling statement due to conflict with recovery/, + "page reuse canceled the standby scan"); + +eval { $psql_w->quit }; + +$node_standby->stop(); +$node_primary->stop(); + +done_testing(); -- That's all, folks. May the source be with you.