Hi,
On 2019-04-08 15:34:46 +0300, Heikki Linnakangas wrote:
> The comments for relation_set_new_relfilenode() callback say that the AM can
> set *freezeXid and *minmulti to invalid. But when I did that, VACUUM hits
> this assertion:
>
> TRAP: FailedAssertion("!(((classForm->relfrozenxid) >= ((TransactionId)
> 3)))", File: "vacuum.c", Line: 1323)
Hm, that necessary change unfortunately escaped into the the zheap tree
(which indeed doesn't set relfrozenxid). That's why I'd not noticed
this. How about something like the attached?
I found a related problem in VACUUM FULL / CLUSTER while working on the
above, not fixed in the attached yet. Namely even if a relation doesn't
yet have a valid relfrozenxid/relminmxid before a VACUUM FULL / CLUSTER,
we'll set one after that. That's not great.
I suspect the easiest fix would be to to make the relevant
relation_copy_for_cluster() FreezeXid, MultiXactCutoff arguments into
pointer, and allow the AM to reset them to an invalid value if that's
the appropriate one.
It'd probably be better if we just moved the entire xid limit
computation into the AM, but I'm worried that we actually need to move
it *further up* instead - independent of this change. I don't think it's
quite right to allow a table with a toast table to be independently
VACUUM FULL/CLUSTERed from the toast table. GetOldestXmin() can go
backwards for a myriad of reasons (or limited by
old_snapshot_threshold), and I'm fairly certain that e.g. VACUUM FULLing
the toast table, setting a lower old_snapshot_threshold, and VACUUM
FULLing the main table would result in failures.
I think we need to fix this for 12, rather than wait for 13. Does
anybody disagree?
Greetings,
Andres Freund
>From 5c84256ea5e41055b0cb9e0dc121a4daaca43336 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Thu, 18 Apr 2019 13:20:31 -0700
Subject: [PATCH] Allow pg_class xid & multixid horizons to not be set.
This allows table AMs that don't need these horizons. This was already
documented in the tableam relation_set_new_filenode callback, but an
assert prevented if from actually working (the test AM contained the
ne necessary AM itself).
Reported-By: Heikki Linnakangas
Author: Andres Freund
Discussion: https://postgr.es/m/9a7fb9cc-2419-5db7-8840-ddc10c93f...@iki.fi
---
src/backend/access/heap/vacuumlazy.c | 4 +++
src/backend/commands/vacuum.c | 53 ++++++++++++++++++++--------
src/backend/postmaster/autovacuum.c | 4 +--
3 files changed, 45 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 8dc76fa8583..9364cd4c33f 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -213,6 +213,10 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
Assert(params != NULL);
Assert(params->index_cleanup != VACOPT_TERNARY_DEFAULT);
+ /* not every AM requires these to be valid, but heap does */
+ Assert(TransactionIdIsNormal(onerel->rd_rel->relfrozenxid));
+ Assert(MultiXactIdIsValid(onerel->rd_rel->relminmxid));
+
/* measure elapsed time iff autovacuum logging requires it */
if (IsAutoVacuumWorkerProcess() && params->log_min_duration >= 0)
{
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 1a7291d94bc..94fb6f26063 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1313,36 +1313,61 @@ vac_update_datfrozenxid(void)
/*
* Only consider relations able to hold unfrozen XIDs (anything else
- * should have InvalidTransactionId in relfrozenxid anyway.)
+ * should have InvalidTransactionId in relfrozenxid anyway).
*/
if (classForm->relkind != RELKIND_RELATION &&
classForm->relkind != RELKIND_MATVIEW &&
classForm->relkind != RELKIND_TOASTVALUE)
+ {
+ Assert(!TransactionIdIsValid(classForm->relfrozenxid));
+ Assert(!MultiXactIdIsValid(classForm->relminmxid));
continue;
-
- Assert(TransactionIdIsNormal(classForm->relfrozenxid));
- Assert(MultiXactIdIsValid(classForm->relminmxid));
+ }
/*
+ * Some table AMs might not need per-relation xid / multixid
+ * horizons. It therefore seems reasonable to allow relfrozenxid and
+ * relminmxid to not be set (i.e. set to their respective Invalid*Id)
+ * independently. Thus validate and compute horizon for each only if
+ * set.
+ *
* If things are working properly, no relation should have a
* relfrozenxid or relminmxid that is "in the future". However, such
* cases have been known to arise due to bugs in pg_upgrade. If we
* see any entries that are "in the future", chicken out and don't do
- * anything. This ensures we won't truncate clog before those
- * relations have been scanned and cleaned up.
+ * anything. This ensures we won't truncate clog & multixact SLRUs
+ * before those relations have been scanned and cleaned up.
*/
- if (TransactionIdPrecedes(lastSaneFrozenXid, classForm->relfrozenxid) ||
- MultiXactIdPrecedes(lastSaneMinMulti, classForm->relminmxid))
+
+ if (TransactionIdIsValid(classForm->relfrozenxid))
{
- bogus = true;
- break;
+ Assert(TransactionIdIsNormal(classForm->relfrozenxid));
+
+ /* check for values in the future */
+ if (TransactionIdPrecedes(lastSaneFrozenXid, classForm->relfrozenxid))
+ {
+ bogus = true;
+ break;
+ }
+
+ /* determine new horizon */
+ if (TransactionIdPrecedes(classForm->relfrozenxid, newFrozenXid))
+ newFrozenXid = classForm->relfrozenxid;
}
- if (TransactionIdPrecedes(classForm->relfrozenxid, newFrozenXid))
- newFrozenXid = classForm->relfrozenxid;
+ if (MultiXactIdIsValid(classForm->relminmxid))
+ {
+ /* check for values in the future */
+ if (MultiXactIdPrecedes(lastSaneMinMulti, classForm->relminmxid))
+ {
+ bogus = true;
+ break;
+ }
- if (MultiXactIdPrecedes(classForm->relminmxid, newMinMulti))
- newMinMulti = classForm->relminmxid;
+ /* determine new horizon */
+ if (MultiXactIdPrecedes(classForm->relminmxid, newMinMulti))
+ newMinMulti = classForm->relminmxid;
+ }
}
/* we're done with pg_class */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 0976029e737..53c91d92778 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -3033,8 +3033,8 @@ relation_needs_vacanalyze(Oid relid,
multiForceLimit = recentMulti - multixact_freeze_max_age;
if (multiForceLimit < FirstMultiXactId)
multiForceLimit -= FirstMultiXactId;
- force_vacuum = MultiXactIdPrecedes(classForm->relminmxid,
- multiForceLimit);
+ force_vacuum = MultiXactIdIsValid(classForm->relminmxid) &&
+ MultiXactIdPrecedes(classForm->relminmxid, multiForceLimit);
}
*wraparound = force_vacuum;
--
2.21.0.dirty
