Strengthen pg_waldump's --save-fullpage tests
Hi,
A recent commit [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d497093cbecccf6df26365e06a5f8f8614b591c8 added --save-fullpage option to pg_waldump to
extract full page images (FPI) from WAL records and save them into
files (one file per FPI) under a specified directory. While it added
tests to check the LSN from the FPI file name and the FPI file
contents, it missed to further check the FPI contents like the tuples
on the page. I'm attaching a patch that basically reads the FPI file
(saved by pg_waldump) contents and raw page from the table file (using
pageinspect extension) and compares the tuples from both of them. This
test ensures that the pg_waldump outputs the correct FPI. This idea is
also discussed elsewhere [2]/messages/by-id/CALj2ACXesN9DTjgsekM8fig7CxhhxQfQP4fCiSJgcmp9wrZOvA@mail.gmail.com.
Thoughts?
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d497093cbecccf6df26365e06a5f8f8614b591c8
[2]: /messages/by-id/CALj2ACXesN9DTjgsekM8fig7CxhhxQfQP4fCiSJgcmp9wrZOvA@mail.gmail.com
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Strengthen-pg_waldump-s-save-fullpage-tests.patchapplication/x-patch; name=v1-0001-Strengthen-pg_waldump-s-save-fullpage-tests.patchDownload
From f714f13f47f98c7b9a4f88539c76862cacaffe49 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 7 Jan 2023 08:29:28 +0000
Subject: [PATCH v1] Strengthen pg_waldump's --save-fullpage tests
---
src/bin/pg_waldump/Makefile | 2 ++
src/bin/pg_waldump/t/002_save_fullpage.pl | 34 +++++++++++++++++++++--
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_waldump/Makefile b/src/bin/pg_waldump/Makefile
index d6459e17c7..924fb6ad20 100644
--- a/src/bin/pg_waldump/Makefile
+++ b/src/bin/pg_waldump/Makefile
@@ -3,6 +3,8 @@
PGFILEDESC = "pg_waldump - decode and display WAL"
PGAPPICON=win32
+EXTRA_INSTALL = contrib/pageinspect
+
subdir = src/bin/pg_waldump
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
diff --git a/src/bin/pg_waldump/t/002_save_fullpage.pl b/src/bin/pg_waldump/t/002_save_fullpage.pl
index 5072703a3d..193887a115 100644
--- a/src/bin/pg_waldump/t/002_save_fullpage.pl
+++ b/src/bin/pg_waldump/t/002_save_fullpage.pl
@@ -38,16 +38,24 @@ max_wal_senders = 4
});
$node->start;
+$node->safe_psql('postgres', q(CREATE EXTENSION pageinspect));
+
# Generate data/WAL to examine that will have full pages in them.
$node->safe_psql(
'postgres',
"SELECT 'init' FROM pg_create_physical_replication_slot('regress_pg_waldump_slot', true, false);
-CREATE TABLE test_table AS SELECT generate_series(1,100) a;
+CREATE TABLE test_table AS SELECT generate_series(1,2) a;
-- Force FPWs on the next writes.
CHECKPOINT;
-UPDATE test_table SET a = a + 1;
+UPDATE test_table SET a = a * 100 WHERE a = 1;
");
+# Get raw page from the table.
+my $page_from_table = $node->safe_psql(
+ 'postgres',
+ q(SELECT get_raw_page('test_table', 0))
+);
+
($walfile_name, $blocksize) = split '\|' => $node->safe_psql('postgres',
"SELECT pg_walfile_name(pg_switch_wal()), current_setting('block_size')");
@@ -108,4 +116,26 @@ for my $fullpath (glob "$tmp_folder/raw/*")
ok($file_count > 0, 'verify that at least one block has been saved');
+# Check that the pg_waldump saves FPI file.
+my @fpi_files = glob "$tmp_folder/raw/*_main";
+is(scalar(@fpi_files), 1, 'one FPI file was created');
+
+# Read the content of the saved FPI file.
+my $page_from_fpi_file = $node->safe_psql(
+ 'postgres',
+ qq{SELECT pg_read_binary_file('$fpi_files[0]')}
+);
+
+# Compare the raw page from table and FPI file saved, they must contain same rows.
+my $result = $node->safe_psql(
+ 'postgres',
+ qq{SELECT tuple_data_split('test_table'::regclass, t_data, t_infomask, t_infomask2, t_bits)
+ FROM heap_page_items('$page_from_table')
+ EXCEPT
+ SELECT tuple_data_split('test_table'::regclass, t_data, t_infomask, t_infomask2, t_bits)
+ FROM heap_page_items('$page_from_fpi_file')}
+);
+
+is($result, '', 'verify that rows in raw page from table and FPI file saved are same');
+
done_testing();
--
2.34.1
On Mon, Jan 09, 2023 at 08:30:00AM +0530, Bharath Rupireddy wrote:
A recent commit [1] added --save-fullpage option to pg_waldump to
extract full page images (FPI) from WAL records and save them into
files (one file per FPI) under a specified directory. While it added
tests to check the LSN from the FPI file name and the FPI file
contents, it missed to further check the FPI contents like the tuples
on the page. I'm attaching a patch that basically reads the FPI file
(saved by pg_waldump) contents and raw page from the table file (using
pageinspect extension) and compares the tuples from both of them. This
test ensures that the pg_waldump outputs the correct FPI. This idea is
also discussed elsewhere [2].Thoughts?
I am not sure that it is necessary to expand this set of tests to have
dependencies on heap and pageinspect (if we do so, what of index AMs)
and spend more cycles on that, while we already have something in
place to cross-check ReadRecPtr with what's stored in the page header
written on top of the block size.
--
Michael
On Tue, Jan 10, 2023 at 6:52 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jan 09, 2023 at 08:30:00AM +0530, Bharath Rupireddy wrote:
A recent commit [1] added --save-fullpage option to pg_waldump to
extract full page images (FPI) from WAL records and save them into
files (one file per FPI) under a specified directory. While it added
tests to check the LSN from the FPI file name and the FPI file
contents, it missed to further check the FPI contents like the tuples
on the page. I'm attaching a patch that basically reads the FPI file
(saved by pg_waldump) contents and raw page from the table file (using
pageinspect extension) and compares the tuples from both of them. This
test ensures that the pg_waldump outputs the correct FPI. This idea is
also discussed elsewhere [2].Thoughts?
I am not sure that it is necessary to expand this set of tests to have
dependencies on heap and pageinspect (if we do so, what of index AMs)
and spend more cycles on that, while we already have something in
place to cross-check ReadRecPtr with what's stored in the page header
written on top of the block size.
While checking for a page LSN is enough here, there's no harm in
verifying the whole FPI fetched from WAL record with that of the raw
page data. Also, this test illustrates how one can make use of the
fetched FPI - like reading the contents using pg_read_binary_file()
(of course on can also use COPY command to load the FPI data to
postgres) and using pageinspect functions to make sense of the raw
data.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Hi,
On 1/10/23 2:22 AM, Michael Paquier wrote:
On Mon, Jan 09, 2023 at 08:30:00AM +0530, Bharath Rupireddy wrote:
A recent commit [1] added --save-fullpage option to pg_waldump to
extract full page images (FPI) from WAL records and save them into
files (one file per FPI) under a specified directory. While it added
tests to check the LSN from the FPI file name and the FPI file
contents, it missed to further check the FPI contents like the tuples
on the page. I'm attaching a patch that basically reads the FPI file
(saved by pg_waldump) contents and raw page from the table file (using
pageinspect extension) and compares the tuples from both of them. This
test ensures that the pg_waldump outputs the correct FPI. This idea is
also discussed elsewhere [2].Thoughts?
I am not sure that it is necessary to expand this set of tests to have
dependencies on heap and pageinspect (if we do so, what of index AMs)
and spend more cycles on that, while we already have something in
place to cross-check ReadRecPtr with what's stored in the page header
written on top of the block size.
I like the idea of comparing the full page (and not just the LSN) but
I'm not sure that adding the pageinspect dependency is a good thing.
What about extracting the block directly from the relation file and
comparing it with the one extracted from the WAL? (We'd need to skip the
first 8 bytes to skip the LSN though).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Tue, Jan 10, 2023 at 05:25:44PM +0100, Drouvot, Bertrand wrote:
I like the idea of comparing the full page (and not just the LSN) but
I'm not sure that adding the pageinspect dependency is a good thing.What about extracting the block directly from the relation file and
comparing it with the one extracted from the WAL? (We'd need to skip the
first 8 bytes to skip the LSN though).
Byte-by-byte counting for the page hole? The page checksum would
matter as well, FWIW, as it is not set in WAL and a FPW logged in WAL
means that the page got modified. It means that it could have been
flushed, updating its pd_lsn and its pd_checksum on the way.
--
Michael
On Wed, Jan 11, 2023 at 6:32 AM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 10, 2023 at 05:25:44PM +0100, Drouvot, Bertrand wrote:
I like the idea of comparing the full page (and not just the LSN) but
I'm not sure that adding the pageinspect dependency is a good thing.What about extracting the block directly from the relation file and
comparing it with the one extracted from the WAL? (We'd need to skip the
first 8 bytes to skip the LSN though).Byte-by-byte counting for the page hole? The page checksum would
matter as well, FWIW, as it is not set in WAL and a FPW logged in WAL
means that the page got modified. It means that it could have been
flushed, updating its pd_lsn and its pd_checksum on the way.
Right. LSN of FPI from the WAL record and page from the table won't be
the same, essentially FPI LSN <= table page. Since the LSNs are
different, checksums too. This is the reason we have masking functions
common/bufmask.c and rm_mask functions defined for some of the
resource managers while verifying FPI consistency in
verifyBackupPageConsistency(). Note that pageinspect can give only
unmasked/raw page data, which means, byte-by-byte comparison isn't
possible with pageinspect too, hence I was comparing only the rows
with tuple_data_split().
Therefore, reading bytes from the table page and comparing
byte-by-byte with FPI requires us to invent new masking functions in
the tests - simply a no-go IMO.
As the concern here is to not establish pageinspect dependency with
pg_waldump, I'm fine to withdraw this patch and be happy with what we
have today.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Hi,
On 1/11/23 5:17 AM, Bharath Rupireddy wrote:
On Wed, Jan 11, 2023 at 6:32 AM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 10, 2023 at 05:25:44PM +0100, Drouvot, Bertrand wrote:
I like the idea of comparing the full page (and not just the LSN) but
I'm not sure that adding the pageinspect dependency is a good thing.What about extracting the block directly from the relation file and
comparing it with the one extracted from the WAL? (We'd need to skip the
first 8 bytes to skip the LSN though).Byte-by-byte counting for the page hole?
I've in mind to use diff on the whole page (minus the LSN).
The page checksum would
matter as well,
Right, but the TAP test is done without checksum and we could also
skip the checksum from the page if we really want to.
Right. LSN of FPI from the WAL record and page from the table won't be
the same, essentially FPI LSN <= table page.
Right, that's why I proposed to exclude it for the comparison.
What about something like the attached?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Strengthen-pg_waldump-s-save-fullpage-tests.patchtext/plain; charset=UTF-8; name=v2-0001-Strengthen-pg_waldump-s-save-fullpage-tests.patchDownload
diff --git a/src/bin/pg_waldump/t/002_save_fullpage.pl b/src/bin/pg_waldump/t/002_save_fullpage.pl
index 5072703a3d..21adee7771 100644
--- a/src/bin/pg_waldump/t/002_save_fullpage.pl
+++ b/src/bin/pg_waldump/t/002_save_fullpage.pl
@@ -10,6 +10,19 @@ use PostgreSQL::Test::Utils;
use Test::More;
my ($blocksize, $walfile_name);
+my $node = PostgreSQL::Test::Cluster->new('main');
+my $pgdata = $node->data_dir;
+
+# Returns the filesystem path for the named relation.
+sub relation_filepath
+{
+ my ($relname) = @_;
+
+ my $rel = $node->safe_psql('postgres',
+ qq(SELECT pg_relation_filepath('$relname')));
+ die "path not found for relation $relname" unless defined $rel;
+ return "$pgdata/$rel";
+}
# Function to extract the LSN from the given block structure
sub get_block_lsn
@@ -29,7 +42,6 @@ sub get_block_lsn
return ($lsn_hi, $lsn_lo);
}
-my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->append_conf(
'postgresql.conf', q{
@@ -45,7 +57,8 @@ $node->safe_psql(
CREATE TABLE test_table AS SELECT generate_series(1,100) a;
-- Force FPWs on the next writes.
CHECKPOINT;
-UPDATE test_table SET a = a + 1;
+UPDATE test_table SET a = a + 1 where a = 1;
+CHECKPOINT;
");
($walfile_name, $blocksize) = split '\|' => $node->safe_psql('postgres',
@@ -108,4 +121,62 @@ for my $fullpath (glob "$tmp_folder/raw/*")
ok($file_count > 0, 'verify that at least one block has been saved');
+# Check that the pg_waldump saves FPI file.
+my @fpi_files = glob "$tmp_folder/raw/*_main";
+is(scalar(@fpi_files), 1, 'one FPI file was created');
+
+# Now extract the block from the relation file and compare with the FPI
+my $relpath = relation_filepath('test_table');
+my $blk;
+my $blkfpi;
+
+my $frel;
+my $blkfrel;
+my $blkfpifh;
+my $blkfpinolsn;
+
+open($frel, '+<', $relpath)
+ or BAIL_OUT("open failed: $!");
+
+open($blkfrel, '+>', "$tmp_folder/test_table.blk0")
+ or BAIL_OUT("open failed: $!");
+
+open($blkfpifh, '+<', $fpi_files[0])
+ or BAIL_OUT("open failed: $!");
+
+open($blkfpinolsn, '+>', "$tmp_folder/fpinolsn")
+ or BAIL_OUT("open failed: $!");
+
+binmode $frel;
+binmode $blkfrel;
+binmode $blkfpifh;
+binmode $blkfpinolsn;
+
+# Extract the binary data without the LSN from the relation's block
+sysseek($frel, 8, 0); #bypass the LSN
+sysread($frel, $blk, 8184) or die "sysread failed: $!";
+syswrite($blkfrel, $blk) or die "syswrite failed: $!";
+
+# Extract the binary data without the LSN from the FPI
+sysseek($blkfpifh, 8, 0); #bypass the LSN
+sysread($blkfpifh, $blkfpi, 8184) or die "sysread failed: $!";
+syswrite($blkfpinolsn, $blkfpi) or die "syswrite failed: $!";
+
+close($frel)
+ or BAIL_OUT("close failed: $!");
+
+close($blkfrel)
+ or BAIL_OUT("close failed: $!");
+
+close($blkfpifh)
+ or BAIL_OUT("close failed: $!");
+
+close($blkfpinolsn)
+ or BAIL_OUT("close failed: $!");
+
+# Compare the blocks (LSN excluded)
+command_ok(
+ [ 'diff', $tmp_folder . '/fpinolsn', $tmp_folder . '/test_table.blk0' ],
+ 'compare both pages');
+
done_testing();
On Wed, Jan 11, 2023 at 3:28 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
Hi,
On 1/11/23 5:17 AM, Bharath Rupireddy wrote:
On Wed, Jan 11, 2023 at 6:32 AM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 10, 2023 at 05:25:44PM +0100, Drouvot, Bertrand wrote:
I like the idea of comparing the full page (and not just the LSN) but
I'm not sure that adding the pageinspect dependency is a good thing.What about extracting the block directly from the relation file and
comparing it with the one extracted from the WAL? (We'd need to skip the
first 8 bytes to skip the LSN though).Byte-by-byte counting for the page hole?
I've in mind to use diff on the whole page (minus the LSN).
The page checksum would
matter as well,Right, but the TAP test is done without checksum and we could also
skip the checksum from the page if we really want to.Right. LSN of FPI from the WAL record and page from the table won't be
the same, essentially FPI LSN <= table page.Right, that's why I proposed to exclude it for the comparison.
What about something like the attached?
Note that the raw page on the table might differ not just in page LSN
but also in other fields, for instance see heap_mask for instance. It
masks lsn, checksum, hint bits, unused space etc. before verifying FPI
consistency during recovery in
verifyBackupPageConsistency().
I think the job of verifying FPI from WAL record with the page LSN is
better left to the core - via verifyBackupPageConsistency(). Honestly,
pg_waldump is good with what it has currently - LSN checks.
+# Extract the binary data without the LSN from the relation's block
+sysseek($frel, 8, 0); #bypass the LSN
+sysread($frel, $blk, 8184) or die "sysread failed: $!";
+syswrite($blkfrel, $blk) or die "syswrite failed: $!";I suspect that these tests are portable with the hardcoded values such as above.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Jan 11, 2023 at 07:17:47PM +0530, Bharath Rupireddy wrote:
Note that the raw page on the table might differ not just in page LSN
but also in other fields, for instance see heap_mask for instance. It
masks lsn, checksum, hint bits, unused space etc. before verifying FPI
consistency during recovery in
verifyBackupPageConsistency().
FWIW, I don't really want to enter in this business here. That feels
like a good addition of technical debt compared to the potential
gain.
--
Michael
On Thu, Jan 12, 2023 at 10:14 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jan 11, 2023 at 07:17:47PM +0530, Bharath Rupireddy wrote:
Note that the raw page on the table might differ not just in page LSN
but also in other fields, for instance see heap_mask for instance. It
masks lsn, checksum, hint bits, unused space etc. before verifying FPI
consistency during recovery in
verifyBackupPageConsistency().FWIW, I don't really want to enter in this business here. That feels
like a good addition of technical debt compared to the potential
gain.
I couldn't agree more.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Hi,
On 1/12/23 5:44 AM, Michael Paquier wrote:
On Wed, Jan 11, 2023 at 07:17:47PM +0530, Bharath Rupireddy wrote:
Note that the raw page on the table might differ not just in page LSN
but also in other fields, for instance see heap_mask for instance. It
masks lsn, checksum, hint bits, unused space etc. before verifying FPI
consistency during recovery in
verifyBackupPageConsistency().FWIW, I don't really want to enter in this business here. That feels
like a good addition of technical debt compared to the potential
gain.
Agree, let's forget about it.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com