public inbox for [email protected]
help / color / mirror / Atom feedFrom: Aleksander Alekseev <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: [PATCH] Refactor SLRU to always use long file names
Date: Tue, 12 Nov 2024 17:15:52 +0300
Message-ID: <CAJ7c6TMq46VGLtMVCev2z0QUfUYO1Fnfug0ouLuEZby2sa0+FQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAJ7c6TOy7fUW9MuNeOWor3cSFnQg9tgz=mjXHDb94GORtM_Eyg@mail.gmail.com>
<[email protected]>
<CAJ7c6TNs8RY4Yvng4m375mAvsS4wHeUcqR8t4b2_i2j=PE6iNQ@mail.gmail.com>
<[email protected]>
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();
Attachments:
[text/plain] 005_slru.pl.txt (2.5K, 2-005_slru.pl.txt)
download | inline:
# 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();
view thread (7+ 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]
Subject: Re: [PATCH] Refactor SLRU to always use long file names
In-Reply-To: <CAJ7c6TMq46VGLtMVCev2z0QUfUYO1Fnfug0ouLuEZby2sa0+FQ@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