From 80ce1087bc1650f5674f163f599ffcd6b0a7c222 Mon Sep 17 00:00:00 2001 From: Mikhail Nikalayeu Date: Thu, 8 Oct 2026 00:18:47 +0200 Subject: [PATCH v8 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 85865b3910..a980efc96d 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 2a961ad61b..60ef9aeb80 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 bf25cafb61..b460d45838 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