Horton

Re: remove spurious CREATE INDEX CONCURRENTLY wait

Michael Paquier <michael@paquier.xyz>

View thread View on postgr.es
From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Tom Lane <tgl@sss.pgh.pa.us>, Dmitry Dolgov <9erthalion6@gmail.com>, James Coleman <jtc331@gmail.com>, Pg Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2020-11-19T01:51:26Z
Lists: pgsql-hackers 
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

Commits

  1. Restore lock level to set vacuum flags

  2. Restore lock level to update statusFlags

  3. Avoid spurious waits in concurrent indexing

  4. Centralize logic for skipping useless ereport/elog calls.

  5. Don't hold ProcArrayLock longer than needed in rare cases

  6. Relax lock level for setting PGPROC->statusFlags

  7. Rename PGPROC->vacuumFlags to statusFlags

  8. snapshot scalability: Move PGXACT->vacuumFlags to ProcGlobal->vacuumFlags.

  9. Allow an autovacuum worker to be interrupted automatically when it is found