Re: Locks release order in LogStandbySnapshot

Hi,

On 2022-11-09 11:03:04 +0800, Japin Li wrote:
> GetRunningTransactionData requires holding both ProcArrayLock and
> XidGenLock (in that order).  Then LogStandbySnapshot releases those
> locks in that order.  However, to reduce the frequency of having to
> wait for XidGenLock while holding ProcArrayLock, ProcArrayAdd releases
> them in reversed acquisition order.
>
> The comments of LogStandbySnapshot says:
> 
> > GetRunningTransactionData() acquired ProcArrayLock, we must release it.
> > For Hot Standby this can be done before inserting the WAL record
> > because ProcArrayApplyRecoveryInfo() rechecks the commit status using
> > the clog. For logical decoding, though, the lock can't be released
> > early because the clog might be "in the future" from the POV of the
> > historic snapshot. This would allow for situations where we're waiting
> > for the end of a transaction listed in the xl_running_xacts record
> > which, according to the WAL, has committed before the xl_running_xacts
> > record. Fortunately this routine isn't executed frequently, and it's
> > only a shared lock.
> 
> This comment is only for ProcArrayLock, not for XidGenLock. IIUC,
> LogCurrentRunningXacts doesn't need holding XidGenLock, right?

I think it does. If we allow xid assignment before LogCurrentRunningXacts() is
done, those new xids would not have been mentioned in the xl_running_xacts
record, despite already running. Which I think result in corrupted snapshots
during hot standby and logical decoding.


> Does there any sense to release them in reversed acquisition order in
> LogStandbySnapshot like ProcArrayRemove?

I don't think it's worth optimizing for, this happens at a low frequency
(whereas connection establishment can be very frequent). And due to the above,
we can sometimes release ProcArrayLock earlier.

Greetings,

Andres Freund