Avoid spurious waits in concurrent indexing

In the various waiting phases of CREATE INDEX CONCURRENTLY (CIC) and
REINDEX CONCURRENTLY (RC), we wait for other processes to release their
snapshots; this is necessary in general for correctness.  However,
processes doing CIC in other tables cannot possibly affect CIC or RC
done in "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 or RC can ignore it when
waiting.

Note that this logic is only valid if the index does not access other
tables.  For simplicity we avoid setting the flag if the index has a
column that's an expression, or has a WHERE predicate.  (It is possible
to have expressional or partial indexes that do not access other tables,
but figuring that out would require more work.)

This flag can potentially also be used by processes doing REINDEX
CONCURRENTLY to be skipped; and by VACUUM to ignore processes in CIC or
RC for the purposes of computing an Xmin.  That's left for future
commits.

Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Author: Dimitry Dolgov <9erthalion6@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/20200810233815.GA18970@alvherre.pgsql

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 02c7a0c7e1f09d4365c70ecc20d52de9b47c2057..ca24620fd0b184f14da5d87702286aa89a94801a 100644 (file)
--- 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_indexsafe_procflags(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,
        }
    }
 
+   /* Is index safe for others to ignore?  See set_indexsafe_procflags() */
+   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_indexsafe_procflags();
+
    /*
     * 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_indexsafe_procflags();
+
    /*
     * 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_indexsafe_procflags();
+
    /* We should now definitely not be advertising any xmin. */
    Assert(MyProc->xmin == InvalidTransactionId);
 
@@ -3896,3 +3919,37 @@ update_relispartition(Oid relationId, bool newval)
    heap_freetuple(tup);
    table_close(classRel, RowExclusiveLock);
 }
+
+/*
+ * Set the PROC_IN_SAFE_IC flag in MyProc->statusFlags.
+ *
+ * When doing concurrent index builds, we can set this flag
+ * to tell other processes concurrently running CREATE
+ * INDEX CONCURRENTLY or 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.
+ *
+ * This can be done safely only for indexes that don't execute any
+ * expressions that could access other tables, so index must not be
+ * expressional nor partial.  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_indexsafe_procflags(void)
+{
+   /*
+    * This should only be called before installing xid or xmin in MyProc;
+    * otherwise, concurrent processes could see an Xmin that moves backwards.
+    */
+   Assert(MyProc->xid == InvalidTransactionId &&
+          MyProc->xmin == InvalidTransactionId);
+
+   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 00bb244340a3e35f188182e764bd94c76ee67ebf..e75f6e8178254b1f7d637c8e4d7d59fb439a8318 100644 (file)
--- 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,