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)