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