Hi, On 2021-05-26 16:57:31 +0900, Michael Paquier wrote: > Yes, there should not be any as far as I recall. 2PC is kind of > special with its fake ProcArray entries.
It's really quite an awful design :( > > I think to fix the issue we'd have to move > > ShutdownRecoveryTransactionEnvironment() to after > > XLogCtl->SharedRecoveryState > > = RECOVERY_STATE_DONE. > > > > The acquisition of ProcArrayLock() in > > ShutdownRecoveryTransactionEnvironment()->ExpireAllKnownAssignedTransactionIds() > > should prevent the data from being removed between the RecoveryInProgress() > > and the KnownAssignedXidsGetAndSetXmin() calls in GetSnapshotData(). > > > > I haven't yet figured out whether there would be a problem with deferring > > the > > other tasks in ShutdownRecoveryTransactionEnvironment() until after > > RECOVERY_STATE_DONE. > > Hmm. This would mean releasing all the exclusive locks tracked by a > standby, as of StandbyReleaseAllLocks(), after opening the instance > for writes after a promotion. I don't think that's unsafe, but it > would be intrusive. Why would it be intrusive? We're talking a split second here, no? More importantly, I don't think it's correct to release the locks at that point. > Anyway, isn't the issue ExpireAllKnownAssignedTransactionIds() itself, > where we should try to not wipe out the 2PC entries to make sure that > all those snapshots still see the 2PC transactions as something to > count on? I am attaching a crude patch to show the idea. I don't think that's sufficient. We can't do most of the other stuff in ShutdownRecoveryTransactionEnvironment() before changing XLogCtl->SharedRecoveryState either. As long as the other backends think we are in recovery, we shouldn't release e.g. the virtual transaction. Greetings, Andres Freund
