Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 23 May 2022 14:40:16 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: =?utf-8?B?0J/QtdGC0YrRgCDQodC70LDQstC+0LI=?= <pet.slavov@gmail.com>,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17485: Records missing from Primary Key index when doing
 REINDEX INDEX CONCURRENTLY
Message-ID: <YoseQAkUMCEvlxhn@paquier.xyz>
References: <YoX5mCtugHdB5OET@paquier.xyz>
 <202205191203.zzmne46ulpbu@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="q0LGZ3lp2FWTbAbE"
Content-Disposition: inline
In-Reply-To: <202205191203.zzmne46ulpbu@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/YoseQAkUMCEvlxhn%40paquier.xyz>
Precedence: bulk


--q0LGZ3lp2FWTbAbE
Content-Type: multipart/mixed; boundary="35BBNfW5S3nrxXlk"
Content-Disposition: inline


--35BBNfW5S3nrxXlk
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, May 19, 2022 at 02:03:56PM +0200, Alvaro Herrera wrote:
> Hmm, yes, it's definitely possible that it is related.
>=20
> I'll have a look soon.

It took me some time to write a script to bisect that, but I have been
able to establish a correlation with d9d0762 that causes VACUUM to
ignore transactions doing some concurrent reindex operations.  I would
not be surprised to see that this is also related to some of the
reports we have seen lately with reindex operations.  There was one
with logical replication and missing records from a primary key, I
recall.

For the stable branches of 14 and 15, I would tend to play it safe and
revert d9d0762.  I have to admit that f9900df and c98763b stress me a
bit, and that we have not have anticipated all the ramifications of
this set of changes.  Thoughts?
--
Michael

--35BBNfW5S3nrxXlk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Revert-VACUUM-ignore-indexing-operations-with-CONCUR.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 8568ecb968b2a3a8b5b535546fee5bb1589db1d7 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 23 May 2022 14:25:22 +0900
Subject: [PATCH] Revert "VACUUM: ignore indexing operations with CONCURRENT=
LY"

This reverts commit d9d076222f5b94a85e0e318339cfc44b8f26022d.
---
 src/backend/storage/ipc/procarray.c | 41 ++++++-----------------------
 src/backend/utils/misc/guc.c        |  2 +-
 doc/src/sgml/ref/create_index.sgml  |  2 --
 doc/src/sgml/ref/reindex.sgml       |  2 --
 4 files changed, 9 insertions(+), 38 deletions(-)

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/=
procarray.c
index cd58c5faf0..21b468b5a8 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1659,13 +1659,7 @@ TransactionIdIsActive(TransactionId xid)
  * relations that's not required, since only backends in my own database c=
ould
  * ever see the tuples in them. Also, we can ignore concurrently running l=
azy
  * VACUUMs because (a) they must be working on other tables, and (b) they
- * don't need to do snapshot-based lookups.  Similarly, for the non-catalog
- * horizon, we can ignore CREATE INDEX CONCURRENTLY and REINDEX CONCURRENT=
LY
- * when they are working on non-partial, non-expressional indexes, for the
- * same reasons and because they can't run in transaction blocks.  (They a=
re
- * not possible to ignore for catalogs, because CIC and RC do some catalog
- * operations.)  Do note that this means that CIC and RC must use a lock l=
evel
- * that conflicts with VACUUM.
+ * don't need to do snapshot-based lookups.
  *
  * This also computes a horizon used to truncate pg_subtrans. For that
  * backends in all databases have to be considered, and concurrently runni=
ng
@@ -1715,6 +1709,9 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 	bool		in_recovery =3D RecoveryInProgress();
 	TransactionId *other_xids =3D ProcGlobal->xids;
=20
+	/* inferred after ProcArrayLock is released */
+	h->catalog_oldest_nonremovable =3D InvalidTransactionId;
+
 	LWLockAcquire(ProcArrayLock, LW_SHARED);
=20
 	h->latest_completed =3D ShmemVariableCache->latestCompletedXid;
@@ -1734,7 +1731,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
=20
 		h->oldest_considered_running =3D initial;
 		h->shared_oldest_nonremovable =3D initial;
-		h->catalog_oldest_nonremovable =3D initial;
 		h->data_oldest_nonremovable =3D initial;
=20
 		/*
@@ -1805,7 +1801,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		if (statusFlags & (PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING))
 			continue;
=20
-		/* shared tables need to take backends in all databases into account */
+		/* shared tables need to take backends in all database into account */
 		h->shared_oldest_nonremovable =3D
 			TransactionIdOlder(h->shared_oldest_nonremovable, xmin);
=20
@@ -1833,25 +1829,11 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 			(statusFlags & PROC_AFFECTS_ALL_HORIZONS) ||
 			in_recovery)
 		{
-			/*
-			 * We can ignore this backend if it's running CREATE INDEX
-			 * CONCURRENTLY or REINDEX CONCURRENTLY on a "safe" index -- but
-			 * only on vacuums of user-defined tables.
-			 */
-			if (!(statusFlags & PROC_IN_SAFE_IC))
-				h->data_oldest_nonremovable =3D
-					TransactionIdOlder(h->data_oldest_nonremovable, xmin);
-
-			/* Catalog tables need to consider all backends in this db */
-			h->catalog_oldest_nonremovable =3D
-				TransactionIdOlder(h->catalog_oldest_nonremovable, xmin);
+			h->data_oldest_nonremovable =3D
+				TransactionIdOlder(h->data_oldest_nonremovable, xmin);
 		}
 	}
=20
-	/* 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.
@@ -1873,8 +1855,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 			TransactionIdOlder(h->shared_oldest_nonremovable, kaxmin);
 		h->data_oldest_nonremovable =3D
 			TransactionIdOlder(h->data_oldest_nonremovable, kaxmin);
-		h->catalog_oldest_nonremovable =3D
-			TransactionIdOlder(h->catalog_oldest_nonremovable, kaxmin);
 		/* temp relations cannot be accessed in recovery */
 	}
 	else
@@ -1901,9 +1881,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		h->data_oldest_nonremovable =3D
 			TransactionIdRetreatedBy(h->data_oldest_nonremovable,
 									 vacuum_defer_cleanup_age);
-		h->catalog_oldest_nonremovable =3D
-			TransactionIdRetreatedBy(h->catalog_oldest_nonremovable,
-									 vacuum_defer_cleanup_age);
 		/* defer doesn't apply to temp relations */
 	}
=20
@@ -1926,9 +1903,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 	h->shared_oldest_nonremovable =3D
 		TransactionIdOlder(h->shared_oldest_nonremovable,
 						   h->slot_catalog_xmin);
-	h->catalog_oldest_nonremovable =3D
-		TransactionIdOlder(h->catalog_oldest_nonremovable,
-						   h->slot_xmin);
+	h->catalog_oldest_nonremovable =3D h->data_oldest_nonremovable;
 	h->catalog_oldest_nonremovable =3D
 		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 8e9b71375c..5478762cdb 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2740,7 +2740,7 @@ static struct config_int ConfigureNamesInt[] =3D
 			NULL
 		},
 		&vacuum_defer_cleanup_age,
-		0, 0, 1000000,			/* see ComputeXidHorizons */
+		0, 0, 1000000,
 		NULL, NULL, NULL
 	},
 	{
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_i=
ndex.sgml
index 907324b93e..9ffcdc629e 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -861,8 +861,6 @@ Indexes:
    Like any long-running transaction, <command>CREATE INDEX</command> on a
    table can affect which tuples can be removed by concurrent
    <command>VACUUM</command> on any other table.
-   Excepted from this are operations with the <literal>CONCURRENTLY</liter=
al>
-   option for indexes that are not partial and do not index any expression=
s.
   </para>
=20
   <para>
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 36cb4a455b..336ca24b31 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -475,8 +475,6 @@ Indexes:
     Like any long-running transaction, <command>REINDEX</command> on a tab=
le
     can affect which tuples can be removed by concurrent
     <command>VACUUM</command> on any other table.
-    Excepted from this are operations with the <literal>CONCURRENTLY</lite=
ral>
-    option for indexes that are not partial and do not index any expressio=
ns.
    </para>
=20
    <para>
--=20
2.36.1


--35BBNfW5S3nrxXlk--

--q0LGZ3lp2FWTbAbE
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmKLHkAACgkQnvQgOdby
QH1n7xAAmE/csp2zR9w6rUAZNGQfKuteotzSP7WUk9wNfXi7qPF1bmEXw/RqaCdr
/KfU93pR8xOqA5Xi3nmDhGw/ALnns9Tpu1TZYL4i6w4shGUyzbU90DigcixLjGAj
PNwImdMUQpWRqu023Nu2yYAM6cy/U5Cix5aS2QsutaJzOGtr4ehVfB4QQo9/IjtW
DwPa3dUJx9Djq8vquIMT7Zr3c+coiAvagwaCE9KFG8S5qyu6N3iyb5LHqvX+Pbp0
hr+ToSganVxq0Xl5iYJr4GnWfJx3bkiCziz5TQ97y5WCRxo97b67+SwKmF75W+e1
lOac9Ah0wH+CxHpnHcM4yD5ppXrDbDE1cgGz9U7acZSe12vOIeDBPwlPS7AeuiEz
O1v+zGY0qR5+QyawI4aZRPTcrQEhEAFjFkL/Z+grs5tQOaCpCEfYnV8L0Xfajj4N
FnArcHo6NzNNiFpmxQjU0HQwXUR8IJZaC3dfCw+6pejoZxpcyMHzhOQvrUwTtMa5
x4YHXvEC+HZAKFyStZThy3vuP/7db3ogQc76E1rd37ZE23OamyoV6M/uneYQiU0N
6nrD7JE8rBTofiFYcse+bGdnUw/Q6nIfiGfSt2vBkciIEfoc34hR8GbExANAKUMf
+xmEii4jlCpIIDTen9mWCf5NK5SO53VfrQQPXslX7P9hOpLba34=
=PJWK
-----END PGP SIGNATURE-----

--q0LGZ3lp2FWTbAbE--