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

Hello!

> 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.

Thanks for the review! Sorry for the late reply. I have reworked much
of the patch.

>> + * - 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?

REPEATABLE READ and SERIALIZABLE keep a registered transaction snapshot.
That snapshot keeps xmin from advancing, even if we replace the active
snapshot. SnapshotRotationDue() now skips rotation in these modes.

>> + 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.

Moved this into RotateSnapshot(). The cached catalog snapshot can also
keep xmin from advancing. We release it when we replace the scan snapshot.

> 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().
>
> 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.

Fixed as you suggested. RotateSnapshot() first registers the new
snapshot. It then pops the old snapshot, pushes the new one, and removes
the extra registration. xmin stays valid through all these steps.

> 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.

Agreed, removed need_pop_active_snapshot.

>> +++ 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.

Changed to concurrent_index_reset_snapshot_interval, default 100ms.
The code checks the time at safe points. It resets the snapshot when
the interval has passed. A reset can therefore happen later than 100ms.

> 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.

The build now holds an active snapshot at all times. Snapshot replacement
and its checks are in snapmgr.c.

> 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.

An AM enables this feature by setting amcanresetsnapshot. AMs that do
not set this flag keep the old behavior. bloom now sets this flag too.

GIN and BRIN need extra changes. They now fetch external TOAST data
before keeping values in memory for later use. Those values remain
valid after a snapshot reset.

AMs can also call index_build_rotate_snapshot() after the heap scan.
They call it at safe points, mostly near CHECK_FOR_INTERRUPTS().
It uses the same time interval.

Sorting and intermediate merges inside tuplesort still do not rotate
snapshots.

>> 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.

Done that way. The condition variable and counter are now in the shared
scan descriptor. The leader keeps its initial snapshot until all workers
have restored theirs. The WaitForParallelWorkersToAttach() API is unchanged.

> Either way, I don't like what happened in this patch.

Reworked this part.

> I think it'll at
> least need a better solution to schedule the required IO

It now works like this:

1) Sorting has a heap-fetch budget. These fetches let us report some
duplicate keys early.

2) _bt_load() has a separate heap-fetch budget. When no budget remains,
further entries with the same key are left out of the index.
Validation inserts any surviving tuples and checks uniqueness.

Both budgets use index_build_duplicate_check_scale_factor, default
0.05 (not measure).

An entry is also left for validation if a conflicting row is being
deleted or updated to avoid waiting for a transaction (to avoid a
false positive conflict).

One possible next step is to collect duplicate candidates in batches
and prefetch their heap pages using ReadStream
(or better leave them for STIR scan in the future - it uses ReadStream
over sorted tids).

Most tests were written with AI assistance. I have not reviewed them
closely yet.
Also, the latest commit is not fully reviewed, as it mostly shows the concept.

Best regards,
Mikhail.