From 6ec2b6d2cbd746c7dfc0bc725c2d479b77b2048b Mon Sep 17 00:00:00 2001 From: Mikhail Nikalayeu Date: Thu, 8 Oct 2026 00:18:47 +0200 Subject: [PATCH v7 3/7] Allow snapshot resets in concurrent GIN builds GIN's build accumulator keeps keys across heap pages. If a key is stored out of line, copying it only copies the TOAST pointer. After a snapshot reset, vacuum could remove the referenced data before GIN writes the key to the index. Fetch external values when copying them into the accumulator, while the scan snapshot still protects them, and enable amcanresetsnapshot. Built-in opclasses already return keys without external TOAST pointers; this also makes resets safe for other opclasses. Reviewed-by: Matthias van de Meent --- src/backend/access/gin/ginbulk.c | 16 +++++++++++++--- src/backend/access/gin/ginutil.c | 3 +-- .../expected/cic_reset_snapshots.out | 2 ++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/backend/access/gin/ginbulk.c b/src/backend/access/gin/ginbulk.c index 85865b39105..a980efc96d2 100644 --- a/src/backend/access/gin/ginbulk.c +++ b/src/backend/access/gin/ginbulk.c @@ -16,6 +16,7 @@ #include +#include "access/detoast.h" #include "access/gin_private.h" #include "utils/datum.h" #include "utils/memutils.h" @@ -121,8 +122,8 @@ ginInitBA(BuildAccumulator *accum) } /* - * This is basically the same as datumCopy(), but extended to count - * palloc'd space in accum->allocatedMemory. + * Copy a key, fetching external values and tracking memory usage in + * accum->allocatedMemory. */ static Datum getDatumCopy(BuildAccumulator *accum, OffsetNumber attnum, Datum value) @@ -135,7 +136,16 @@ getDatumCopy(BuildAccumulator *accum, OffsetNumber attnum, Datum value) res = value; else { - res = datumCopy(value, false, att->attlen); + /* + * The accumulator keeps keys across heap pages. Fetch external + * values now, before a snapshot reset lets vacuum remove their TOAST + * data. Built-in extractValue functions already return values + * without external TOAST pointers. + */ + if (att->attlen == -1 && VARATT_IS_EXTERNAL(DatumGetPointer(value))) + res = PointerGetDatum(detoast_external_attr(DatumGetPointer(value))); + else + res = datumCopy(value, false, att->attlen); accum->allocatedMemory += GetMemoryChunkSpace(DatumGetPointer(res)); } return res; diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c index 2a961ad61bb..60ef9aeb80b 100644 --- a/src/backend/access/gin/ginutil.c +++ b/src/backend/access/gin/ginutil.c @@ -59,8 +59,7 @@ ginhandler(PG_FUNCTION_ARGS) .ampredlocks = true, .amcanparallel = false, .amcanbuildparallel = true, - /* not yet: the build accumulator keeps keys across heap pages */ - .amcanresetsnapshot = false, + .amcanresetsnapshot = true, .amcaninclude = false, .amusemaintenanceworkmem = true, .amsummarizing = false, diff --git a/src/test/modules/injection_points/expected/cic_reset_snapshots.out b/src/test/modules/injection_points/expected/cic_reset_snapshots.out index bf25cafb617..b460d45838d 100644 --- a/src/test/modules/injection_points/expected/cic_reset_snapshots.out +++ b/src/test/modules/injection_points/expected/cic_reset_snapshots.out @@ -129,7 +129,9 @@ REINDEX INDEX CONCURRENTLY cic_reset_snap.idx; NOTICE: notice triggered for injection point heap_beginscan_reset_snapshot DROP INDEX CONCURRENTLY cic_reset_snap.idx; CREATE INDEX CONCURRENTLY idx ON cic_reset_snap.tbl USING GIN((ARRAY[i, j])); +NOTICE: notice triggered for injection point heap_beginscan_reset_snapshot REINDEX INDEX CONCURRENTLY cic_reset_snap.idx; +NOTICE: notice triggered for injection point heap_beginscan_reset_snapshot DROP INDEX CONCURRENTLY cic_reset_snap.idx; -- The same in parallel mode ALTER TABLE cic_reset_snap.tbl SET (parallel_workers=2); -- 2.43.0