Catalog invalidations vs catalog scans vs ScanPgRelation()

Hi,

While self reviewing a patch I'm about to send I changed the assertion
in index_getnext_tid from
Assert(TransactionIdIsValid(RecentGlobalXmin))
to instead test (via an indirection)
Assert(TransactionIdIsValid(MyProc->xmin))

Without ->xmin being set, it's not safe to do scans. And
checking RecentGlobalXmin doesn't really do much.

But, uh. That doesn't get very far through the regression tests.

Turns out that I am to blame for that. All the way back in 9.4. For
logical decoding I needed to make ScanPgRelation() use a specific type
of snapshot during one corner case of logical decoding. For reasons lost
to time, I didn't continue to pass NULL to systable_beginscan() in the
plain case, but did an explicit GetCatalogSnapshot(RelationRelationId).
Note the missing RegisterSnapshot()...

That's bad because:

a) If invalidation processing triggers a InvalidateCatalogSnapshot(),
the contained SnapshotResetXmin() may find no other snapshot, and
reset ->xmin. Which then may cause relevant row versions to be
removed.
b) If there's a subsequent GetCatalogSnapshot() during invalidation
processing, that will GetSnapshotData() into the snapshot currently
being used.

The fix itself is trivial, just pass NULL for the normal case, rather
than doing GetCatalogSnapshot().

But I think this points to some severe holes in relevant assertions /
infrastructure:

1) Asserting that RecentGlobalXmin is set - like many routines do -
isn't meaningful, because it stays set even if SnapshotResetXmin()
releases the transaction's snapshot. These are fairly old assertions
(d53a56687f3d). As far as I can tell these routines really should
verify that a snapshot is set.

2) I think we need to reset TransactionXmin, RecentXmin whenever
SnapshotResetXmin() clears xmin. While we'll set them again the next
time a snapshot is acquired, the fact that they stay set seems likely
to hide bugs.

We also could remove TransactionXmin and instead use the
pgproc/pgxact's ->xmin. I don't really see the point of having it?

3) Similarly, I think we ought to reset reset RecentGlobal[Data]Xmin at
the end of the transaction or such.

But I'm not clear what protects those values from being affected by
wraparound in a longrunning transaction? Initially they are obviously
protected against that due to the procarray - but once the oldest
procarray entry releases its snapshot, the global xmin horizon can
advance. That allows transactions that up to ~2 billion into the
future of the current backend, whereas RecentGlobalXmin might be
nearly ~2 billion transactions in the past relative to ->xmin.

That might not have been a likely problem many years ago, but seems
far from impossible today?

I propose to commit a fix, but then also add an assertion for
TransactionIdIsValid(MyPgXact->xmin) instead (or in addition) to the
TransactionIdIsValid(RecentGlobalXmin) tests right now. And in master
clear the various *Xmin variables whenever we reset xmin.

I think in master we should also start to make RecentGlobalXmin etc
FullTransactionIds. We can then convert the 32bit xids we compare with
RecentGlobal* to 64bit xids (which is safe, because live xids have to be
within [oldestXid, nextXid)). I have that as part of another patch
anyway...

Greetings,

Andres Freund