On Fri, Nov 28, 2025 at 6:35 PM Ashutosh Bapat <[email protected]> wrote: > > On Fri, Nov 21, 2025 at 7:26 PM Heikki Linnakangas <[email protected]> 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.
One more thing, An UPDATE waits for FOR SHARE query to finish, and vice versa. In my experiments I didn't see an UPDATE creating a multi-xact. Why do we have UPDATEs in the load created by the test? Am I missing something? -- Best Wishes, Ashutosh Bapat
