Date: Thu, 21 Apr 2022 12:54:00 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Andres Freund <andres@anarazel.de>, Ben Chobot <bench@silentmedia.com>,
	Peter Geoghegan <pg@bowt.ie>,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17401: REINDEX TABLE CONCURRENTLY creates a race condition
 on a streaming replica
Message-ID: <YmDVWJt4WjRKFvAV@paquier.xyz>
References: <7625EC28-D102-450F-86CE-3D37095636F5@yandex-team.ru>
 <CAH2-WznyJT_TJ9gc=_YBkEZ5D1ddt-NfOWzbYURg4jq6QDFStw@mail.gmail.com>
 <C50E352E-FE90-46EC-94C2-3E96D9DDB141@yandex-team.ru>
 <bafcc4ab-8732-0503-ef17-0ee0420cf527@silentmedia.com>
 <04D2D137-102E-400D-B66B-C073C27A1327@yandex-team.ru>
 <20220211001240.47idtavhfyzi4aa3@alap3.anarazel.de>
 <AE561C32-E452-43D5-86EB-1CEB57405F95@yandex-team.ru>
 <29311A7B-240A-451A-9DC8-4909A0FEAE12@yandex-team.ru>
 <Yl+y7kvCDR6x4V+E@paquier.xyz>
 <5ABDE9D8-D09C-4281-9F84-BF73A865A726@yandex-team.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="HsZX7KLl5o+TpQIP"
Content-Disposition: inline
In-Reply-To: <5ABDE9D8-D09C-4281-9F84-BF73A865A726@yandex-team.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/YmDVWJt4WjRKFvAV%40paquier.xyz>
Precedence: bulk


--HsZX7KLl5o+TpQIP
Content-Type: multipart/mixed; boundary="UtCqPU03pz6sksL+"
Content-Disposition: inline


--UtCqPU03pz6sksL+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Apr 20, 2022 at 11:23:43PM +0500, Andrey Borodin wrote:
> Cool! Can you please share it?

Sure.  I am finishing for this part with the attached, that would be
backpatchable.  Based on the analysis of upthread, I have stuck with
logging standby snapshots once we are done in WaitForOlderSnapshots()
so as we make sure that a standby does not attempt to look at the data
of any running transactions it should wait for and the log of AELs at
the end of WaitForLockersMultiple() to avoid the access of the lockers
too early.  I have to admit that a direct use of
LogAccessExclusiveLock() feels a bit funky but we cannot use
LockAcquire() AFAIK, and that should be fine with all the lock tags at
hand.

I have stressed quite a lot a primary/standby setup with flavors of
the tests from Ben upthread, and I am not seeing the cache problems
anymore on the standby, with concurrent CREATE/DROP INDEX or REINDEX
sequences.

> I did not succeed with my tests on this way. But probably I was
> doing something wrong. Or, perhaps, these tests were encountering
> second problem.

There is a second thing going on with amcheck here.  REINDEX
CONCURRENTLY in isolation is working correctly when you run
bt_index_check() in parallel of it (aka attach gdb to the backend
process running REINDEX CONCURRENTLY and run bt_index_check()
repeatedly in parallel at the various steps with a \watch or a new
connection each time), but depending on your transaction order you may
finish with an invalid index given as input of
bt_index_check_internal(), roughly:
- REINDEX CONCURRENTLY begins, goes up to the transaction doing the
swap where the old index is marked as indisvalid.
- Execute bt_index_check(), that refers to the old index OID, still
valid at the moment where amcheck begins checking this index, before
the Relation of the parent table is opened.
- The transaction doing the swap commits, making visible that
indisvalid is false for the old index.
- bt_index_check() continues, opens the parent Relation, and complains
about an invalid index.

amcheck could be made more robust here, by calling
RelationGetIndexList() after opening the Relation of the parent to
check if it is still listed in the returned list as it would look at
the relcache and discard any invalid indexes on the way.  Returning an
error for an invalid index seems less helpful to me here than doing
nothing, particularly if you begin doing a full check of the indexes
based on the contents of pg_index.
--
Michael

--UtCqPU03pz6sksL+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-standby-fix.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexc=
mds.c
index cd30f15eba..704d8f3744 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -475,6 +475,16 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool pr=
ogress)
 		if (progress)
 			pgstat_progress_update_param(PROGRESS_WAITFOR_DONE, i + 1);
 	}
+
+	/*
+	 * Take a snapshot of running transactions and write this to WAL.  With
+	 * this information, a standby is able to know that it should not access
+	 * too soon any information we expect to wait for with a concurrent build.
+	 *
+	 * Skip the log of this information if disabled.
+	 */
+	if (XLogStandbyInfoActive())
+		LogStandbySnapshot();
 }
=20
=20
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmg=
r.c
index 1543da6162..81f0e7c88c 100644
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -974,6 +974,25 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmo=
de, bool progress)
 		pgstat_progress_update_multi_param(3, index, values);
 	}
=20
+	/*
+	 * Now that the wait is done, log a set of access exclusive locks on
+	 * the relation locktags we waited for.  This level of lock is higher
+	 * than what a standby should need to be able to look at the catalog
+	 * information of the objects involved.  This ensures to block any
+	 * attempt to the objects that it will until the transaction running
+	 * this routine is committed at recovery.
+	 */
+	foreach(lc, locktags)
+	{
+		LOCKTAG *locktag =3D lfirst(lc);
+
+		if (locktag->locktag_type !=3D LOCKTAG_RELATION)
+			continue;
+
+		LogAccessExclusiveLock(locktag->locktag_field1,		/* dbId */
+							   locktag->locktag_field2);	/* relId */
+	}
+
 	list_free_deep(holders);
 }
=20

--UtCqPU03pz6sksL+--

--HsZX7KLl5o+TpQIP
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJg1VgACgkQnvQgOdby
QH3Gag/+LGlWaAHGNpg436jWhqOGLnwTm34DqdRn77CD5cJ91tAkxkH5redyx9TF
DVYYXkmNz4Ps0yY8rFhdMsWa0Sb2JrFyDz2lAkzr3D2q2o17wcon2NUZzkwzVZDS
6MAfnCnuqGBxLviiLX9NkVknFedkEHtARbxi+dVLJwkKSWgOh2luLSJhCWCyEu1F
JVqGO9SscsJYK5kTdrXt02MhgNdT3oc1HuGzHoq59vCoe0mlmxeVQSXBZMzIs53M
Hys9ffuejPOfr8hYPS20B5WzgtFTOZMObGgxs2biC9yxrV8myyjjU7kQbVWv5FAX
gceh09P/+jvjsc/8kP0WyEsC3x186531GssdPUell1f+q1Hj7DvDgtx+qao633Xh
brvEdf4ax8B3RXf35Q/O45i7SB8E0kEx3s459OzGaSoCSMy5UFjtlg3dsy3owEtm
RohHQ/B9wFcZwri/gWf+boTDFjrLh0HL6leAvQFVVVb+dEvH2L5Eiiju1Bk3ETB5
92XjDW5rnLWcen6mudLR6SU1xqAfPDPSf9XqurYbUQ5z7TprJyVlxOLCGhRcLcvS
OEbPqi04HztuUAqtMwN4hrLBD10yKhAZkmvh+S7kC/4p9D7tRM2Uov7RO7Jq+sDt
LjEUJcekbmDr4xXJoiMupsh3OV3s9tknhNWWeeQmNkLVE2BIv3A=
=JCmG
-----END PGP SIGNATURE-----

--HsZX7KLl5o+TpQIP--