On Wed, 9 Jun 2021 at 04:42, Michael Paquier <mich...@paquier.xyz> wrote: > > On Tue, Jun 08, 2021 at 05:47:28PM -0700, Peter Geoghegan wrote: > > I don't have time to try this out myself today, but offhand I'm pretty > > confident that this is sufficient to reproduce the underlying bug > > itself. And if that's true then I guess it can't have anything to do > > with the pg_upgrade/pg_resetwal issue Tom just referenced, despite the > > apparent similarity. > > Agreed. It took me a couple of minutes to get autovacuum to run in an > infinite loop with a standalone instance. Nice catch, Justin!
I believe that I've found the culprit: GetOldestNonRemovableTransactionId(rel) does not use the exact same conditions for returning OldestXmin as GlobalVisTestFor(rel) does. This results in different minimal XIDs, and subsequently this failure. The attached patch fixes this inconsistency, and adds a set of asserts to ensure that GetOldesNonRemovableTransactionId is equal to the maybe_needed of the GlobalVisTest of that relation, plus some at GlobalVisUpdateApply such that it will fail whenever it is called with arguments that would move the horizons in the wrong direction. Note that there was no problem in GlobalVisUpdateApply, but it helped me determine that that part was not the source of the problem, and I think that having this safeguard is a net-positive. Another approach might be changing GlobalVisTestFor(rel) instead to reflect the conditions in GetOldestNonRemovableTransactionId. With attached prototype patch, I was unable to reproduce the problematic case in 10 minutes. Without, I got the problematic behaviour in seconds. With regards, Matthias
From fe5cb0430a06a44823d5b62f2f909da624505962 Mon Sep 17 00:00:00 2001 From: Matthias van de Meent <boekew...@gmail.com> Date: Wed, 9 Jun 2021 17:26:34 +0200 Subject: [PATCH v1] Fix a bug in GetOldestNonRemovableTransactionId GetOldestNonRemovableTransactionId(rel) did not return values consistent with GlobalVisTestFor(rel). This is now updated, and some assertions are added to ensure this problem case does not return. --- src/backend/storage/ipc/procarray.c | 39 +++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 42a89fc5dc..7177d50351 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -1944,18 +1944,25 @@ TransactionId GetOldestNonRemovableTransactionId(Relation rel) { ComputeXidHorizonsResult horizons; + TransactionId result; ComputeXidHorizons(&horizons); - /* select horizon appropriate for relation */ - if (rel == NULL || rel->rd_rel->relisshared) - return horizons.shared_oldest_nonremovable; - else if (RelationIsAccessibleInLogicalDecoding(rel)) - return horizons.catalog_oldest_nonremovable; + /* select horizon appropriate for relation. Mirrored from GlobalVisTestFor */ + if (rel == NULL || rel->rd_rel->relisshared || RecoveryInProgress()) + result = horizons.shared_oldest_nonremovable; + else if (IsCatalogRelation(rel) || RelationIsAccessibleInLogicalDecoding(rel)) + result = horizons.catalog_oldest_nonremovable; else if (RELATION_IS_LOCAL(rel)) - return horizons.temp_oldest_nonremovable; + result = horizons.temp_oldest_nonremovable; else - return horizons.data_oldest_nonremovable; + result = horizons.data_oldest_nonremovable; + + /* Sanity check */ + Assert(TransactionIdEquals( + XidFromFullTransactionId(GlobalVisTestFor(rel)->maybe_needed), + result)); + return result; } /* @@ -4032,6 +4039,24 @@ GlobalVisTestShouldUpdate(GlobalVisState *state) static void GlobalVisUpdateApply(ComputeXidHorizonsResult *horizons) { + /* assert non-decreasing nature of horizons */ + Assert(FullTransactionIdFollowsOrEquals( + FullXidRelativeTo(horizons->latest_completed, + horizons->shared_oldest_nonremovable), + GlobalVisSharedRels.maybe_needed)); + Assert(FullTransactionIdFollowsOrEquals( + FullXidRelativeTo(horizons->latest_completed, + horizons->catalog_oldest_nonremovable), + GlobalVisCatalogRels.maybe_needed)); + Assert(FullTransactionIdFollowsOrEquals( + FullXidRelativeTo(horizons->latest_completed, + horizons->data_oldest_nonremovable), + GlobalVisDataRels.maybe_needed)); + Assert(FullTransactionIdFollowsOrEquals( + FullXidRelativeTo(horizons->latest_completed, + horizons->temp_oldest_nonremovable), + GlobalVisTempRels.maybe_needed)); + GlobalVisSharedRels.maybe_needed = FullXidRelativeTo(horizons->latest_completed, horizons->shared_oldest_nonremovable); -- 2.20.1