On Thu, Sep 19, 2024 at 05:35:33PM -0400, Tom Lane wrote:
> So the fix seems clear to me: RestoreRelationMap needs to happen
> before anything that could result in catalog lookups. I'm kind
> of inclined to move up the adjacent restores of non-transactional
> low-level stuff too, particularly RestoreReindexState which has
> direct impact on how catalog lookups are done.
Thanks for debugging that. RestorePendingSyncs() also changes what
RelationInitPhysicalAddr() puts in the relcache entry, so it needs to stay
with RestoreRelationMap(). I'm attaching the fix I have in mind. Since the
above (commit 126ec0b), the following has been getting an
AssertPendingSyncConsistency() trap:
make check-tests TESTS=reindex_catalog
PG_TEST_INITDB_EXTRA_OPTS='-cwal_level=minimal -cmax_wal_senders=0'
In commit range [6e086fa,126ec0b^], that failed differently, reflecting the
outdated relmapper. (6e086fa is mostly innocent here, though.)
I looked for ways we'd regret not back-patching commit 126ec0b and this, but I
didn't find a concrete problem. After InvalidateSystemCaches(), the
back-branch parallel worker relcache contains the nailed relations. Each
entry then has:
- rd_locator possibly outdated
- rd_firstRelfilelocatorSubid==0 (correct for rd_locator)
- rd_isvalid==false, from RelationReloadNailed()
>From code comments, I gather RelationCacheInitializePhase2() is the latest we
use an rd_locator without first making the relcache entry rd_isvalid=true. If
that's right, so long as no catalogs get rd_isvalid=true between
InvalidateSystemCaches() and RestorePendingSyncs(), we're okay without a
back-patch.
I was going to add check-world coverage of this case, but I stopped in light
of the tricky deadlocks that led to commit fe4d022 disabling the
reindex_catalog test. check-world does reach it, in inplace.spec, if one runs
with both wal_level=minimal and debug_parallel_query=regress. (inplace.spec
may eventually fail with the same deadlocks. I've not heard of it deadlocking
so far, and being a NO_INSTALLCHECK test helps.)
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
Fix parallel worker tracking of new catalog relfilenumbers.
Reunite RestorePendingSyncs() with RestoreRelationMap(). If
RelationInitPhysicalAddr() ran after RestoreRelationMap() but before
RestorePendingSyncs(), the relcache entry could cause RelationNeedsWAL()
to return true erroneously. Trouble required commands of the current
transaction to include REINDEX or CLUSTER of a system catalog. The
parallel leader correctly derived RelationNeedsWAL()==false from the new
relfilenumber, but the worker saw RelationNeedsWAL()==true. Worker
MarkBufferDirtyHint() then wrote unwanted WAL. Recovery of that
unwanted WAL could lose tuples like the system could before commit
c6b92041d38512a4176ed76ad06f713d2e6c01a8 introduced this tracking.
RestorePendingSyncs() and RestoreRelationMap() were adjacent till commit
126ec0bc76d044d3a9eb86538b61242bf7da6db4, so no back-patch for now.
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/backend/access/transam/parallel.c
b/src/backend/access/transam/parallel.c
index d4e84aa..4a2e352 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1421,17 +1421,18 @@ ParallelWorkerMain(Datum main_arg)
StartParallelWorkerTransaction(tstatespace);
/*
- * Restore relmapper and reindex state early, since these affect catalog
- * access. Ideally we'd do this even before calling InitPostgres, but
- * that has order-of-initialization problems, and also the relmapper
would
- * get confused during the CommitTransactionCommand call above.
+ * Restore state that affects catalog access. Ideally we'd do this even
+ * before calling InitPostgres, but that has order-of-initialization
+ * problems, and also the relmapper would get confused during the
+ * CommitTransactionCommand call above.
*/
+ pendingsyncsspace = shm_toc_lookup(toc, PARALLEL_KEY_PENDING_SYNCS,
+
false);
+ RestorePendingSyncs(pendingsyncsspace);
relmapperspace = shm_toc_lookup(toc, PARALLEL_KEY_RELMAPPER_STATE,
false);
RestoreRelationMap(relmapperspace);
reindexspace = shm_toc_lookup(toc, PARALLEL_KEY_REINDEX_STATE, false);
RestoreReindexState(reindexspace);
-
- /* Restore combo CID state. */
combocidspace = shm_toc_lookup(toc, PARALLEL_KEY_COMBO_CID, false);
RestoreComboCIDState(combocidspace);
@@ -1488,11 +1489,6 @@ ParallelWorkerMain(Datum main_arg)
SetTempNamespaceState(fps->temp_namespace_id,
fps->temp_toast_namespace_id);
- /* Restore pending syncs. */
- pendingsyncsspace = shm_toc_lookup(toc, PARALLEL_KEY_PENDING_SYNCS,
-
false);
- RestorePendingSyncs(pendingsyncsspace);
-
/* Restore uncommitted enums. */
uncommittedenumsspace = shm_toc_lookup(toc,
PARALLEL_KEY_UNCOMMITTEDENUMS,
false);
