Re: POC: make mxidoff 64 bits

Thanks for working on this!

On 19/11/2024 19:53, Maxim Orlov wrote:
> The test itself is in src/bin/pg_upgrade/t/005_offset.pl
> <http://005_offset.pl> It is rather heavy and took about 45 minutes on
> my i5 with 2.7 Gb data generated. Basically, each test here is creating
> a cluster and fill it with multixacts. Thus, dozens of segments are
> created using two methods. One is with prepared transactions, and it
> creates, roughly, the same amount of segments for members and for
> offsets. The other one is based on Heikki's multixids.py and creates
> more members than offsets. I've used both of these methods to generate
> as much diverse data as possible.
>
> Here is how I test this patch set:
>
> 1. You need two pg clusters: the "old" one, i.e. without patch set, and
> the "new" with patch set v9 applied.
> 2. Apply v9-0005-TEST-initdb-option-to-initialize-cluster-with-
> non.patch.txt to the "old" and "new" clusters. Note, this is only
> patch required for "old" cluster. This will allow you to create a
> cluster with non-standard initial multixact and multixact offset.
> Unfortunately, this patch was not did not arouse public interest
> since it is assumed that there is similar functionality to the
> pg_resetwal utility. But similar is not mean equal. See, pg_resetwal
> must be used after cluster init, thus, we step into some problems
> with vacuum and some SLRU segments must be filled with zeroes. Also,
> template0 datminmxidmust be manually updated. So, in me view, using
> this patch is justifiedand very handy here.
> 3. Also, apply all the "TEST" (0006 and 0007) patches to the "new" cluster.
> 4. Build "old" and "new" pg clusters.
> 5. Run the test with: PROVE_TESTS=t/005_offset.pl
> <http://005_offset.pl> PG_TEST_NOCLEAN=1 oldinstall=/home/orlov/
> proj/OFFSET3/pgsql-old make check -s -C src/bin/pg_upgrade/
> 6. In my case, it took around 45 minutes and generate roughly 2.7 Gb of
> data.
>
> "TEST" patches, of course, are for the test purposes and not to be
> committed.
>
> In src/bin/pg_upgrade/t/005_offset.pl <http://005_offset.pl> I try to
> consider next cases:
>
> * Basic sanity checks.
> Here I test various initial multi and offset values (including
> wraparound) and see how appropriate segments are generated.
> * pg_upgarde tests.
> Here is oldinstall ENV is for. Run pg_upgrade for old cluster with
> multi and offset values just like in previous step. i.e. with
> various combinations.
> * Self pg_upgarde.

Attached is some more cleanup on top of patch set v9, removing more dead
stuff related to wraparound. I also removed the oldestOffsetKnown
variable and related code. It was needed to deal with clusters upgraded
from buggy 9.3 and 9.4 era versions, but now that pg_upgrade will
rewrite the SLRUs, it's no longer needed.

Does the pg_upgrade code work though, if you have that buggy situation
where oldestOffsetKnown == false ?

>
> if (!TransactionIdIsValid(*xactptr))
> {
> /* Corner case 3: we must be looking at unused slot zero */
> Assert(offset == 0);
> continue;
> }

After upgrade, this corner case 3 would *not* happen on offset == 0. So
looks like we're still missing test coverage for this upgrade corner case.

I'm still felt pretty uneasy about the pg_upgrade code. It's
complicated, and the way it rewrites offsets and members separately and
page at a time is quite different from the normal codepaths in
multixact.c, so it's a bit hard to see if it's handling all those corner
cases the same way. I rewrito that so that it's easier to understand,
IMHO anyway. The code for reading the old format and writing the new
format is now more decoupled. The code for reading the old format is in
a separate source file, multixact_old.c. The interface to that is the
GetOldMultiXactIdSingleMember() that returns the updating member of a
given multixid, much like the GetMultiXactIdMembers() backend function.
The conversion routine calls that for each multixid, and write it back
out in the new format, one multixid at a time.

The new code now "squeezes out" locking-only XIDs, keeping only the
updating ones. Not that important, but with this new code structure, it
was trivial and even easier to do than retaining all the XIDs.

Now that the offsets are rewritten one by one, we don't need the
"special case 3" in GetMultiXactIdMembers. The upgrade process removes
that special case.

I tried to keep my changes sepearate from your patches in the attached
patch series. This needs some more cleanup and squashing before
committing, but I think we're getting close.

--
Heikki Linnakangas
Neon (https://neon.tech)