Date: Mon, 22 Feb 2021 12:56:42 -0300
From: =?iso-8859-1?Q?=C1lvaro?= Herrera <alvherre@alvh.no-ip.org>
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
	Hamid Akhtar <hamid.akhtar@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: {CREATE INDEX, REINDEX} CONCURRENTLY improvements
Message-ID: <20210222155642.GA26534@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="envbJBWh7q8WU6mo"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210118202734.GA25753@alvherre.pgsql>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


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

Here's an updated patch.

I haven't added commentary or documentation to account for the new
assumption, per Matthias' comment and Robert's discussion thereof.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat)

--envbJBWh7q8WU6mo
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-Teach-VACUUM-to-ignore-CIC.patch"

From 4e1cf2ee0e6c9fb49fc2bc5932d3a59d65ee9ea7 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 15 Jan 2021 14:03:14 -0300
Subject: [PATCH v2] Teach VACUUM to ignore CIC

Discussion: https://postgr.es/m/20210115133858.GA18931@alvherre.pgsql
---
 src/backend/storage/ipc/procarray.c | 40 +++++++++++++++++++++++------
 src/backend/utils/misc/guc.c        |  2 +-
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index a5daea8957..88b8271c22 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1610,7 +1610,12 @@ TransactionIdIsActive(TransactionId xid)
  * relations that's not required, since only backends in my own database could
  * ever see the tuples in them. Also, we can ignore concurrently running lazy
  * VACUUMs because (a) they must be working on other tables, and (b) they
- * don't need to do snapshot-based lookups.
+ * don't need to do snapshot-based lookups.  Also, for the non-catalog
+ * horizon, we can ignore CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY
+ * when they are working on non-partial, non-expressional indexes, for the
+ * same reasons and because they can't run in transaction blocks.  (They are
+ * not possible to ignore for catalogs, because CIC and RC do some catalog
+ * operations.)
  *
  * This also computes a horizon used to truncate pg_subtrans. For that
  * backends in all databases have to be considered, and concurrently running
@@ -1660,9 +1665,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 	bool		in_recovery = RecoveryInProgress();
 	TransactionId *other_xids = ProcGlobal->xids;
 
-	/* inferred after ProcArrayLock is released */
-	h->catalog_oldest_nonremovable = InvalidTransactionId;
-
 	LWLockAcquire(ProcArrayLock, LW_SHARED);
 
 	h->latest_completed = ShmemVariableCache->latestCompletedXid;
@@ -1682,6 +1684,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 
 		h->oldest_considered_running = initial;
 		h->shared_oldest_nonremovable = initial;
+		h->catalog_oldest_nonremovable = initial;
 		h->data_oldest_nonremovable = initial;
 
 		/*
@@ -1752,7 +1755,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		if (statusFlags & (PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING))
 			continue;
 
-		/* shared tables need to take backends in all database into account */
+		/* shared tables need to take backends in all databases into account */
 		h->shared_oldest_nonremovable =
 			TransactionIdOlder(h->shared_oldest_nonremovable, xmin);
 
@@ -1768,16 +1771,33 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		 * to prune still needed data away). If the current backend never
 		 * connects to a database that is harmless, because
 		 * data_oldest_nonremovable will never be utilized.
+		 *
+		 * Additionally, processes doing CREATE INDEX CONCURRENTLY and REINDEX
+		 * CONCURRENTLY on "safe" indexes can be ignored for non-catalog
+		 * horizon. (But not for catalogs: some transactions in CIC/RC do
+		 * catalog updates.)
 		 */
 		if (in_recovery ||
 			MyDatabaseId == InvalidOid || proc->databaseId == MyDatabaseId ||
 			proc->databaseId == 0)	/* always include WalSender */
 		{
-			h->data_oldest_nonremovable =
-				TransactionIdOlder(h->data_oldest_nonremovable, xmin);
+			if (statusFlags & PROC_IN_SAFE_IC)
+				h->catalog_oldest_nonremovable =
+					TransactionIdOlder(h->catalog_oldest_nonremovable, xmin);
+			else
+			{
+				h->data_oldest_nonremovable =
+					TransactionIdOlder(h->data_oldest_nonremovable, xmin);
+				h->catalog_oldest_nonremovable =
+					TransactionIdOlder(h->catalog_oldest_nonremovable, xmin);
+			}
 		}
 	}
 
+	/* catalog horizon should never be later than data */
+	Assert(TransactionIdPrecedesOrEquals(h->catalog_oldest_nonremovable,
+										 h->data_oldest_nonremovable));
+
 	/*
 	 * If in recovery fetch oldest xid in KnownAssignedXids, will be applied
 	 * after lock is released.
@@ -1799,6 +1819,8 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 			TransactionIdOlder(h->shared_oldest_nonremovable, kaxmin);
 		h->data_oldest_nonremovable =
 			TransactionIdOlder(h->data_oldest_nonremovable, kaxmin);
+		h->catalog_oldest_nonremovable =
+			TransactionIdOlder(h->catalog_oldest_nonremovable, kaxmin);
 		/* temp relations cannot be accessed in recovery */
 	}
 	else
@@ -1825,6 +1847,9 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		h->data_oldest_nonremovable =
 			TransactionIdRetreatedBy(h->data_oldest_nonremovable,
 									 vacuum_defer_cleanup_age);
+		h->catalog_oldest_nonremovable =
+			TransactionIdRetreatedBy(h->catalog_oldest_nonremovable,
+									 vacuum_defer_cleanup_age);
 		/* defer doesn't apply to temp relations */
 	}
 
@@ -1847,7 +1872,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 	h->shared_oldest_nonremovable =
 		TransactionIdOlder(h->shared_oldest_nonremovable,
 						   h->slot_catalog_xmin);
-	h->catalog_oldest_nonremovable = h->data_oldest_nonremovable;
 	h->catalog_oldest_nonremovable =
 		TransactionIdOlder(h->catalog_oldest_nonremovable,
 						   h->slot_catalog_xmin);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 260ec7b97e..d626731723 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2583,7 +2583,7 @@ static struct config_int ConfigureNamesInt[] =
 			NULL
 		},
 		&vacuum_defer_cleanup_age,
-		0, 0, 1000000,
+		0, 0, 1000000,		/* see ComputeXidHorizons */
 		NULL, NULL, NULL
 	},
 
-- 
2.20.1


--envbJBWh7q8WU6mo--