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
