> On Fri, Nov 13, 2020 at 09:25:40AM +0900, Michael Paquier wrote:
> On Thu, Nov 12, 2020 at 04:36:32PM +0100, Dmitry Dolgov wrote:
> > Interesting enough, similar discussion happened about vaccumFlags before
> > with the same conclusion that theoretically it's fine to update without
> > holding the lock, but this assumption could change one day and it's
> > better to avoid such risks. Having said that I believe it makes sense to
> > continue with locking. Are there any other opinions? I'll try to
> > benchmark it in the meantime.
>
> Thanks for planning some benchmarking for this specific patch. I have
> to admit that the possibility of switching vacuumFlags to use atomics
> is very appealing in the long term, with or without considering this
> patch, even if we had better be sure that this patch has no actual
> effect on concurrency first if atomics are not used in worst-case
> scenarios.
I've tried first to test scenarios where GetSnapshotData produces
significant lock contention and "reindex concurrently" implementation
with locks interferes with it. The idea I had is to create a test
function that constantly calls GetSnapshotData (perf indeed shows
significant portion of time spent on contended lock), and clash it with
a stream of "reindex concurrently" of an empty relation (which still
reaches safe_index check). I guess it could be considered as an
artificial extreme case. Measuring GetSnapshotData (or rather the
surrounding wrapper, to distinguish calls from the test function from
everything else) latency without reindex, with reindex and locks, with
reindex without locks should produce different "modes" and comparing
them we can make some conclusions.
Latency histograms without reindex (nanoseconds):
nsecs : count distribution
512 -> 1023 : 0 | |
1024 -> 2047 : 10001209 |****************************************|
2048 -> 4095 : 76936 | |
4096 -> 8191 : 1468 | |
8192 -> 16383 : 98 | |
16384 -> 32767 : 39 | |
32768 -> 65535 : 6 | |
The same with reindex without locks:
nsecs : count distribution
512 -> 1023 : 0 | |
1024 -> 2047 : 111345 | |
2048 -> 4095 : 6997627 |****************************************|
4096 -> 8191 : 18575 | |
8192 -> 16383 : 586 | |
16384 -> 32767 : 312 | |
32768 -> 65535 : 18 | |
The same with reindex with locks:
nsecs : count distribution
512 -> 1023 : 0 | |
1024 -> 2047 : 59438 | |
2048 -> 4095 : 6901187 |****************************************|
4096 -> 8191 : 18584 | |
8192 -> 16383 : 581 | |
16384 -> 32767 : 280 | |
32768 -> 65535 : 84 | |
Looks like with reindex without locks is indeed faster (there are mode
samples in lower time section), but not particularly significant to the
whole distribution, especially taking into account extremity of the
test.
I'll take a look at benchmarking of switching vacuumFlags to use
atomics, but as it's probably a bit off topic I'm going to attach
another version of the patch with locks and suggested changes. To which
I have one question:
> Michael Paquier <mich...@paquier.xyz> writes:
> I think that this should be in its own routine, and that we had better
> document that this should be called just after starting a transaction,
> with an assertion enforcing that.
I'm not sure which exactly assertion condition do you mean?
>From 07c4705a22e6cdd5717df46a974ce00a69fc901f Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Wed, 11 Nov 2020 15:19:48 +0100
Subject: [PATCH v4 1/2] Rename vaccumFlags to statusFlags
With more flags associated to a PGPROC entry that are not related to
vacuum (currently existing or planned), the name "statusFlags" describes
its purpose better.
---
src/backend/access/transam/twophase.c | 2 +-
src/backend/commands/vacuum.c | 6 +--
src/backend/postmaster/autovacuum.c | 6 +--
src/backend/replication/logical/logical.c | 4 +-
src/backend/replication/slot.c | 4 +-
src/backend/storage/ipc/procarray.c | 54 +++++++++++------------
src/backend/storage/lmgr/deadlock.c | 4 +-
src/backend/storage/lmgr/proc.c | 20 ++++-----
src/include/storage/proc.h | 12 ++---
9 files changed, 56 insertions(+), 56 deletions(-)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 7940060443..873bf9bad9 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -464,7 +464,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
proc->xid = xid;
Assert(proc->xmin == InvalidTransactionId);
proc->delayChkpt = false;
- proc->vacuumFlags = 0;
+ proc->statusFlags = 0;
proc->pid = 0;
proc->backendId = InvalidBackendId;
proc->databaseId = databaseid;
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 1b6717f727..d89ba3031f 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1742,10 +1742,10 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
* might appear to go backwards, which is probably Not Good.
*/
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
- MyProc->vacuumFlags |= PROC_IN_VACUUM;
+ MyProc->statusFlags |= PROC_IN_VACUUM;
if (params->is_wraparound)
- MyProc->vacuumFlags |= PROC_VACUUM_FOR_WRAPAROUND;
- ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ MyProc->statusFlags |= PROC_VACUUM_FOR_WRAPAROUND;
+ ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
LWLockRelease(ProcArrayLock);
}
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2cef56f115..aa5b97fbac 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2509,7 +2509,7 @@ do_autovacuum(void)
tab->at_datname, tab->at_nspname, tab->at_relname);
EmitErrorReport();
- /* this resets ProcGlobal->vacuumFlags[i] too */
+ /* this resets ProcGlobal->statusFlags[i] too */
AbortOutOfAnyTransaction();
FlushErrorState();
MemoryContextResetAndDeleteChildren(PortalContext);
@@ -2525,7 +2525,7 @@ do_autovacuum(void)
did_vacuum = true;
- /* ProcGlobal->vacuumFlags[i] are reset at the next end of xact */
+ /* ProcGlobal->statusFlags[i] are reset at the next end of xact */
/* be tidy */
deleted:
@@ -2702,7 +2702,7 @@ perform_work_item(AutoVacuumWorkItem *workitem)
cur_datname, cur_nspname, cur_relname);
EmitErrorReport();
- /* this resets ProcGlobal->vacuumFlags[i] too */
+ /* this resets ProcGlobal->statusFlags[i] too */
AbortOutOfAnyTransaction();
FlushErrorState();
MemoryContextResetAndDeleteChildren(PortalContext);
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index d5cfbeaa4a..f1f4df7d70 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -182,8 +182,8 @@ StartupDecodingContext(List *output_plugin_options,
if (!IsTransactionOrTransactionBlock())
{
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
- MyProc->vacuumFlags |= PROC_IN_LOGICAL_DECODING;
- ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ MyProc->statusFlags |= PROC_IN_LOGICAL_DECODING;
+ ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
LWLockRelease(ProcArrayLock);
}
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 09be1d8c48..5ed955ba0b 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -528,8 +528,8 @@ ReplicationSlotRelease(void)
/* might not have been set when we've been a plain slot */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
- MyProc->vacuumFlags &= ~PROC_IN_LOGICAL_DECODING;
- ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ 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 05661e379e..3a28da0e80 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -488,13 +488,13 @@ ProcArrayAdd(PGPROC *proc)
(arrayP->numProcs - index) * sizeof(*ProcGlobal->xids));
memmove(&ProcGlobal->subxidStates[index + 1], &ProcGlobal->subxidStates[index],
(arrayP->numProcs - index) * sizeof(*ProcGlobal->subxidStates));
- memmove(&ProcGlobal->vacuumFlags[index + 1], &ProcGlobal->vacuumFlags[index],
- (arrayP->numProcs - index) * sizeof(*ProcGlobal->vacuumFlags));
+ memmove(&ProcGlobal->statusFlags[index + 1], &ProcGlobal->statusFlags[index],
+ (arrayP->numProcs - index) * sizeof(*ProcGlobal->statusFlags));
arrayP->pgprocnos[index] = proc->pgprocno;
ProcGlobal->xids[index] = proc->xid;
ProcGlobal->subxidStates[index] = proc->subxidStatus;
- ProcGlobal->vacuumFlags[index] = proc->vacuumFlags;
+ ProcGlobal->statusFlags[index] = proc->statusFlags;
arrayP->numProcs++;
@@ -562,7 +562,7 @@ ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
Assert(TransactionIdIsValid(ProcGlobal->xids[proc->pgxactoff] == 0));
Assert(TransactionIdIsValid(ProcGlobal->subxidStates[proc->pgxactoff].count == 0));
Assert(TransactionIdIsValid(ProcGlobal->subxidStates[proc->pgxactoff].overflowed == false));
- ProcGlobal->vacuumFlags[proc->pgxactoff] = 0;
+ ProcGlobal->statusFlags[proc->pgxactoff] = 0;
for (index = 0; index < arrayP->numProcs; index++)
{
@@ -575,8 +575,8 @@ ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
(arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->xids));
memmove(&ProcGlobal->subxidStates[index], &ProcGlobal->subxidStates[index + 1],
(arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->subxidStates));
- memmove(&ProcGlobal->vacuumFlags[index], &ProcGlobal->vacuumFlags[index + 1],
- (arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->vacuumFlags));
+ memmove(&ProcGlobal->statusFlags[index], &ProcGlobal->statusFlags[index + 1],
+ (arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->statusFlags));
arrayP->pgprocnos[arrayP->numProcs - 1] = -1; /* for debugging */
arrayP->numProcs--;
@@ -660,13 +660,13 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
/* must be cleared with xid/xmin: */
/* avoid unnecessarily dirtying shared cachelines */
- if (proc->vacuumFlags & PROC_VACUUM_STATE_MASK)
+ if (proc->statusFlags & PROC_VACUUM_STATE_MASK)
{
Assert(!LWLockHeldByMe(ProcArrayLock));
LWLockAcquire(ProcArrayLock, LW_SHARED);
- Assert(proc->vacuumFlags == ProcGlobal->vacuumFlags[proc->pgxactoff]);
- proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
- ProcGlobal->vacuumFlags[proc->pgxactoff] = proc->vacuumFlags;
+ Assert(proc->statusFlags == ProcGlobal->statusFlags[proc->pgxactoff]);
+ proc->statusFlags &= ~PROC_VACUUM_STATE_MASK;
+ ProcGlobal->statusFlags[proc->pgxactoff] = proc->statusFlags;
LWLockRelease(ProcArrayLock);
}
}
@@ -695,10 +695,10 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
/* must be cleared with xid/xmin: */
/* avoid unnecessarily dirtying shared cachelines */
- if (proc->vacuumFlags & PROC_VACUUM_STATE_MASK)
+ if (proc->statusFlags & PROC_VACUUM_STATE_MASK)
{
- proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
- ProcGlobal->vacuumFlags[proc->pgxactoff] = proc->vacuumFlags;
+ proc->statusFlags &= ~PROC_VACUUM_STATE_MASK;
+ ProcGlobal->statusFlags[proc->pgxactoff] = proc->statusFlags;
}
/* Clear the subtransaction-XID cache too while holding the lock */
@@ -875,7 +875,7 @@ ProcArrayClearTransaction(PGPROC *proc)
proc->xmin = InvalidTransactionId;
proc->recoveryConflictPending = false;
- Assert(!(proc->vacuumFlags & PROC_VACUUM_STATE_MASK));
+ Assert(!(proc->statusFlags & PROC_VACUUM_STATE_MASK));
Assert(!proc->delayChkpt);
/*
@@ -1710,7 +1710,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
{
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- int8 vacuumFlags = ProcGlobal->vacuumFlags[index];
+ int8 statusFlags = ProcGlobal->statusFlags[index];
TransactionId xid;
TransactionId xmin;
@@ -1745,7 +1745,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
* removed, as long as pg_subtrans is not truncated) or doing logical
* decoding (which manages xmin separately, check below).
*/
- if (vacuumFlags & (PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING))
+ if (statusFlags & (PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING))
continue;
/* shared tables need to take backends in all database into account */
@@ -2194,7 +2194,7 @@ GetSnapshotData(Snapshot snapshot)
TransactionId *xip = snapshot->xip;
int *pgprocnos = arrayP->pgprocnos;
XidCacheStatus *subxidStates = ProcGlobal->subxidStates;
- uint8 *allVacuumFlags = ProcGlobal->vacuumFlags;
+ uint8 *allStatusFlags = ProcGlobal->statusFlags;
/*
* First collect set of pgxactoff/xids that need to be included in the
@@ -2204,7 +2204,7 @@ GetSnapshotData(Snapshot snapshot)
{
/* Fetch xid just once - see GetNewTransactionId */
TransactionId xid = UINT32_ACCESS_ONCE(other_xids[pgxactoff]);
- uint8 vacuumFlags;
+ uint8 statusFlags;
Assert(allProcs[arrayP->pgprocnos[pgxactoff]].pgxactoff == pgxactoff);
@@ -2243,8 +2243,8 @@ GetSnapshotData(Snapshot snapshot)
* Skip over backends doing logical decoding which manages xmin
* separately (check below) and ones running LAZY VACUUM.
*/
- vacuumFlags = allVacuumFlags[pgxactoff];
- if (vacuumFlags & (PROC_IN_LOGICAL_DECODING | PROC_IN_VACUUM))
+ statusFlags = allStatusFlags[pgxactoff];
+ if (statusFlags & (PROC_IN_LOGICAL_DECODING | PROC_IN_VACUUM))
continue;
if (NormalTransactionIdPrecedes(xid, xmin))
@@ -2483,11 +2483,11 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
{
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- int vacuumFlags = ProcGlobal->vacuumFlags[index];
+ int statusFlags = ProcGlobal->statusFlags[index];
TransactionId xid;
/* Ignore procs running LAZY VACUUM */
- if (vacuumFlags & PROC_IN_VACUUM)
+ if (statusFlags & PROC_IN_VACUUM)
continue;
/* We are only interested in the specific virtual transaction. */
@@ -3142,7 +3142,7 @@ IsBackendPid(int pid)
* If excludeXmin0 is true, skip processes with xmin = 0.
* If allDbs is false, skip processes attached to other databases.
* If excludeVacuum isn't zero, skip processes for which
- * (vacuumFlags & excludeVacuum) is not zero.
+ * (statusFlags & excludeVacuum) is not zero.
*
* Note: the purpose of the limitXmin and excludeXmin0 parameters is to
* allow skipping backends whose oldest live snapshot is no older than
@@ -3176,12 +3176,12 @@ GetCurrentVirtualXIDs(TransactionId limitXmin, bool excludeXmin0,
{
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- uint8 vacuumFlags = ProcGlobal->vacuumFlags[index];
+ uint8 statusFlags = ProcGlobal->statusFlags[index];
if (proc == MyProc)
continue;
- if (excludeVacuum & vacuumFlags)
+ if (excludeVacuum & statusFlags)
continue;
if (allDbs || proc->databaseId == MyDatabaseId)
@@ -3596,7 +3596,7 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
{
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- uint8 vacuumFlags = ProcGlobal->vacuumFlags[index];
+ uint8 statusFlags = ProcGlobal->statusFlags[index];
if (proc->databaseId != databaseId)
continue;
@@ -3610,7 +3610,7 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
else
{
(*nbackends)++;
- if ((vacuumFlags & PROC_IS_AUTOVACUUM) &&
+ if ((statusFlags & PROC_IS_AUTOVACUUM) &&
nautovacs < MAXAUTOVACPIDS)
autovac_pids[nautovacs++] = proc->pid;
}
diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c
index e1246b8a4d..cb7a8f0fd2 100644
--- a/src/backend/storage/lmgr/deadlock.c
+++ b/src/backend/storage/lmgr/deadlock.c
@@ -618,7 +618,7 @@ FindLockCycleRecurseMember(PGPROC *checkProc,
* that an autovacuum won't be canceled with less than
* deadlock_timeout grace period.
*
- * Note we read vacuumFlags without any locking. This is
+ * Note we read statusFlags without any locking. This is
* OK only for checking the PROC_IS_AUTOVACUUM flag,
* because that flag is set at process start and never
* reset. There is logic elsewhere to avoid canceling an
@@ -628,7 +628,7 @@ FindLockCycleRecurseMember(PGPROC *checkProc,
* ProcArrayLock.
*/
if (checkProc == MyProc &&
- proc->vacuumFlags & PROC_IS_AUTOVACUUM)
+ proc->statusFlags & PROC_IS_AUTOVACUUM)
blocking_autovacuum_proc = proc;
/* We're done looking at this proclock */
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 88566bd9fa..d1738c65f5 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -111,7 +111,7 @@ ProcGlobalShmemSize(void)
size = add_size(size, mul_size(TotalProcs, sizeof(*ProcGlobal->xids)));
size = add_size(size, mul_size(TotalProcs, sizeof(*ProcGlobal->subxidStates)));
- size = add_size(size, mul_size(TotalProcs, sizeof(*ProcGlobal->vacuumFlags)));
+ size = add_size(size, mul_size(TotalProcs, sizeof(*ProcGlobal->statusFlags)));
return size;
}
@@ -210,8 +210,8 @@ InitProcGlobal(void)
MemSet(ProcGlobal->xids, 0, TotalProcs * sizeof(*ProcGlobal->xids));
ProcGlobal->subxidStates = (XidCacheStatus *) ShmemAlloc(TotalProcs * sizeof(*ProcGlobal->subxidStates));
MemSet(ProcGlobal->subxidStates, 0, TotalProcs * sizeof(*ProcGlobal->subxidStates));
- ProcGlobal->vacuumFlags = (uint8 *) ShmemAlloc(TotalProcs * sizeof(*ProcGlobal->vacuumFlags));
- MemSet(ProcGlobal->vacuumFlags, 0, TotalProcs * sizeof(*ProcGlobal->vacuumFlags));
+ ProcGlobal->statusFlags = (uint8 *) ShmemAlloc(TotalProcs * sizeof(*ProcGlobal->statusFlags));
+ MemSet(ProcGlobal->statusFlags, 0, TotalProcs * sizeof(*ProcGlobal->statusFlags));
for (i = 0; i < TotalProcs; i++)
{
@@ -393,10 +393,10 @@ InitProcess(void)
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
MyProc->delayChkpt = false;
- MyProc->vacuumFlags = 0;
+ MyProc->statusFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
- MyProc->vacuumFlags |= PROC_IS_AUTOVACUUM;
+ MyProc->statusFlags |= PROC_IS_AUTOVACUUM;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
MyProc->waitLock = NULL;
@@ -574,7 +574,7 @@ InitAuxiliaryProcess(void)
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
MyProc->delayChkpt = false;
- MyProc->vacuumFlags = 0;
+ MyProc->statusFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
MyProc->waitLock = NULL;
@@ -1310,7 +1310,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM && allow_autovacuum_cancel)
{
PGPROC *autovac = GetBlockingAutoVacuumPgproc();
- uint8 vacuumFlags;
+ uint8 statusFlags;
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
@@ -1318,9 +1318,9 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
* Only do it if the worker is not working to protect against Xid
* wraparound.
*/
- vacuumFlags = ProcGlobal->vacuumFlags[autovac->pgxactoff];
- if ((vacuumFlags & PROC_IS_AUTOVACUUM) &&
- !(vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))
+ statusFlags = ProcGlobal->statusFlags[autovac->pgxactoff];
+ if ((statusFlags & PROC_IS_AUTOVACUUM) &&
+ !(statusFlags & PROC_VACUUM_FOR_WRAPAROUND))
{
int pid = autovac->pid;
StringInfoData locktagbuf;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..63aea0e253 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -49,7 +49,7 @@ struct XidCache
};
/*
- * Flags for ProcGlobal->vacuumFlags[]
+ * Flags for ProcGlobal->statusFlags[]
*/
#define PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */
#define PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */
@@ -175,9 +175,9 @@ struct PGPROC
bool delayChkpt; /* true if this proc delays checkpoint start */
- uint8 vacuumFlags; /* this backend's vacuum flags, see PROC_*
+ uint8 statusFlags; /* this backend's status flags, see PROC_*
* above. mirrored in
- * ProcGlobal->vacuumFlags[pgxactoff] */
+ * ProcGlobal->statusFlags[pgxactoff] */
/*
* Info to allow us to wait for synchronous replication, if needed.
* waitLSN is InvalidXLogRecPtr if not waiting; set only by user backend.
@@ -273,7 +273,7 @@ extern PGDLLIMPORT PGPROC *MyProc;
* allow for as tight loops accessing the data as possible. Second, to prevent
* updates of frequently changing data (e.g. xmin) from invalidating
* cachelines also containing less frequently changing data (e.g. xid,
- * vacuumFlags). Third to condense frequently accessed data into as few
+ * statusFlags). Third to condense frequently accessed data into as few
* cachelines as possible.
*
* There are two main reasons to have the data mirrored between these dense
@@ -315,10 +315,10 @@ typedef struct PROC_HDR
XidCacheStatus *subxidStates;
/*
- * Array mirroring PGPROC.vacuumFlags for each PGPROC currently in the
+ * Array mirroring PGPROC.statusFlags for each PGPROC currently in the
* procarray.
*/
- uint8 *vacuumFlags;
+ uint8 *statusFlags;
/* Length of allProcs array */
uint32 allProcCount;
--
2.21.0
>From 0474461b71420d7331d0d59b84ae497ffb20f0d1 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 4 Aug 2020 22:04:57 -0400
Subject: [PATCH v4 2/2] Avoid spurious CREATE INDEX CONCURRENTLY waits
---
src/backend/commands/indexcmds.c | 92 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 ++-
2 files changed, 94 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 75552c64ed..4abb60ea44 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -94,6 +94,7 @@ static void ReindexMultipleInternal(List *relids, int options);
static void reindex_error_callback(void *args);
static void update_relispartition(Oid relationId, bool newval);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
+static void set_safe_index_flag(void);
/*
* callback argument type for RangeVarCallbackForReindexIndex()
@@ -385,7 +386,10 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
* lazy VACUUMs, because they won't be fazed by missing index entries
* either. (Manual ANALYZEs, however, can't be excluded because they
* might be within transactions that are going to do arbitrary operations
- * later.)
+ * later.) Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
+ * on indexes that are neither expressional nor partial are also safe to
+ * ignore, since we know that those processes won't examine any data
+ * outside the table they're indexing.
*
* Also, GetCurrentVirtualXIDs never reports our own vxid, so we need not
* check for that.
@@ -406,7 +410,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
VirtualTransactionId *old_snapshots;
old_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false,
- PROC_IS_AUTOVACUUM | PROC_IN_VACUUM,
+ PROC_IS_AUTOVACUUM | PROC_IN_VACUUM
+ | PROC_IN_SAFE_IC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -426,7 +431,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
newer_snapshots = GetCurrentVirtualXIDs(limitXmin,
true, false,
- PROC_IS_AUTOVACUUM | PROC_IN_VACUUM,
+ PROC_IS_AUTOVACUUM | PROC_IN_VACUUM
+ | PROC_IN_SAFE_IC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -519,6 +525,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1045,6 +1052,17 @@ DefineIndex(Oid relationId,
}
}
+ /*
+ * When doing concurrent index builds, we can set a PGPROC flag to tell
+ * concurrent VACUUM, CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY
+ * to ignore us when waiting for concurrent snapshots. That can only be
+ * done for indexes that don't execute any expressions. Determine that.
+ * (The flag is reset automatically at transaction end, so it must be
+ * set for each transaction.)
+ */
+ safe_index = indexInfo->ii_Expressions == NIL &&
+ indexInfo->ii_Predicate == NIL;
+
/*
* Report index creation if appropriate (delay this till after most of the
* error checks)
@@ -1431,6 +1449,10 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1490,6 +1512,10 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 3 of concurrent index build
*
@@ -1546,6 +1572,10 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
@@ -3021,6 +3051,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
PROGRESS_CREATEIDX_ACCESS_METHOD_OID
};
int64 progress_vals[4];
+ bool safe_index = true;
/*
* Create a memory context that will survive forced transaction commits we
@@ -3324,6 +3355,23 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock);
+ /*
+ * When doing concurrent reindex, we can set a PGPROC flag to tell
+ * concurrent VACUUM, CREATE INDEX CONCURRENTLY and REINDEX
+ * CONCURRENTLY to ignore us when waiting for concurrent snapshots.
+ * That can only be done for indexes that don't execute any
+ * expressions. Determine that for all involved indexes together. (The
+ * flag is reset automatically at transaction end, so it must be set
+ * for each transaction.)
+ */
+ if (safe_index)
+ {
+ IndexInfo *newIndexInfo = BuildIndexInfo(newIndexRel);
+
+ safe_index = newIndexInfo->ii_Expressions == NIL &&
+ newIndexInfo->ii_Predicate == NIL;
+ }
+
/*
* Save the list of OIDs and locks in private context
*/
@@ -3393,6 +3441,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 2 of REINDEX CONCURRENTLY
*
@@ -3456,6 +3508,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
}
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 3 of REINDEX CONCURRENTLY
*
@@ -3559,6 +3615,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
forboth(lc, indexIds, lc2, newIndexIds)
{
char *oldName;
@@ -3609,6 +3669,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 5 of REINDEX CONCURRENTLY
*
@@ -3641,6 +3705,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 6 of REINDEX CONCURRENTLY
*
@@ -3692,6 +3760,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* Start a new transaction to finish process properly */
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/* Log what we did */
if (options & REINDEXOPT_VERBOSE)
{
@@ -3896,3 +3968,17 @@ update_relispartition(Oid relationId, bool newval)
heap_freetuple(tup);
table_close(classRel, RowExclusiveLock);
}
+
+/*
+ * Set a PGPROC flag to tell concurrent VACUUM, CREATE INDEX CONCURRENTLY and
+ * REINDEX CONCURRENTLY to ignore us when waiting for concurrent snapshots.
+ * Should be called just after starting a transaction.
+ */
+static void
+set_safe_index_flag()
+{
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc->statusFlags |= PROC_IN_SAFE_IC;
+ ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+ LWLockRelease(ProcArrayLock);
+}
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 63aea0e253..6664803e2a 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -53,13 +53,17 @@ struct XidCache
*/
#define PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */
#define PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */
+#define PROC_IN_SAFE_IC 0x04 /* currently running CREATE INDEX
+ * CONCURRENTLY or REINDEX
+ * CONCURRENTLY on non-expressional,
+ * non-partial index */
#define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */
#define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical
* decoding outside xact */
/* flags reset at EOXact */
#define PROC_VACUUM_STATE_MASK \
- (PROC_IN_VACUUM | PROC_VACUUM_FOR_WRAPAROUND)
+ (PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.21.0
