public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ashutosh Bapat <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: Maxim Orlov <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: wenhui qiu <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: POC: make mxidoff 64 bits
Date: Fri, 28 Nov 2025 18:47:24 +0530
Message-ID: <CAExHW5sg=0f9Tn8hoVM5t7OB3+4kgcRzxZw_Zw2AZ6dZGCTqPg@mail.gmail.com> (raw)
In-Reply-To: <CAExHW5t3kzJiVqmoqCLyGmfkTjD4Rwa27kXH-S_XvHWLkM2fzw@mail.gmail.com>
References: <CACG=ezaWg7_nt-8ey4aKv2w9LcuLthHknwCawmBgEeTnJrJTcw@mail.gmail.com>
<CACG=ezajc_Pcqmy6fcq-N8+LzCRMzOzJzez2_BgHEu-6RVJtKQ@mail.gmail.com>
<[email protected]>
<CACG=ezbKwypBp=14q9+hMQApus3=1hKxJ9x1+KinUhtT48570Q@mail.gmail.com>
<[email protected]>
<CACG=ezZwdvsijzuXE3hex3xHcoz75EQYBXRTsQJVwbo5J5sS3g@mail.gmail.com>
<CACG=ezbs912S58=uR17b4w8uuWv1=OcCRaTW_OWdFm4+tXZA6w@mail.gmail.com>
<CAGjGUA+BfcWyccNN4=tHsW_E-koRxbg8h8ut6hjvPsHMgmek6w@mail.gmail.com>
<CACG=ezYbYO_KHWdeDedbDcY0tOS0JfaqBxG3=bG5+DdsDK4MpQ@mail.gmail.com>
<CACG=ezYpZRPwoRCz_h3Qerd3XJNdpTHCpwGbZphNdy26tA4_qQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<CACG=ezYUJSvnuxntkURNWo_1vZ+AtmcQfqd_h6WgDzGaudfw+Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAExHW5tUEkiQrvm9hgccjKUNkWBnJ5_HDUrAwiHBTxu+Vuj29Q@mail.gmail.com>
<[email protected]>
<CAExHW5t3kzJiVqmoqCLyGmfkTjD4Rwa27kXH-S_XvHWLkM2fzw@mail.gmail.com>
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
view thread (79+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: POC: make mxidoff 64 bits
In-Reply-To: <CAExHW5sg=0f9Tn8hoVM5t7OB3+4kgcRzxZw_Zw2AZ6dZGCTqPg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox