So I made the change to set the statusFlags with only LW_SHARED -- both
in existing uses (see 0002) and in the new CIC code (0003). I don't
think 0002 is going to have a tremendous performance impact, because it
changes operations that are very infrequent. But even so, it would be
weird to leave code around that uses exclusive lock when we're going to
add new code that uses shared lock for the same thing.
I still left the REINDEX CONCURRENTLY support in split out in 0004; I
intend to get the first three patches pushed now, and look into 0004
again later.
>From 5e2af4e082df8f839bf257d933d70bb9645f6cfe Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 17 Nov 2020 11:37:36 -0300
Subject: [PATCH v6 1/4] indexcmds.c: reorder function prototypes
... out of an overabundance of neatnikism, perhaps.
---
src/backend/commands/indexcmds.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 35696f9f75..02c7a0c7e1 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@
/* non-export function prototypes */
+static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
Oid *typeOidP,
@@ -87,13 +88,11 @@ static char *ChooseIndexNameAddition(List *colnames);
static List *ChooseIndexColumnNames(List *indexElems);
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
-static bool ReindexRelationConcurrently(Oid relationOid, int options);
-
+static void reindex_error_callback(void *args);
static void ReindexPartitions(Oid relid, int options, bool isTopLevel);
static void ReindexMultipleInternal(List *relids, int options);
-static void reindex_error_callback(void *args);
+static bool ReindexRelationConcurrently(Oid relationOid, int options);
static void update_relispartition(Oid relationId, bool newval);
-static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
/*
* callback argument type for RangeVarCallbackForReindexIndex()
--
2.20.1
>From 3e2778d553448c67e956bd385c4a94039933d3c6 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 17 Nov 2020 13:48:16 -0300
Subject: [PATCH v6 2/4] relax lock level for setting statusFlags
---
src/backend/commands/vacuum.c | 2 +-
src/backend/replication/logical/logical.c | 2 +-
src/backend/replication/slot.c | 2 +-
src/backend/storage/ipc/procarray.c | 2 ++
4 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index d89ba3031f..395e75f768 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1741,7 +1741,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
* MyProc->xid/xmin, otherwise GetOldestNonRemovableTransactionId()
* might appear to go backwards, which is probably Not Good.
*/
- LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
MyProc->statusFlags |= PROC_IN_VACUUM;
if (params->is_wraparound)
MyProc->statusFlags |= PROC_VACUUM_FOR_WRAPAROUND;
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index f1f4df7d70..4324e32656 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_EXCLUSIVE);
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
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 5ed955ba0b..9c7cf13d4d 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -527,7 +527,7 @@ ReplicationSlotRelease(void)
MyReplicationSlot = NULL;
/* might not have been set when we've been a plain slot */
- LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
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 e2cb6e990d..ebe91907d6 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -662,6 +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 only share lock */
+ Assert(proc == MyProc);
Assert(!LWLockHeldByMe(ProcArrayLock));
LWLockAcquire(ProcArrayLock, LW_SHARED);
Assert(proc->statusFlags == ProcGlobal->statusFlags[proc->pgxactoff]);
--
2.20.1
>From 5578f8824e9e9baccd9a3e1ad0b970bb120e4793 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 17 Nov 2020 11:41:19 -0300
Subject: [PATCH v6 3/4] CREATE INDEX CONCURRENTLY: don't wait for its kin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
In the various waiting phases of CREATE INDEX CONCURRENTLY, we wait
for other processes to release their snapshots; in general terms this is
necessary for correctness. However, processes doing CIC for other
tables cannot possibly affect CIC done on "this" table, so we don't need
to wait for *those*. This commit adds a flag in MyProc->statusFlags to
indicate that the current process is doing CIC, so that other processes
doing CIC can ignore it when waiting.
This flag can potentially also be used by processes doing REINDEX
CONCURRENTLY also to avoid waiting, and by VACUUM to ignore the process
in CIC for the purposes of computing an Xmin. That's left for future
commits.
Author: Álvaro Herrera <alvhe...@alvh.no-ip.org>
Author: Dimitry Dolgov <9erthali...@gmail.com>
Reviewed-by: Michael Paquier <mich...@paquier.xyz>
Discussion: https://postgr.es/m/20200810233815.GA18970@alvherre.pgsql
---
src/backend/commands/indexcmds.c | 55 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 5 ++-
2 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 02c7a0c7e1..e4acdf1e1f 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -93,6 +93,7 @@ static void ReindexPartitions(Oid relid, int options, bool isTopLevel);
static void ReindexMultipleInternal(List *relids, int options);
static bool ReindexRelationConcurrently(Oid relationOid, int options);
static void update_relispartition(Oid relationId, bool newval);
+static inline void set_safe_index_flag(void);
/*
* callback argument type for RangeVarCallbackForReindexIndex()
@@ -384,7 +385,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
+ * 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.
@@ -405,7 +409,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);
@@ -425,7 +430,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++)
{
@@ -518,6 +524,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1044,6 +1051,10 @@ DefineIndex(Oid relationId,
}
}
+ /* Determine whether we can call set_safe_index_flag */
+ safe_index = indexInfo->ii_Expressions == NIL &&
+ indexInfo->ii_Predicate == NIL;
+
/*
* Report index creation if appropriate (delay this till after most of the
* error checks)
@@ -1430,6 +1441,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.
*/
@@ -1489,6 +1504,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
*
@@ -1545,6 +1564,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);
@@ -3896,3 +3919,29 @@ update_relispartition(Oid relationId, bool newval)
heap_freetuple(tup);
table_close(classRel, RowExclusiveLock);
}
+
+/*
+ * Set the PROC_IN_SAFE_IC flag in my PGPROC entry.
+ *
+ * When doing concurrent index builds, we can set this flag
+ * to tell other processes concurrently running CREATE
+ * INDEX CONCURRENTLY to ignore us when
+ * doing their waits for concurrent snapshots. On one hand it
+ * avoids pointlessly waiting for a process that's not interesting
+ * anyway, but more importantly it avoids deadlocks in some cases.
+ *
+ * This can only be done for indexes that don't execute any expressions.
+ * Caller is responsible for only calling this routine when that
+ * assumption holds true.
+ *
+ * (The flag is reset automatically at transaction end, so it must be
+ * set for each transaction.)
+ */
+static inline void
+set_safe_index_flag(void)
+{
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+ 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 1067f58f51..2673ac4c37 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -53,13 +53,16 @@ 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 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.20.1
>From 14e9e62716e71dc446b53b9dcfa954df3f0ec600 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 17 Nov 2020 11:42:22 -0300
Subject: [PATCH v6 4/4] Support safe flag in REINDEX CONCURRENTLY
---
src/backend/commands/indexcmds.c | 27 +++++++++++++++++++++++++--
src/include/storage/proc.h | 1 +
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e4acdf1e1f..7bf641a2bd 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ 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.) Processes running CREATE INDEX CONCURRENTLY
+ * 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.
@@ -3043,6 +3043,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
PROGRESS_CREATEIDX_ACCESS_METHOD_OID
};
int64 progress_vals[4];
+ bool all_indexes_safe = true;
/*
* Create a memory context that will survive forced transaction commits we
@@ -3347,6 +3348,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock);
+ /* consider safety of this index for set_safe_index_flag */
+ if (all_indexes_safe &&
+ (newIndexRel->rd_indexprs != NIL ||
+ newIndexRel->rd_indpred != NIL))
+ all_indexes_safe = false;
+
/*
* Save the list of OIDs and locks in private context
*/
@@ -3416,6 +3423,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (all_indexes_safe)
+ set_safe_index_flag();
+
/*
* Phase 2 of REINDEX CONCURRENTLY
*
@@ -3479,6 +3490,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
}
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (all_indexes_safe)
+ set_safe_index_flag();
+
/*
* Phase 3 of REINDEX CONCURRENTLY
*
@@ -3632,6 +3647,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (all_indexes_safe)
+ set_safe_index_flag();
+
/*
* Phase 5 of REINDEX CONCURRENTLY
*
@@ -3664,6 +3683,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (all_indexes_safe)
+ set_safe_index_flag();
+
/*
* Phase 6 of REINDEX CONCURRENTLY
*
@@ -3925,7 +3948,7 @@ update_relispartition(Oid relationId, bool newval)
*
* When doing concurrent index builds, we can set this flag
* to tell other processes concurrently running CREATE
- * INDEX CONCURRENTLY to ignore us when
+ * INDEX CONCURRENTLY and REINDEX CONCURRENTLY to ignore us when
* doing their waits for concurrent snapshots. On one hand it
* avoids pointlessly waiting for a process that's not interesting
* anyway, but more importantly it avoids deadlocks in some cases.
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 2673ac4c37..b2347ffd79 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ 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 */
--
2.20.1
