On Wed, Nov 18, 2020 at 11:09:28AM -0800, Andres Freund wrote: > Uh, wait a second. The acquisition of this lock hasn't been affected by > the snapshot scalability changes, and therefore are unrelated to > ->pgxactoff changing or not. > > In 13 this is: > LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); > MyPgXact->vacuumFlags |= PROC_IN_VACUUM; > if (params->is_wraparound) > MyPgXact->vacuumFlags |= PROC_VACUUM_FOR_WRAPAROUND; > LWLockRelease(ProcArrayLock); > > Lowering this to a shared lock doesn't seem right, at least without a > detailed comment explaining why it's safe. Because GetSnapshotData() etc > look at all procs with just an LW_SHARED ProcArrayLock, changing > vacuumFlags without a lock means that two concurrent horizon > computations could come to a different result. > > I'm not saying it's definitely wrong to relax things here, but I'm not > sure we've evaluated it sufficiently.
Yeah. While I do like the new assertion that 27838981 has added in ProcArrayEndTransactionInternal(), this commit feels a bit rushed to me. Echoing with my comment from upthread, I am not sure that we still document enough why a shared lock would be completely fine in the case of statusFlags. We have no hints that this could be fine before 2783898, and 2783898 does not make that look better. FWIW, I think that just using LW_EXCLUSIVE for the CIC change would have been fine enough first, after evaluating the performance impact. We could evaluate if it is possible to lower the ProcArrayLock acquisition in those code paths on a separate thread. -- Michael
signature.asc
Description: PGP signature
