Re: Resetting snapshots during the first phase of [CREATE |RE]INDEX CONCURRENTLY

On Sat, 18 Apr 2026 at 15:34, Mihail Nikalayeu
<mihailnikalayeu(at)gmail(dot)com> wrote:
>
> Some small fixes around.

Here's my long-overdue review of this patchset. I've reviewed all but
patch 0001, as I assume that patch to not get committed.

------

0002:

The patch title and description could use some love. The title is too
long (whole paragraph), and the remainder could use some manual
wrapping

> +++ b/src/include/catalog/index.h
> + * Snapshot resetting is only applicable when all of:
> + * - the build is concurrent (ii_Concurrent)
> [...]
> + * - isolation level is not REPEATABLE READ/SERIALIZABLE (those keep a
> + * registered transaction snapshot)

This seems strange to me - with R/CIC we know we're a top-level
command that outlives every transaction that touches the index's
table, so why do we suddenly care about serializability?

> +++ b/contrib/amcheck/verify_nbtree.c
> - true); /* syncscan OK? */
> + true, /* syncscan OK? */
> + false);

Please add the comment indicating what the flag indicates, e.g. /*
reset_snapshot OK? */

> +++ b/src/backend/access/brin/brin.c
[...]
> + {
> snapshot = RegisterSnapshot(GetTransactionSnapshot());
> + PushActiveSnapshot(GetTransactionSnapshot());
> + }

The snapshot management in _brin_begin_parallel here seems wasteful;
It does both RegisterSnapshot() and PushActiveSnapshot() on the output
of GetTransactionSnapshot(). This usually causes the same snapshot to
be allocated and copied twice. Why not call PushActiveSnapshot() with
the result of RegisterSnapshot(), like what _bt_begin_parallel does?
I.e.

+ {
snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ PushActiveSnapshot(snapshot);
+ }

The same comment applies for _gin_begin_parallel.

> + InvalidateCatalogSnapshot();

I see this sprinkled around in various places that don't directly do
any other snapshot management -related things, nor do they seem to
handle much related to catalog content management. Why are they
sprinkled around those places, and not as part of the main scan
snapshot management? It's not just in brin.c, but also other AMs,
like access/hash/hash.c, which doesn't have *any* other modifications.

> +++ b/src/backend/access/heap/heapam.c
>
> +/*
> + * Reset the active snapshot during a scan.
> + * This ensures the xmin horizon can advance while maintaining safe tuple visibility.
> + * Note: No other snapshot should be active during this operation.
> + */
> +static inline void
> +heap_reset_scan_snapshot(TableScanDesc sscan)
> +{
> + /* Make sure no other snapshot was set as active. */
> + Assert(GetActiveSnapshot() == sscan->rs_snapshot);
> + /* And make sure active snapshot is not registered. */
> + Assert(GetActiveSnapshot()->regd_count == 0);

Snapshots MUST be registered before you can use them. It is inherently
unsafe to not register snapshots you're going to use, so please use
UnregisterSnapshot here, and RegisterSnapshot plus
PushActiveSnapshot().

Note that UnregisterSnapshot() also updates MyProc->xmin through
SnapshotResetXmin, so that should be one less concern about correcting
xmin invalidations.

> + PopActiveSnapshot();
> +
> + sscan->rs_snapshot = InvalidSnapshot; /* just to be tidy */
> + Assert(!HaveRegisteredOrActiveSnapshot());
> + InvalidateCatalogSnapshot();
> +
> + /* The goal of snapshot reset is to allow horizon to advance. */
> + Assert(!TransactionIdIsValid(MyProc->xmin));
> +#if USE_INJECTION_POINTS
> + /* In some cases it is still not possible due xid assign. */
> + if (!TransactionIdIsValid(MyProc->xid))
> + INJECTION_POINT("heap_reset_scan_snapshot_effective", NULL);
> +#endif
> +
> + PushActiveSnapshot(GetLatestSnapshot());
> + sscan->rs_snapshot = GetActiveSnapshot();

I think it's safer to do things in this order:

old_snap = sscan->rs_snapshot;
new_snap = RegisterSnapshot(GetLatestSnapshot());
PopActiveSnapshot();
PushActiveSnapshot(new_snap);
sscan->rs_snapshot = new_snap;
UnregisterSnapshot(old_snap);

This avoids clearing MyProc->xmin, it only increases it if the value
actually changes.

The "no registered snapshots" issue extends to various other pieces of
code, that'll all need to be updated accordingly to make sure they
actually register their snapshots, so that other code which does
correctly handle snapshots doesn't accidentally lose track of the
backend's xmins.

> + heap_reset_scan_snapshot((TableScanDesc) scan);

I don't think this part makes much sense. HeapTableScanDesc contains a
TableScanDesc at offset 0, so the cast itself is safe, but either
heap_reset_* should be table_reset_* (and live in tableam.c), or
accept a HeapTableScanDesc.

> +++ b/src/backend/optimizer/plan/planner.c
> + /* Set ActiveSnapshot since functions in the indexes may need it */
> + if (!ActiveSnapshotSet())
> + {
> + PushActiveSnapshot(GetTransactionSnapshot());
> + need_pop_active_snapshot = true;
> + }

Shouldn't this be called exclusively from inside CREATE INDEX
commands, which should have an active snapshot when it starts calling
into index operations? The whole need_pop_active_snapshot idea seems
like it fixes the wrong issue.

> +++ b/src/backend/utils/misc/guc_parameters.dat
> + variable => 'concurrent_index_reset_snapshot_every_n_pages',

I'm not fully convinced that this is the right way to do this.
Some pages may take much longer to process than others (e.g. TOAST
pages are not accounted for, a huge gin pending list, ...), so I think
a timer will make more sense. You can test if your snapshot has aged
with xactCompletionCount (currently not an atomic integer, so requires
ProcArrayLock, but a little bit of effort would fix that [0]), and
reset your baseline time if it hasn't changed yet to avoid resetting
snapshots if no transaction has committed since you first acquired the
snapshot. Then you can reset the snapshot every X milliseconds after
we need to update the snapshot, rather than assuming that page counts
are a good (and safe) approximation of snapshot xmin holdback delay.

> +++ b/src/backend/catalog/index.c
> - /* This had better make sure that a snapshot is active */
> - Assert(ActiveSnapshotSet());

> + Assert(!TransactionIdIsValid(MyProc->xmin));
> + Assert(!TransactionIdIsValid(MyProc->xid));

I think you'll have to find a better solution than "don't hold a
snapshot, unless". We should always be holding a snapshot in this scan
phase, and just change out which snapshot that is every once in a
while. The invalidations of snapshots, and the assertion of a lack of
snapshots, at arbitrary points in the code seems extremely fragile,
and doesn't feel like a well thought-through design. If all index AMs
are required to implement changes to support this (like how all
builtin AMs have changed, except contrib/bloom), then I don't think
this will be a viable approach.

------

Comments on 0003:

> +++ b/src/backend/access/brin/brin.c
> + reset_snapshot = isconcurrent && !IsolationUsesXactSnapshot();
(etc., across various AMs)

Why are index AMs so deeply involved in what should be
TableScan-internal behaviour? It seems like it shows it's the wrong
abstraction.

> WaitForParallelWorkersToAttach

I don't like this getting shoe-horned into the generic
WaitForParallelWorkersToAttach. It doesn't make much sense to me to
modify this here, shouldn't a ConditionVariable and a shared counter
(number of successfully initialized backends), controlled by the
shared table scan infrastructure, be sufficient for this? Then the
primary can wait on that CV until the shared counter reaches
nworkers_launched, without changing the API for parallel contexts.

------

Comments on 0004:

I think this patch needs a lot more effort. I don't think that it
won't work, but it changes things around that it probably shouldn't
change (e.g. _bt_keep_natts()/_fast()), and there are various code
style issues, like the following:

> +++ b/src/backend/access/nbtree/nbtsort.c
> [...]
> + _bt_buildadd(wstate, state, itup, 0);
> + if (prev) pfree(prev);
> + prev = CopyIndexTuple(itup);

I'm also not super happy about the inconsistent use of
IndexBuildResetsSnapshots().

E.g., heapam_handler.c disables reset_snapshots when
is_system_catalog, but no index AM code that tests
IndexBuildResetsSnapshots agrees with that. BtSpool now has
unique_dead_ignored, and _bt_leafbuild() gets a new effectively-unused
parameter reset_snapshots that approximately mirrors the value of the
field.

Either way, I don't like what happened in this patch. I think it'll at
least need a better solution to schedule the required IO - random IOs
shouldn't be the solution, and reduce its changes on hot paths like
_bt_keep_natts()/_fast().

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

[0] https://postgr.es/m/CAEze2Wjj5LPNpuq6fNy-wv552wbk61dGRJ52NdV%2BWddq0Uf33A%40mail.gmail.com