On Thu, 4 Dec 2025 at 13:39, Heikki Linnakangas <[email protected]> wrote:
> However, that doesn't hold for pg_upgrade. pg_upgrade will try to read > all the multixids. So we need to make the multixact reading code > tolerant of the situations that could be present after a crash. I think > the right philosophy here is that we try to read all the old multixids, > and do our best to interpret them the same way that the old server > would. Something like attached? Now previous scheme of upgrade with the bytes joggling start looking not so bad. Just a funny thought that came to my mind. Perhaps we should check that all the files exist and have the correct sizes in the pre-check stage Not sure about it. Because SLRU does not support "holes", simply checking if the first and last multixacts exist will be enough. But we'll do it anyway in a real conversion. PFA to start a conversation. -- Best regards, Maxim Orlov.
From c2ccb107bef898420e6417c37d56c6b30578d28f Mon Sep 17 00:00:00 2001 From: Maxim Orlov <[email protected]> Date: Thu, 4 Dec 2025 17:02:35 +0300 Subject: [PATCH] rough draft of skipping bogus offsets --- src/bin/pg_upgrade/multixact_old.c | 38 ++++++++++++++++++++++++------ src/bin/pg_upgrade/multixact_old.h | 2 +- src/bin/pg_upgrade/pg_upgrade.c | 15 ++++++------ 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/bin/pg_upgrade/multixact_old.c b/src/bin/pg_upgrade/multixact_old.c index 529eeeb93b6..685bfaeff82 100644 --- a/src/bin/pg_upgrade/multixact_old.c +++ b/src/bin/pg_upgrade/multixact_old.c @@ -136,7 +136,7 @@ AllocOldMultiXactRead(char *pgdata, MultiXactId nextMulti, * - Because there's no concurrent activity, We don't need to worry about * locking and some corner cases. */ -void +bool GetOldMultiXactIdSingleMember(OldMultiXactReader *state, MultiXactId multi, TransactionId *result, MultiXactStatus *status) { @@ -189,7 +189,18 @@ GetOldMultiXactIdSingleMember(OldMultiXactReader *state, MultiXactId multi, offptr += entryno; offset = *offptr; - Assert(offset != 0); + if (offset == 0) + { + pg_log(PG_WARNING, "multixact %u, offset is empty", multi); + return false; + } +#if 0 + if ( <more checks> ) + { + pg_log(PG_WARNING, "multixact %u, offset is bogus", multi); + return false; + } +#endif /* * Use the same increment rule as GetNewMultiXactId(), that is, don't @@ -224,9 +235,13 @@ GetOldMultiXactIdSingleMember(OldMultiXactReader *state, MultiXactId multi, /* * Corner case 2: next multixact is still being filled in, this cannot - * happen during upgrade. + * happen during upgrade, but if it does, complain. */ - Assert(nextMXOffset != 0); + if (nextMXOffset == 0) + { + pg_log(PG_WARNING, "multixact next to %u is empty", multi); + return false; + } length = nextMXOffset - offset; } @@ -272,8 +287,11 @@ GetOldMultiXactIdSingleMember(OldMultiXactReader *state, MultiXactId multi, { /* sanity check */ if (result_isupdate) - pg_fatal("multixact %u has more than one updating member", - multi); + { + pg_log(PG_WARNING, + "multixact %u has more than one updating member", multi); + return false; + } result_xid = *xactptr; result_isupdate = true; } @@ -282,11 +300,17 @@ GetOldMultiXactIdSingleMember(OldMultiXactReader *state, MultiXactId multi, } /* A multixid with zero members should not happen */ - Assert(TransactionIdIsValid(result_xid)); + if (!TransactionIdIsValid(result_xid)) + { + pg_log(PG_WARNING, "multixact %u have zero members", multi); + return false; + } *result = result_xid; *status = result_isupdate ? MultiXactStatusUpdate : MultiXactStatusForKeyShare; + + return true; } /* diff --git a/src/bin/pg_upgrade/multixact_old.h b/src/bin/pg_upgrade/multixact_old.h index 4f9e086a1fb..b7352159d83 100644 --- a/src/bin/pg_upgrade/multixact_old.h +++ b/src/bin/pg_upgrade/multixact_old.h @@ -29,7 +29,7 @@ typedef struct OldMultiXactReader extern OldMultiXactReader *AllocOldMultiXactRead(char *pgdata, MultiXactId nextMulti, OldMultiXactOffset nextOffset); -extern void GetOldMultiXactIdSingleMember(OldMultiXactReader *state, +extern bool GetOldMultiXactIdSingleMember(OldMultiXactReader *state, MultiXactId multi, TransactionId *result, MultiXactStatus *status); diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index ff937b9e104..c5da56fe785 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -832,14 +832,15 @@ convert_multixacts(MultiXactId from_multi, MultiXactId to_multi) * one updating one, or if there was no update, arbitrarily the * first locking xid. */ - GetOldMultiXactIdSingleMember(old_reader, multi, &xid, &status); + if (GetOldMultiXactIdSingleMember(old_reader, multi, &xid, &status)) + { + /* Write it out in new format */ + member.xid = xid; + member.status = status; + RecordNewMultiXact(new_writer, next_offset, multi, 1, &member); + next_offset += 1; + } - /* Write it out in new format */ - member.xid = xid; - member.status = status; - RecordNewMultiXact(new_writer, next_offset, multi, 1, &member); - - next_offset += 1; multi++; /* handle wraparound */ if (multi < FirstMultiXactId) -- 2.43.0
From 0ac8eb292c21a06da31215aa41adb53ec1f90872 Mon Sep 17 00:00:00 2001 From: Maxim Orlov <[email protected]> Date: Thu, 4 Dec 2025 18:31:37 +0300 Subject: [PATCH 2/2] Check is first and last multis exists --- src/bin/pg_upgrade/multixact_old.c | 10 ++++++++++ src/bin/pg_upgrade/multixact_old.h | 2 ++ src/bin/pg_upgrade/pg_upgrade.c | 23 +++++++++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/src/bin/pg_upgrade/multixact_old.c b/src/bin/pg_upgrade/multixact_old.c index 685bfaeff82..ffd06ad908f 100644 --- a/src/bin/pg_upgrade/multixact_old.c +++ b/src/bin/pg_upgrade/multixact_old.c @@ -324,3 +324,13 @@ FreeOldMultiXactReader(OldMultiXactReader *state) pfree(state); } + +void +CheckOldMultiXactIdExist(OldMultiXactReader *state, MultiXactId multi) +{ + int64 pageno = MultiXactIdToOffsetPage(multi); + char *buf = SlruReadSwitchPage(state->offset, pageno); + + if (!buf) + pg_fatal("could not read multixact %u", multi); +} diff --git a/src/bin/pg_upgrade/multixact_old.h b/src/bin/pg_upgrade/multixact_old.h index b7352159d83..86141ac392f 100644 --- a/src/bin/pg_upgrade/multixact_old.h +++ b/src/bin/pg_upgrade/multixact_old.h @@ -34,5 +34,7 @@ extern bool GetOldMultiXactIdSingleMember(OldMultiXactReader *state, TransactionId *result, MultiXactStatus *status); extern void FreeOldMultiXactReader(OldMultiXactReader *reader); +extern void CheckOldMultiXactIdExist(OldMultiXactReader *state, + MultiXactId multi); #endif /* MULTIXACT_OLD_H */ diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index c5da56fe785..647e05f350a 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -772,6 +772,21 @@ copy_subdir_files(const char *old_subdir, const char *new_subdir) check_ok(); } +static void +check_multixacts(MultiXactId from_multi, MultiXactId to_multi) +{ + OldMultiXactReader *reader; + + reader = AllocOldMultiXactRead(old_cluster.pgdata, + old_cluster.controldata.chkpnt_nxtmulti, + old_cluster.controldata.chkpnt_nxtmxoff); + + CheckOldMultiXactIdExist(reader, from_multi); + CheckOldMultiXactIdExist(reader, to_multi); + + FreeOldMultiXactReader(reader); +} + /* * Convert pg_multixact/offset and /members from the old format with 32-bit * offsets. @@ -958,6 +973,14 @@ copy_xact_xlog_xid(void) remove_new_subdir("pg_multixact/members", false); remove_new_subdir("pg_multixact/offsets", false); + /* + * Before the actual conversion do sanity check. + * XXX: place it properly, it should be better place for this + */ + prep_status("Sanity check pg_multixact files"); + check_multixacts(oldstMulti, nxtmulti); + check_ok(); + /* * Create new pg_multixact files, converting old ones if needed. */ -- 2.43.0
