Re: Snapshot related assert failure on skink

On 21/03/2025 17:16, Andres Freund wrote:
> Am I right in understanding that the only scenario (when in
> STANDBY_SNAPSHOT_READY), where ExpireOldKnownAssignedTransactionIds() would
> "legally" remove a transaction, rather than the commit / abort records doing
> so, is if the primary crash-restarted while transactions were ongoing?
>
> Those transactions won't have a commit/abort records and thus won't trigger
> ExpireTreeKnownAssignedTransactionIds(), which otherwise would have updated
> ->xactCompletionCount?

Correct.

> When writing the snapshot caching patch, I tried to make sure that all the
> places that maintain ->latestCompletedXid also update
> ->xactCompletionCount. Afaict that's still the case. Which, I think, means
> that we're also missing calls to MaintainLatestCompletedXidRecovery()?

Yep, I was just looking at that too.

> If latestCompletedXid is incorrect visibility determinations end up wrong...

I think it happens to work, because the transactions are effectively
aborted. latestCompletedXid is used to initialize xmax in
GetSnapshotData. If, for example, latestCompletedXid is incorrectly set
to 1000 even though XID 1001 already aborted, a snapshot with xmax=1000
still correctly considers XID 1001 as "not visible". As soon as a
transaction commits, latestCompletedXid is fixed.

AFAICS we could skip updating latestCompletedXid on aborts altogether
and rename it to latestCommittedXid. But it's hardly worth optimizing
for aborts.

For the same reason, I believe the assertion failure we're discussing
here is also harmless. Even though the reused snapshot has a smaller
xmin than expected, all those transactions aborted and are thus not
visible anyway.

In any case, let's be tidy and fix both latestCompletedXid and
xactCompletionCount.

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