A few patches to clarify snapshot management

While working on the CSN snapshot patch, I got sidetracked looking
closer into the snapshot tracking in snapmgr.c. Attached are a few
patches to clarify some things.

# Patch 1: Remove unnecessary GetTransactionSnapshot() calls and FIXME
comments

In commit dc7420c2c927, Andres added FIXME comments like these in a few
places:

autovacuum.c, get_database_list(void):
> /*
> * Start a transaction so we can access pg_database, and get a snapshot.
> * We don't have a use for the snapshot itself, but we're interested in
> * the secondary effect that it sets RecentGlobalXmin. (This is critical
> * for anything that reads heap pages, because HOT may decide to prune
> * them even if the process doesn't attempt to modify any tuples.)
> *
> * FIXME: This comment is inaccurate / the code buggy. A snapshot that is
> * not pushed/active does not reliably prevent HOT pruning (->xmin could
> * e.g. be cleared when cache invalidations are processed).
> */
> StartTransactionCommand();
> (void) GetTransactionSnapshot();

Those GetTransactionSnapshot() calls are unnecessary, because we hold
onto registered copy of CatalogSnapshot throughout the catalog scans.
This patch removes those unnecessary calls, and the FIXMEs.

# Patch 2: Assert that a snapshot is active or registered before it's used

GetTransactionSnapshot() comment said:

> * Note that the return value may point at static storage that will be modified
> * by future calls and by CommandCounterIncrement(). Callers should call
> * RegisterSnapshot or PushActiveSnapshot on the returned snap if it is to be
> * used very long.

That's pretty vague. Firstly, it says the returned value _may_ point to
static storage, but ISTM it _always_ does, if you interpret "static
storage" liberally. Some callers actually rely on the fact that you can
call GetTransactionSnapshot() and throw away the result without having a
leak. So I propose rewording that to "return value points at static
storage", rather than just "may point".

In REPEATABLE READ mode, the returned CurrentSnapshot is palloc'd, not a
pointer directly to a static variable, but all calls within the same
transaction return the same palloc'd Snapshot pointer, and will be
modified by CommandCounterIncrement(). From the caller's point of view,
it's like a static.

Secondly, what exactly is "used very long"? It means until the next call
of any of the Get*Snapshot() functions, CommandCounterIncrement(), or
anything that might call SnapshotResetXmin() like PopActiveSnapshot().
Given how complicated that gets, I feel it's dangerous to do pretty much
anything else than immediately call PushActiveSnapshot() or
RegisterSnapshot() with it. To try to enforce that, this patch adds an
assertion in HeapTupleSatisfiesMVCC() that the snapshot must be
registered or pushed active. That's not a very accurate check of that
stricter rule: some callers were violating the new assertion and had
comments to explain why it was safe, and OTOH it won't catch calls to
those invalidating functions that don't involve visibility checks.

We were violating that assertion in a few places, which were not wrong
and had explaining comments, but this patch changes them to just
register the snapshot instead of explaining why it's safe to skip it.

# Patch 3: Add comment with more details on active snapshots

Now that I have this swapped in my head, I wrote a few paragraphs on how
the active snapshot stack works at high level.

# Patch 4: Add checks that no snapshots are "leaked"

This patch is not to be committed right now, just for discussion.

I'm not very happy with how GetTransactionSnapshot() and friends return
a statically allocated snapshot. The whole "return value should not be
used very long" thing is just so vague. If we changed it to return a
palloc'd snapshot, would we introduce leaks? This patch adds assertions
that every call to GetTransactionSnapshot() is paired with a
PushActiveSnapshot() or RegsiterSnapshot() call, and changes a few
places that were violating that stricter rule. Some of those changes
seem nice anyway, like registering the snapshot in verify_heapam(), even
though they're not strictly necessary today.

A perhaps better way to enforce that would be to replace
GetTransactionSnapshot() with functions that also push or register the
snapshot:

RegisterSnapshot(GetTransactionSnapshot()) -> RegisterTransactionSnapshot()

PushActiveSnapshot(GetTransactionSnapshot()) -> PushTransactionSnapshot()

That function signature would eliminate the concept of a returned
statically-allocated snapshot, and the whole question of what does "used
very long" mean in GetTransactionSnapshot(). Thoughts on that?

--
Heikki Linnakangas
Neon (https://neon.tech)