Re: POC: make mxidoff 64 bits

On Fri, Nov 21, 2025 at 7:26 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
>
> > I have reviewed patch 0002 and multxact.c changes in 0003. So far I
> > have only these comments. I will review the pg_upgrade.c changes next.

007_multixact_conversion.pl fires thousands of queries through
BackgroundPsql which prints debug output for each of the queries. When
running this file with oldinstall set,
2.2M regress_log_007_multixact_conversion (size of file)
77874 regress_log_007_multixact_conversion (wc -l output)

Since this output is also copied in testlog.txt, the effect is two-fold.

Most, if not all, of this output is useless. It also makes it hard to
find the output we are looking for. PFA patch which reduces this
output. The patch adds a flag verbose to query_safe() and query() to
toggle this output. With the patch the sizes are
27K regress_log_007_multixact_conversion
588 regress_log_007_multixact_conversion

And it makes the test faster by about a second or two on my laptop.
Something on those lines or other is required to reduce the output
from query_safe().

Some more comments
+++ b/src/bin/pg_upgrade/multixact_old.c

We may need to introduce new _new and then _old will become _older.
Should we rename the files to have pre19 and post19 or some similar
suffixes which make it clear what is meant by old and new?

+
+static inline int64
+MultiXactIdToOffsetPage(MultiXactId multi)

The prologue mentions that the definitions are copy-pasted from
multixact.c from version 18, but they share the names with functions
in the current version. I think that's going to be a good source of
confusion especially in a file which is a few hundred lines long. Can
we rename them to have "Old" prefix or something similar?

+
+# Dump contents of the 'mxofftest' table, created by mxact_workload
+sub get_dump_for_comparison

This local function shares its name with a local function in
002_pg_upgrade.pl. Better to use a separate name. Also it's not
"dumping" data using "pg_dump", so "dump" in the name can be
misleading.

+ $newnode->start;
+ my $new_dump = get_dump_for_comparison($newnode, "newnode_${tag}_dump");
+ $newnode->stop;

There is no code which actually looks at the multixact offsets here to
make sure that the conversion happened correctly. I guess the test
relies on visibility checks for that. Anyway, we need a comment
explaining why just comparing the contents of the table is enough to
ensure correct conversion. Better if we can add an explicit test that
the offsets were converted correctly. I don't have any idea of how to
do that right now, though. Maybe use pg_get_multixact_members()
somehow in the query to extract data out of the table?

+
+ compare_files($old_dump, $new_dump,
+ 'dump outputs from original and restored regression databases match');

A shared test name too :); but there is not regression database here.

+
+ note ">>> case #${tag}\n"
+ . " oldnode mxoff from ${start_mxoff} to ${finish_mxoff}\n"
+ . " newnode mxoff ${new_next_mxoff}\n";

Should we check that some condition holds between finish_mxoff and
new_next_mxoff?

I will continue reviewing it further.

--
Best Wishes,
Ashutosh Bapat