Date: Tue, 4 Aug 2020 22:11:09 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: James Coleman <jtc331@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: [DOC] Document concurrent index builds waiting on each other
Message-ID: <20200805021109.GA9079@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="VS++wcV0S1rZb1Fb"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200325195841.gq4hpl25t6pxv3gl@alap3.anarazel.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


--VS++wcV0S1rZb1Fb
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2020-Mar-25, Andres Freund wrote:

> What I was thinking of was a new flag, with a distinct value from
> PROC_IN_VACUUM. It'd currently just be specified in the
> GetCurrentVirtualXIDs() calls in WaitForOlderSnapshots(). That'd avoid
> needing to wait for other CICs on different relations. Since CIC is not
> permitted on system tables, and CIC doesn't do DML on normal tables, it
> seems fairly obviously correct to exclude other CICs.

Hmm, that does work, and seems a pretty small patch -- attached.  Of
course, some more commentary is necessary, but the theory of operation
is as Andres says.  (It does not solve the vacuuming problem I was
describing in the other thread, only the spurious waiting that James is
complaining about in this thread.)

I'm going to try and poke holes on this now ... (Expression indexes with
falsely immutable functions?)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--VS++wcV0S1rZb1Fb
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Flag-CREATE-INDEX-CONCURRENTLY-to-avoid-spurious-wai.patch"

From a691c48ddd5d9ff99e2f17b91777028bd5fbf36b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 4 Aug 2020 22:04:57 -0400
Subject: [PATCH] Flag CREATE INDEX CONCURRENTLY to avoid spurious waiting

---
 src/backend/commands/indexcmds.c | 13 +++++++++++--
 src/include/storage/proc.h       |  2 ++
 src/include/storage/procarray.h  |  6 +++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a3cb3cd47f..241c8a337e 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -393,7 +393,7 @@ 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_CIC,
 										  &n_old_snapshots);
 	if (progress)
 		pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +413,7 @@ 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_CIC,
 													&n_newer_snapshots);
 			for (j = i; j < n_old_snapshots; j++)
 			{
@@ -1420,6 +1420,9 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/* Set flag for other concurrent index builds to ignore us */
+	MyPgXact->vacuumFlags |= PROC_IN_CIC;
+
 	/*
 	 * The index is now visible, so we can report the OID.
 	 */
@@ -1482,6 +1485,9 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/* Set flag for other concurrent index builds to ignore us */
+	MyPgXact->vacuumFlags |= PROC_IN_CIC;
+
 	/*
 	 * Phase 3 of concurrent index build
 	 *
@@ -1541,6 +1547,9 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/* Set flag for other concurrent index builds to ignore us */
+	MyPgXact->vacuumFlags |= PROC_IN_CIC;
+
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyPgXact->xmin == InvalidTransactionId);
 
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index b20e2ad4f6..43c8ea3e31 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -53,6 +53,8 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_ANALYZE		0x04	/* currently running analyze */
+#define		PROC_IN_CIC			0x40	/* currently running CREATE INDEX
+										   CONCURRENTLY */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
 #define		PROC_IN_LOGICAL_DECODING	0x10	/* currently doing logical
 												 * decoding outside xact */
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index a5c7d0c064..0255949307 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -36,6 +36,9 @@
 
 #define		PROCARRAY_SLOTS_XMIN			0x20	/* replication slot xmin,
 													 * catalog_xmin */
+#define		PROCARRAY_CIC_FLAG				0x40	/* currently running CREATE INDEX
+													 * CONCURRENTLY */
+
 /*
  * Only flags in PROCARRAY_PROC_FLAGS_MASK are considered when matching
  * PGXACT->vacuumFlags. Other flags are used for different purposes and
@@ -43,7 +46,8 @@
  */
 #define		PROCARRAY_PROC_FLAGS_MASK	(PROCARRAY_VACUUM_FLAG | \
 										 PROCARRAY_ANALYZE_FLAG | \
-										 PROCARRAY_LOGICAL_DECODING_FLAG)
+										 PROCARRAY_LOGICAL_DECODING_FLAG | \
+										 PROCARRAY_CIC_FLAG)
 
 /* Use the following flags as an input "flags" to GetOldestXmin function */
 /* Consider all backends except for logical decoding ones which manage xmin separately */
-- 
2.20.1


--VS++wcV0S1rZb1Fb--