On 2020-Nov-23, Andres Freund wrote: > On 2020-11-23 12:30:05 -0300, Alvaro Herrera wrote:
> > In other words, my conclusion is that there definitely is a bug here and > > I am going to restore the use of exclusive lock for setting the > > statusFlags. > > Cool. Here's a patch. Note it also moves the computation of vacuum's Xmin (per GetTransactionSnapshot) to *after* the bit has been set in statusFlags.
>From b813c67a4abe2127b8bd13db7e920f958db15d59 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Tue, 24 Nov 2020 18:10:42 -0300 Subject: [PATCH] Restore lock level to update statusFlags Reverts 27838981be9d (some comments are kept). Per discussion, it does not seem safe to relax the lock level used for this; in order for it to be safe, there would have to be memory barriers between the point we set the flag and the point we set the trasaction Xid, which perhaps would not be so bad; but there would also have to be barriers at the readers' side, which from a performance perspective might be bad. Now maybe this analysis is wrong and it *is* safe for some reason, but proof of that is not trivial. Discussion: https://postgr.es/m/20201118190928.vnztes7c2sldu...@alap3.anarazel.de --- src/backend/commands/vacuum.c | 20 +++++++++++--------- src/backend/replication/logical/logical.c | 2 +- src/backend/storage/ipc/procarray.c | 4 +--- src/include/storage/proc.h | 6 +++--- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 395e75f768..f1112111de 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -1712,13 +1712,6 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) /* Begin a transaction for vacuuming this relation */ StartTransactionCommand(); - /* - * Need to acquire a snapshot to prevent pg_subtrans from being truncated, - * cutoff xids in local memory wrapping around, and to have updated xmin - * horizons. - */ - PushActiveSnapshot(GetTransactionSnapshot()); - if (!(params->options & VACOPT_FULL)) { /* @@ -1739,9 +1732,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) * Note: these flags remain set until CommitTransaction or * AbortTransaction. We don't want to clear them until we reset * MyProc->xid/xmin, otherwise GetOldestNonRemovableTransactionId() - * might appear to go backwards, which is probably Not Good. + * might appear to go backwards, which is probably Not Good. (We also + * set PROC_IN_VACUUM *before* taking our own snapshot, so that our + * xmin doesn't become visible ahead of setting the flag.) */ - LWLockAcquire(ProcArrayLock, LW_SHARED); + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); MyProc->statusFlags |= PROC_IN_VACUUM; if (params->is_wraparound) MyProc->statusFlags |= PROC_VACUUM_FOR_WRAPAROUND; @@ -1749,6 +1744,13 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) LWLockRelease(ProcArrayLock); } + /* + * Need to acquire a snapshot to prevent pg_subtrans from being truncated, + * cutoff xids in local memory wrapping around, and to have updated xmin + * horizons. + */ + PushActiveSnapshot(GetTransactionSnapshot()); + /* * Check for user-requested abort. Note we want this to be inside a * transaction, so xact.c doesn't issue useless WARNING. diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index 4324e32656..f1f4df7d70 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -181,7 +181,7 @@ StartupDecodingContext(List *output_plugin_options, */ if (!IsTransactionOrTransactionBlock()) { - LWLockAcquire(ProcArrayLock, LW_SHARED); + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); MyProc->statusFlags |= PROC_IN_LOGICAL_DECODING; ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; LWLockRelease(ProcArrayLock); diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 94edb24b22..c7848c0b69 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -662,10 +662,8 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid) /* avoid unnecessarily dirtying shared cachelines */ if (proc->statusFlags & PROC_VACUUM_STATE_MASK) { - /* Only safe to change my own flags with just share lock */ - Assert(proc == MyProc); Assert(!LWLockHeldByMe(ProcArrayLock)); - LWLockAcquire(ProcArrayLock, LW_SHARED); + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); Assert(proc->statusFlags == ProcGlobal->statusFlags[proc->pgxactoff]); proc->statusFlags &= ~PROC_VACUUM_STATE_MASK; ProcGlobal->statusFlags[proc->pgxactoff] = proc->statusFlags; diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 00bb244340..22046e4e36 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -99,9 +99,9 @@ typedef enum * but its myProcLocks[] lists are valid. * * We allow many fields of this struct to be accessed without locks, such as - * statusFlags or delayChkpt. However, keep in mind that writing mirrored ones - * (see below) requires holding ProcArrayLock or XidGenLock in at least shared - * mode, so that pgxactoff does not change concurrently. + * delayChkpt and isBackgroundWorker. However, keep in mind that writing + * mirrored ones (see below) requires holding ProcArrayLock or XidGenLock in + * at least shared mode, so that pgxactoff does not change concurrently. * * Mirrored fields: * -- 2.20.1
