Re: Snapshot related assert failure on skink

Hi,

On 2025-03-19 09:17:23 +0200, Heikki Linnakangas wrote:
> On 19/03/2025 04:22, Tomas Vondra wrote:
> > I kept stress-testing this, and while the frequency massively increased
> > on PG18, I managed to reproduce this all the way back to PG14. I see
> > ~100x more corefiles on PG18.
> > 
> > That is not a proof the issue was introduced in PG14, maybe it's just
> > the assert that was added there or something. Or maybe there's another
> > bug in PG18, making the impact worse.
> > 
> > But I'd suspect this is a bug in
> > 
> > commit 623a9ba79bbdd11c5eccb30b8bd5c446130e521c
> > Author: Andres Freund <andres@anarazel.de>
> > Date:   Mon Aug 17 21:07:10 2020 -0700
> > 
> >      snapshot scalability: cache snapshots using a xact completion counter.
> > 
> >      Previous commits made it faster/more scalable to compute snapshots.
> > But not
> >      building a snapshot is still faster. Now that GetSnapshotData() does not
> >      maintain RecentGlobal* anymore, that is actually not too hard:
> > 
> >      ...

Thanks for debugging and analyzing this!


> Looking at the code, shouldn't ExpireAllKnownAssignedTransactionIds() and
> ExpireOldKnownAssignedTransactionIds() update xactCompletionCount? This can
> happen during hot standby:
> 
> 1. Backend acquires snapshot A with xmin 1000
> 2. Startup process calls ExpireOldKnownAssignedTransactionIds(),
> 3. Backend acquires snapshot B with xmin 1050
> 4. Backend releases snapshot A, updating TransactionXmin to 1050
> 5. Backend acquires new snapshot, calls GetSnapshotDataReuse(), reusing
> snapshot A's data.
> 
> Because xactCompletionCount is not updated in step 2, the
> GetSnapshotDataReuse() call will reuse the snapshot A. But snapshot A has a
> lower xmin.

I've swapped a lot of the KnownAssigned* code out:

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?


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

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

Greetings,

Andres Freund