Hi Michael,

Thanks for your feedback!

> Your patch is just doing a rename() of the files from short to long
> names.  How about adding a new TAP script in pg_upgrade that creates a
> couple of empty files with short files names in each path that needs
> to do the transfer?  Then the test could run one pg_upgrade command
> and check that the new names are in place.  You could use a array of
> objects, with the base path, the old name and the new name, then loop
> over it.  With the check in check_slru_segment_filenames() based on
> SLRU_SEG_FILENAMES_CHANGE_CAT_VER, that should work.

OK I gave it a try and discovered that the test becomes very ugly very
fast. Attached is the draft of the test to give you an idea of how
it's going to look like.

In order to trigger renaming of SLRU segments first we have to
downgrade the catalog version in the pg_control file. Otherwise the
check in check_slru_segment_filenames() is not going to pass and
pg_upgrade will do nothing (*). This per se is easily done with
binmode() and pack() however the file has a CRC. Calculating it is not
difficult since we have pg_read_binary_file() and crc32c() SQL
functions, although personally I don't find a need for starting a
cluster for this quite satisfactory. The CRC is stored by the offset
`sizeof(ControlData)-4` and unless I'm wrong is platform-dependent.

I see several solutions for this problem:

* We could add sizeof(ControlData) to the output of `pg_controldata`
so we could use it from Perl
* We could teach `initdb` to override the catalog version
* We could implement a new tool for editing pg_control file

On top of that I should add that the test takes about 7 seconds on my
laptop. Apparently executing two initdb's and one pg_upgrade is not
very cheap. This makes me wonder if instead of writing a new test we
should modify 002_pg_upgrade.pl. This however will make the test even
less readable and maintainable.

What do you think?

(*) BTW I noticed a mistake in the commented code. The condition
should be `>=`, not `<`, i.e:

```
    if(new_cluster.controldata.cat_ver >= SLRU_SEG_FILENAMES_CHANGE_CAT_VER)
        return;
```

-- 
Best regards,
Aleksander Alekseev
# Copyright (c) 2024, PostgreSQL Global Development Group

use strict;
use warnings FATAL => 'all';

use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;

# This test ensures that pg_upgrade renames SLRU segments.
# After the upgrade all segments should have long file names.

# equals SLRU_SEG_FILENAMES_CHANGE_CAT_VER in pg_upgrade.h
my $slru_seg_filenames_change_cat_ver = 202411121;

my @slru_dirs = (
        "pg_xact",
        "pg_commit_ts",
        "pg_multixact/offsets",
        "pg_multixact/members",
        "pg_subtrans",
        "pg_serial",
);

my $short_segment_name = "1234";
my $long_segment_name = "000000000001234";

my $oldnode = PostgreSQL::Test::Cluster->new('old_node');
$oldnode->init();
my $oldbindir = $oldnode->config_data('--bindir');

my $newnode = PostgreSQL::Test::Cluster->new('new_node');
$newnode->init();
my $newbindir = $newnode->config_data('--bindir');

# Fill data_dir of the old node with SLRU segments that use short file names.
# pg_upgrade renames the files without looking at the content, so the content
# is not important.
foreach my $dir (@slru_dirs)
{
        my $fname = $oldnode->data_dir."/".$dir."/".$short_segment_name;
        open my $fh, ">", $fname or die $!;
        close $fh;
}

# Modify pg_control of the old node to make it look like a version that needs
# migration (decrease ControlData->cat_ver). Otherwise pg_upgrade will skip it.
my $pg_control_fname = $oldnode->data_dir."/global/pg_control";

open my $fh, "+<", $pg_control_fname or die $!;
binmode($fh);
sysseek($fh, 12, 0);
my $binval = pack("L!", $slru_seg_filenames_change_cat_ver - 1);
syswrite($fh, $binval, 4);
close($fh);

# Calculate CRC of the updated file using pg_read_binary_file() and crc32c()
my $fsize = -s $pg_control_fname; # WRONG! should be sizeof(ControlFile)
$newnode->start;
my $newcrc;
$newnode->psql(
        "postgres",
        "SELECT 
crc32(pg_read_binary_file('".$pg_control_fname."',0,".($fsize-4)."));",
        stdout => \$newcrc,
        on_error_die => 1,
        );
$newnode->stop;

# Update CRC
open $fh, "+<", $pg_control_fname or die $!;
binmode($fh);
sysseek($fh, $fsize-4, 0);
my $bincrc = pack("L!", $newcrc);
syswrite($fh, $bincrc, 4);
close($fh);

$newnode->command_ok(
        [
                'pg_upgrade',
                '--old-datadir', $oldnode->data_dir,
                '--new-datadir', $newnode->data_dir,
                '--old-bindir', $oldbindir,
                '--new-bindir', $newbindir,
        ],
        'run of pg_upgrade');

# Check that pg_upgrade renamed the SLRU segments we created
foreach my $dir (@slru_dirs)
{
        ok(-e $newnode->data_dir."/".$dir."/".$long_segment_name);
}

done_testing();

Reply via email to