From: Michael Paquier <michael@paquier.xyz>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Andres Freund <andres@anarazel.de>
Cc: Ben Chobot <bench@silentmedia.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17401: REINDEX TABLE CONCURRENTLY creates a race condition on a streaming replica
Date: Thu, 21 Apr 2022 12:54:00 +0900
Message-ID: <YmDVWJt4WjRKFvAV@paquier.xyz> (raw)
In-Reply-To: <5ABDE9D8-D09C-4281-9F84-BF73A865A726@yandex-team.ru>
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>
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
Attachments:reindex-standby-fix.patch (text/x-diff, 1.7K) downloaddiff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.cindex cd30f15eba..704d8f3744 100644--- a/src/backend/commands/indexcmds.c+++ b/src/backend/commands/indexcmds.c@@ -475,6 +475,16 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
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();
}
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.cindex 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 lockmode, bool progress)
pgstat_progress_update_multi_param(3, index, values);
}
+ /*+ * 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 = lfirst(lc);++ if (locktag->locktag_type != LOCKTAG_RELATION)+ continue;++ LogAccessExclusiveLock(locktag->locktag_field1, /* dbId */+ locktag->locktag_field2); /* relId */+ }+
list_free_deep(holders);
}
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, x4mmm@yandex-team.ru, andres@anarazel.de, bench@silentmedia.com, pg@bowt.ie, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17401: REINDEX TABLE CONCURRENTLY creates a race condition on a streaming replica
In-Reply-To: <YmDVWJt4WjRKFvAV@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox