pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedAdd basic tests for the low-level backup method.
14+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2024-03-14 071e3ad59d Add basic TAP tests for the low-level backup method, take two
2024-03-14 99b4a63bef Add basic TAP tests for the low-level backup method
* Add basic tests for the low-level backup method.
@ 2023-11-11 19:21 David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
0 siblings, 1 reply; 14+ messages in thread
From: David Steele @ 2023-11-11 19:21 UTC (permalink / raw)
To: pgsql-hackers
Hackers,
As promised in [1], attached are some basic tests for the low-level
backup method.
There are currently no tests for the low-level backup method.
pg_backup_start() and pg_backup_stop() are called but not exercised in
any real fashion.
There is a lot more that can be done, but this at least supplies some
basic tests and provides a place for future improvement.
Regards,
-David
[1]
https://www.postgresql.org/message-id/2daf8adc-8db7-4204-a7f2-a7e94e2bfa4b%40pgmasters.net
Attachments:
low-level-backup-test-v1.patch (text/plain, 4.7K) download
From b1a10ab4ab912e6f174206fcb8ce67496f93df83 Mon Sep 17 00:00:00 2001
From: David Steele <david@pgmasters.net>
Date: Sat, 11 Nov 2023 19:12:49 +0000
Subject: Add basic tests for the low-level backup method.
There are currently no tests for the low-level backup method. pg_backup_start()
and pg_backup_stop() are called but not exercised in any real fashion.
There is a lot more that can be done, but this at least provides some basic
tests and provides a place for future improvement.
---
src/test/recovery/t/040_low_level_backup.pl | 101 ++++++++++++++++++++
1 file changed, 101 insertions(+)
create mode 100644 src/test/recovery/t/040_low_level_backup.pl
diff --git a/src/test/recovery/t/040_low_level_backup.pl b/src/test/recovery/t/040_low_level_backup.pl
new file mode 100644
index 00000000000..3f30cb6b59c
--- /dev/null
+++ b/src/test/recovery/t/040_low_level_backup.pl
@@ -0,0 +1,101 @@
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Test low-level backup method by using pg_backup_start() and pg_backup_stop()
+# to create backups.
+
+use strict;
+use warnings;
+
+use File::Copy qw(copy);
+use File::Path qw(remove_tree);
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Start primary node with archiving
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+$node_primary->init(has_archiving => 1, allows_streaming => 1);
+$node_primary->start;
+
+# Start backup
+my $backup_name = 'backup1';
+my $psql = $node_primary->background_psql('postgres');
+
+$psql->query_safe("SET client_min_messages TO WARNING");
+$psql->set_query_timer_restart();
+$psql->query_safe("select pg_backup_start('test label')");
+
+# Copy files
+my $backup_dir = $node_primary->backup_dir() . '/' . $backup_name;
+
+PostgreSQL::Test::RecursiveCopy::copypath(
+ $node_primary->data_dir(), $backup_dir);
+
+# Cleanup files/paths that should not be in the backup. Also remove pg_control
+# because it needs to be copied later and it will be obvious if the copy fails.
+unlink("$backup_dir/postmaster.pid")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.pid");
+unlink("$backup_dir/postmaster.opts")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.opts");
+unlink("$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to unlink $backup_dir/global/pg_control");
+
+remove_tree("$backup_dir/pg_wal", {keep_root => 1})
+ or BAIL_OUT("unable to unlink contents of $backup_dir/pg_wal");
+
+# Create a table that will be used to verify that recovery started at the
+# correct location, rather than the location recorded in pg_control
+$psql->query_safe("create table canary (id int)");
+
+# Advance the checkpoint location in pg_control past the location where the
+# backup started. Switch the WAL to make it really obvious that the location
+# is different and to put the checkpoint in a new WAL segment.
+$psql->query_safe("select pg_switch_wal()");
+$psql->query_safe("checkpoint");
+
+# Copy pg_control last so it contains the new checkpoint
+copy($node_primary->data_dir() . '/global/pg_control',
+ "$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to copy global/pg_control");
+
+# Stop backup and get backup_label
+my $backup_label = $psql->query_safe("select labelfile from pg_backup_stop()");
+
+# Rather than writing out backup_label, try to recover the backup without
+# backup_label to demonstrate that recovery will not work correctly without it,
+# i.e. the canary table will be missing and the cluster will be corrupt. Provide
+# only the WAL segment that recovery will think it needs.
+#
+# The point of this test is to explicitly demonstrate that backup_label is
+# being used in a later test to get the correct recovery info.
+my $node_replica = PostgreSQL::Test::Cluster->new('replica_fail');
+$node_replica->init_from_backup($node_primary, $backup_name);
+my $canary_query = "select count(*) from pg_class where relname = 'canary'";
+
+copy($node_primary->archive_dir() . '/000000010000000000000003',
+ $node_replica->data_dir() . '/pg_wal/000000010000000000000003')
+ or BAIL_OUT("unable to copy 000000010000000000000003");
+
+$node_replica->start;
+
+ok($node_replica->safe_psql('postgres', $canary_query) == 0,
+ 'canary is missing');
+
+$node_replica->teardown_node();
+$node_replica->clean_node();
+
+# Save backup_label into the backup directory and recover using the primary's
+# archive. This time recovery will succeed and the canary table will be
+# present.
+append_to_file("$backup_dir/backup_label", $backup_label);
+
+$node_replica = PostgreSQL::Test::Cluster->new('replica_success');
+$node_replica->init_from_backup($node_primary, $backup_name,
+ has_restoring => 1);
+$node_replica->start;
+
+ok($node_replica->safe_psql('postgres', $canary_query) == 1,
+ 'canary is present');
+
+done_testing();
--
2.34.1
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
@ 2024-02-28 21:30 ` David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 14+ messages in thread
From: David Steele @ 2024-02-28 21:30 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
On 11/12/23 08:21, David Steele wrote:
>
> As promised in [1], attached are some basic tests for the low-level
> backup method.
Added to the 2024-03 CF.
Regards,
-David
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
@ 2024-02-29 03:55 ` Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2024-02-29 03:55 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: pgsql-hackers@lists.postgresql.org
On Thu, Feb 29, 2024 at 10:30:52AM +1300, David Steele wrote:
> On 11/12/23 08:21, David Steele wrote:
>> As promised in [1], attached are some basic tests for the low-level
>> backup method.
>
> Added to the 2024-03 CF.
There is already 040_standby_failover_slots_sync.pl in recovery/ that
uses the number of your test script. You may want to bump it, that's
a nit.
+unlink("$backup_dir/postmaster.pid")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.pid");
+unlink("$backup_dir/postmaster.opts")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.opts");
+unlink("$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to unlink $backup_dir/global/pg_control");
RELCACHE_INIT_FILENAME as well?
+# Rather than writing out backup_label, try to recover the backup without
+# backup_label to demonstrate that recovery will not work correctly without it,
+# i.e. the canary table will be missing and the cluster will be corrupt. Provide
+# only the WAL segment that recovery will think it needs.
Okay, why not. No objections to this addition. I am a bit surprised
that this is not scanning some of the logs produced by the startup
process for particular patterns.
+# Save backup_label into the backup directory and recover using the primary's
+# archive. This time recovery will succeed and the canary table will be
+# present.
Here are well, I think that we should add some log_contains() with
pre-defined patterns to show that recovery has completed the way we
want it with a backup_label up to the end-of-backup record.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
@ 2024-03-13 00:12 ` David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 14+ messages in thread
From: David Steele @ 2024-03-13 00:12 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers@lists.postgresql.org
On 2/29/24 16:55, Michael Paquier wrote:
> On Thu, Feb 29, 2024 at 10:30:52AM +1300, David Steele wrote:
>> On 11/12/23 08:21, David Steele wrote:
>>> As promised in [1], attached are some basic tests for the low-level
>>> backup method.
>>
>> Added to the 2024-03 CF.
>
> There is already 040_standby_failover_slots_sync.pl in recovery/ that
> uses the number of your test script. You may want to bump it, that's
> a nit.
Renamed to 042_low_level_backup.pl.
> +unlink("$backup_dir/postmaster.pid")
> + or BAIL_OUT("unable to unlink $backup_dir/postmaster.pid");
> +unlink("$backup_dir/postmaster.opts")
> + or BAIL_OUT("unable to unlink $backup_dir/postmaster.opts");
> +unlink("$backup_dir/global/pg_control")
> + or BAIL_OUT("unable to unlink $backup_dir/global/pg_control");
>
> RELCACHE_INIT_FILENAME as well?
I'm not trying to implement the full exclusion list here, just enough to
get the test working. Since exclusions are optional according to the
docs I don't think we need them for a valid tests.
> +# Rather than writing out backup_label, try to recover the backup without
> +# backup_label to demonstrate that recovery will not work correctly without it,
> +# i.e. the canary table will be missing and the cluster will be corrupt. Provide
> +# only the WAL segment that recovery will think it needs.
>
> Okay, why not. No objections to this addition. I am a bit surprised
> that this is not scanning some of the logs produced by the startup
> process for particular patterns.
Not sure what to look for here. There are no distinct messages for crash
recovery. Perhaps there should be?
> +# Save backup_label into the backup directory and recover using the primary's
> +# archive. This time recovery will succeed and the canary table will be
> +# present.
>
> Here are well, I think that we should add some log_contains() with
> pre-defined patterns to show that recovery has completed the way we
> want it with a backup_label up to the end-of-backup record.
Sure, I added a check for the new log message when recovering with a
backup_label.
Regards,
-David
Attachments:
low-level-backup-test-v2.patch (text/plain, 5.0K) download
From 076429a578ece3c213525220a4961cb5b56c190a Mon Sep 17 00:00:00 2001
From: David Steele <david@pgmasters.net>
Date: Wed, 13 Mar 2024 00:01:55 +0000
Subject: Add basic tests for the low-level backup method.
There are currently no tests for the low-level backup method. pg_backup_start()
and pg_backup_stop() are called but not exercised in any real fashion.
There is a lot more that can be done, but this at least provides some basic
tests and provides a place for future improvement.
---
src/test/recovery/t/042_low_level_backup.pl | 108 ++++++++++++++++++++
1 file changed, 108 insertions(+)
create mode 100644 src/test/recovery/t/042_low_level_backup.pl
diff --git a/src/test/recovery/t/042_low_level_backup.pl b/src/test/recovery/t/042_low_level_backup.pl
new file mode 100644
index 0000000000..22668bdc0d
--- /dev/null
+++ b/src/test/recovery/t/042_low_level_backup.pl
@@ -0,0 +1,108 @@
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Test low-level backup method by using pg_backup_start() and pg_backup_stop()
+# to create backups.
+
+use strict;
+use warnings;
+
+use File::Copy qw(copy);
+use File::Path qw(remove_tree);
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Start primary node with archiving
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+$node_primary->init(has_archiving => 1, allows_streaming => 1);
+$node_primary->start;
+
+# Start backup
+my $backup_name = 'backup1';
+my $psql = $node_primary->background_psql('postgres');
+
+$psql->query_safe("SET client_min_messages TO WARNING");
+$psql->set_query_timer_restart();
+$psql->query_safe("select pg_backup_start('test label')");
+
+# Copy files
+my $backup_dir = $node_primary->backup_dir() . '/' . $backup_name;
+
+PostgreSQL::Test::RecursiveCopy::copypath(
+ $node_primary->data_dir(), $backup_dir);
+
+# Cleanup some files/paths that should not be in the backup. There is no
+# attempt to all the exclusions done by pg_basebackup here, in part because
+# they are not required, but also to keep the test simple.
+#
+# Also remove pg_control because it needs to be copied later and it will be
+# obvious if the copy fails.
+unlink("$backup_dir/postmaster.pid")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.pid");
+unlink("$backup_dir/postmaster.opts")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.opts");
+unlink("$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to unlink $backup_dir/global/pg_control");
+
+remove_tree("$backup_dir/pg_wal", {keep_root => 1})
+ or BAIL_OUT("unable to unlink contents of $backup_dir/pg_wal");
+
+# Create a table that will be used to verify that recovery started at the
+# correct location, rather than the location recorded in pg_control
+$psql->query_safe("create table canary (id int)");
+
+# Advance the checkpoint location in pg_control past the location where the
+# backup started. Switch the WAL to make it really obvious that the location
+# is different and to put the checkpoint in a new WAL segment.
+$psql->query_safe("select pg_switch_wal()");
+$psql->query_safe("checkpoint");
+
+# Copy pg_control last so it contains the new checkpoint
+copy($node_primary->data_dir() . '/global/pg_control',
+ "$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to copy global/pg_control");
+
+# Stop backup and get backup_label
+my $backup_label = $psql->query_safe("select labelfile from pg_backup_stop()");
+
+# Rather than writing out backup_label, try to recover the backup without
+# backup_label to demonstrate that recovery will not work correctly without it,
+# i.e. the canary table will be missing and the cluster will be corrupt. Provide
+# only the WAL segment that recovery will think it needs.
+#
+# The point of this test is to explicitly demonstrate that backup_label is
+# being used in a later test to get the correct recovery info.
+my $node_replica = PostgreSQL::Test::Cluster->new('replica_fail');
+$node_replica->init_from_backup($node_primary, $backup_name);
+my $canary_query = "select count(*) from pg_class where relname = 'canary'";
+
+copy($node_primary->archive_dir() . '/000000010000000000000003',
+ $node_replica->data_dir() . '/pg_wal/000000010000000000000003')
+ or BAIL_OUT("unable to copy 000000010000000000000003");
+
+$node_replica->start;
+
+ok($node_replica->safe_psql('postgres', $canary_query) == 0,
+ 'canary is missing');
+
+$node_replica->teardown_node();
+$node_replica->clean_node();
+
+# Save backup_label into the backup directory and recover using the primary's
+# archive. This time recovery will succeed and the canary table will be
+# present.
+append_to_file("$backup_dir/backup_label", $backup_label);
+
+$node_replica = PostgreSQL::Test::Cluster->new('replica_success');
+$node_replica->init_from_backup($node_primary, $backup_name,
+ has_restoring => 1);
+$node_replica->start;
+
+ok($node_replica->log_contains('completed backup recovery with redo LSN'),
+ 'verify backup recovery performed with backup_label');
+
+ok($node_replica->safe_psql('postgres', $canary_query) == 1,
+ 'canary is present');
+
+done_testing();
--
2.34.1
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
@ 2024-03-13 06:15 ` Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2024-03-13 06:15 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: pgsql-hackers@lists.postgresql.org
On Wed, Mar 13, 2024 at 01:12:28PM +1300, David Steele wrote:
> On 2/29/24 16:55, Michael Paquier wrote:
>> +unlink("$backup_dir/postmaster.pid")
>> + or BAIL_OUT("unable to unlink $backup_dir/postmaster.pid");
>> +unlink("$backup_dir/postmaster.opts")
>> + or BAIL_OUT("unable to unlink $backup_dir/postmaster.opts");
>> +unlink("$backup_dir/global/pg_control")
>> + or BAIL_OUT("unable to unlink $backup_dir/global/pg_control");
>>
>> RELCACHE_INIT_FILENAME as well?
>
> I'm not trying to implement the full exclusion list here, just enough to get
> the test working. Since exclusions are optional according to the docs I
> don't think we need them for a valid tests.
Okay. Fine by me at the end.
>> +# Rather than writing out backup_label, try to recover the backup without
>> +# backup_label to demonstrate that recovery will not work correctly without it,
>> +# i.e. the canary table will be missing and the cluster will be corrupt. Provide
>> +# only the WAL segment that recovery will think it needs.
>>
>> Okay, why not. No objections to this addition. I am a bit surprised
>> that this is not scanning some of the logs produced by the startup
>> process for particular patterns.
>
> Not sure what to look for here. There are no distinct messages for crash
> recovery. Perhaps there should be?
The closest thing I can think of here would be "database system was
not properly shut down; automatic recovery in progress" as we don't
have InArchiveRecovery, after checking that the canary is missing. If
you don't like this suggestion, feel free to say so, of course :)
>> +# Save backup_label into the backup directory and recover using the primary's
>> +# archive. This time recovery will succeed and the canary table will be
>> +# present.
>>
>> Here are well, I think that we should add some log_contains() with
>> pre-defined patterns to show that recovery has completed the way we
>> want it with a backup_label up to the end-of-backup record.
>
> Sure, I added a check for the new log message when recovering with a
> backup_label.
+ok($node_replica->log_contains('completed backup recovery with redo LSN'),
+ 'verify backup recovery performed with backup_label');
Okay for this choice. I was thinking first about "starting backup
recovery with redo LSN", closer to the area where the backup_label is
read.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
@ 2024-03-13 20:12 ` David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 14+ messages in thread
From: David Steele @ 2024-03-13 20:12 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers@lists.postgresql.org
On 3/13/24 19:15, Michael Paquier wrote:
> On Wed, Mar 13, 2024 at 01:12:28PM +1300, David Steele wrote:
>>
>> Not sure what to look for here. There are no distinct messages for crash
>> recovery. Perhaps there should be?
>
> The closest thing I can think of here would be "database system was
> not properly shut down; automatic recovery in progress" as we don't
> have InArchiveRecovery, after checking that the canary is missing. If
> you don't like this suggestion, feel free to say so, of course :)
That works for me. I think I got it confused with "database system was
interrupted..." when I was looking at the success vs. fail logs.
>> Sure, I added a check for the new log message when recovering with a
>> backup_label.
>
> +ok($node_replica->log_contains('completed backup recovery with redo LSN'),
> + 'verify backup recovery performed with backup_label');
>
> Okay for this choice. I was thinking first about "starting backup
> recovery with redo LSN", closer to the area where the backup_label is
> read.
I think you are right that the start message is better since it can only
appear once when the backup_label is found. The completed message could
in theory appear after a restart, though the backup_label must have been
found at some point.
Regards,
-David
Attachments:
low-level-backup-test-v3.patch (text/plain, 5.3K) download
From 91f8e8a390713760116990be05d96f8a7e0d1bde Mon Sep 17 00:00:00 2001
From: David Steele <david@pgmasters.net>
Date: Wed, 13 Mar 2024 20:02:40 +0000
Subject: Add basic tests for the low-level backup method.
There are currently no tests for the low-level backup method. pg_backup_start()
and pg_backup_stop() are called but not exercised in any real fashion.
There is a lot more that can be done, but this at least provides some basic
tests and provides a place for future improvement.
---
src/test/recovery/t/042_low_level_backup.pl | 114 ++++++++++++++++++++
1 file changed, 114 insertions(+)
create mode 100644 src/test/recovery/t/042_low_level_backup.pl
diff --git a/src/test/recovery/t/042_low_level_backup.pl b/src/test/recovery/t/042_low_level_backup.pl
new file mode 100644
index 0000000000..76bac7e324
--- /dev/null
+++ b/src/test/recovery/t/042_low_level_backup.pl
@@ -0,0 +1,114 @@
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Test low-level backup method by using pg_backup_start() and pg_backup_stop()
+# to create backups.
+
+use strict;
+use warnings;
+
+use File::Copy qw(copy);
+use File::Path qw(remove_tree);
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Start primary node with archiving
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+$node_primary->init(has_archiving => 1, allows_streaming => 1);
+$node_primary->start;
+
+# Start backup
+my $backup_name = 'backup1';
+my $psql = $node_primary->background_psql('postgres');
+
+$psql->query_safe("SET client_min_messages TO WARNING");
+$psql->set_query_timer_restart();
+$psql->query_safe("select pg_backup_start('test label')");
+
+# Copy files
+my $backup_dir = $node_primary->backup_dir() . '/' . $backup_name;
+
+PostgreSQL::Test::RecursiveCopy::copypath(
+ $node_primary->data_dir(), $backup_dir);
+
+# Cleanup some files/paths that should not be in the backup. There is no
+# attempt to all the exclusions done by pg_basebackup here, in part because
+# they are not required, but also to keep the test simple.
+#
+# Also remove pg_control because it needs to be copied later and it will be
+# obvious if the copy fails.
+unlink("$backup_dir/postmaster.pid")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.pid");
+unlink("$backup_dir/postmaster.opts")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.opts");
+unlink("$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to unlink $backup_dir/global/pg_control");
+
+remove_tree("$backup_dir/pg_wal", {keep_root => 1})
+ or BAIL_OUT("unable to unlink contents of $backup_dir/pg_wal");
+
+# Create a table that will be used to verify that recovery started at the
+# correct location, rather than the location recorded in pg_control
+$psql->query_safe("create table canary (id int)");
+
+# Advance the checkpoint location in pg_control past the location where the
+# backup started. Switch the WAL to make it really obvious that the location
+# is different and to put the checkpoint in a new WAL segment.
+$psql->query_safe("select pg_switch_wal()");
+$psql->query_safe("checkpoint");
+
+# Copy pg_control last so it contains the new checkpoint
+copy($node_primary->data_dir() . '/global/pg_control',
+ "$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to copy global/pg_control");
+
+# Stop backup and get backup_label
+my $backup_label = $psql->query_safe("select labelfile from pg_backup_stop()");
+
+# Rather than writing out backup_label, try to recover the backup without
+# backup_label to demonstrate that recovery will not work correctly without it,
+# i.e. the canary table will be missing and the cluster will be corrupt. Provide
+# only the WAL segment that recovery will think it needs.
+#
+# The point of this test is to explicitly demonstrate that backup_label is
+# being used in a later test to get the correct recovery info.
+my $node_replica = PostgreSQL::Test::Cluster->new('replica_fail');
+$node_replica->init_from_backup($node_primary, $backup_name);
+my $canary_query = "select count(*) from pg_class where relname = 'canary'";
+
+copy($node_primary->archive_dir() . '/000000010000000000000003',
+ $node_replica->data_dir() . '/pg_wal/000000010000000000000003')
+ or BAIL_OUT("unable to copy 000000010000000000000003");
+
+$node_replica->start;
+
+ok($node_replica->safe_psql('postgres', $canary_query) == 0,
+ 'canary is missing');
+
+# Check log to ensure crash recovery was used because backup label was missing
+ok($node_replica->log_contains(
+ 'database system was not properly shut down; automatic recovery in progress'),
+ 'verify backup recovery performed with crash recovery');
+
+$node_replica->teardown_node();
+$node_replica->clean_node();
+
+# Save backup_label into the backup directory and recover using the primary's
+# archive. This time recovery will succeed and the canary table will be
+# present.
+append_to_file("$backup_dir/backup_label", $backup_label);
+
+$node_replica = PostgreSQL::Test::Cluster->new('replica_success');
+$node_replica->init_from_backup($node_primary, $backup_name,
+ has_restoring => 1);
+$node_replica->start;
+
+ok($node_replica->safe_psql('postgres', $canary_query) == 1,
+ 'canary is present');
+
+# Check log to ensure backup_label was used for recovery
+ok($node_replica->log_contains('starting backup recovery with redo LSN'),
+ 'verify backup recovery performed with backup_label');
+
+done_testing();
--
2.34.1
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
@ 2024-03-14 07:00 ` Michael Paquier <michael@paquier.xyz>
2024-03-14 20:40 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2024-03-14 07:00 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: pgsql-hackers@lists.postgresql.org
On Thu, Mar 14, 2024 at 09:12:52AM +1300, David Steele wrote:
> I think you are right that the start message is better since it can only
> appear once when the backup_label is found. The completed message could in
> theory appear after a restart, though the backup_label must have been found
> at some point.
So, I've given a try to this patch with 99b4a63bef94, to note that
sidewinder failed because of a timing issue on Windows: the recovery
of the node without backup_label, expected to fail, would try to
backup the last segment it has replayed, because, as it has no
backup_label, it behaves like the primary. It would try to use the
same archive location as the primary, leading to a conflict failure on
Windows. This one was easy to fix, by overwritting postgresql.conf on
the node to not do archiving.
Following that, I've noticed a second race condition: we don't wait
for the segment after pg_switch_wal() to be archived. This one can be
easily avoided with a poll on pg_stat_archiver.
After that, also because I've initially managed to, cough, forget an
update of meson.build to list the new test, I've noticed a third
failure on Windows for the case of the node that has a backup_label.
Here is one of the failures:
https://cirrus-ci.com/task/5245341683941376
regress_log_042_low_level_backup and
042_low_level_backup_replica_success.log have all the information
needed, that can be summarized like that:
The system cannot find the file specified.
2024-03-14 06:02:37.670 GMT [560][startup] FATAL: invalid data in file "backup_label"
The first message is something new to me, that seems to point to a
corruption failure of the file system. Why don't we see something
similar in other tests, then? Leaving that aside..
The second LOG is something that can be acted on. I've added some
debugging to the parsing of the backup_label file in the backend, and
noticed that the first fscanf() for START WAL LOCATION is failing
because the last %c is detected as \r rather than \n. Tweaking the
contents stored from pg_backend_stop() with a sed won't help, because
the issue is that we write the CRLFs with append_to_file, and the
startup process cannot cope with that. The simplest method I can
think of is to use binmode, as of the attached.
It is the first time that we'd take the contents received from a
BackgroundPsql and write them to a file parsed by the backend, so
perhaps we should try to do that in a more general way, but I'm not
sure how, tbh, and the case of this test is special while adding
handling for \r when reading the backup_label got discussed in the
past but we were OK with what we are doing now on HEAD.
On top of all that, note that I have removed remove_tree as I am not
sure if this would be OK in all the buildfarm animals, added a quit()
for BackgroundPsql, moved queries to use less BackgroundPsql, as well
as a few other things like avoiding the hardcoded segment names.
meson.build is.. Cough.. Updated now.
I am attaching an updated patch with all that fixed, which is stable
in the CI and any tests I've run. Do you have any comments about
that?
--
Michael
Attachments:
v3-0001-Add-basic-TAP-tests-for-the-low-level-backup-meth.patch (text/x-diff, 7.1K) download
From 5799e7e48f02cd2ed1d8b2afb88001884c173855 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 14 Mar 2024 15:57:58 +0900
Subject: [PATCH v3] Add basic TAP tests for the low-level backup method
There are currently no tests for the low-level backup method where
pg_backup_start() and pg_backup_stop() are involved while taking a
file-system backup. The tests introduced in this commit rely on a
background psql process to make sure that the backup is taken while the
session doing the SQL start and stop calls remains alive.
Two cases are checked here with the backup taken:
- Recovery without a backup_label, leading to a corrupted state.
- Recovery with a backup_label, with a consistent state reached.
Both cases cross-check some patterns in the logs generated when running
recovery.
Author: David Steele
Discussion: https://postgr.es/m/f20fcc82-dadb-478d-beb4-1e2ffb0ace76@pgmasters.net
---
src/test/recovery/meson.build | 1 +
src/test/recovery/t/042_low_level_backup.pl | 144 ++++++++++++++++++++
2 files changed, 145 insertions(+)
create mode 100644 src/test/recovery/t/042_low_level_backup.pl
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index c67249500e..b1eb77b1ec 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -50,6 +50,7 @@ tests += {
't/039_end_of_wal.pl',
't/040_standby_failover_slots_sync.pl',
't/041_checkpoint_at_promote.pl',
+ 't/042_low_level_backup.pl',
],
},
}
diff --git a/src/test/recovery/t/042_low_level_backup.pl b/src/test/recovery/t/042_low_level_backup.pl
new file mode 100644
index 0000000000..0a75a8624a
--- /dev/null
+++ b/src/test/recovery/t/042_low_level_backup.pl
@@ -0,0 +1,144 @@
+
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+# Test low-level backup method by using pg_backup_start() and pg_backup_stop()
+# to create backups.
+
+use strict;
+use warnings;
+
+use File::Copy qw(copy);
+use File::Path qw(rmtree);
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Start primary node with archiving.
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+$node_primary->init(has_archiving => 1, allows_streaming => 1);
+$node_primary->start;
+
+# Start backup.
+my $backup_name = 'backup1';
+my $psql = $node_primary->background_psql('postgres');
+
+$psql->query_safe("SET client_min_messages TO WARNING");
+$psql->set_query_timer_restart;
+$psql->query_safe("select pg_backup_start('test label')");
+
+# Copy files.
+my $backup_dir = $node_primary->backup_dir . '/' . $backup_name;
+
+PostgreSQL::Test::RecursiveCopy::copypath($node_primary->data_dir,
+ $backup_dir);
+
+# Cleanup some files/paths that should not be in the backup. There is no
+# attempt to handle all the exclusions done by pg_basebackup here, in part
+# because these are not required, but also to keep the test simple.
+#
+# Also remove pg_control because it needs to be copied later.
+unlink("$backup_dir/postmaster.pid")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.pid");
+unlink("$backup_dir/postmaster.opts")
+ or BAIL_OUT("unable to unlink $backup_dir/postmaster.opts");
+unlink("$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to unlink $backup_dir/global/pg_control");
+
+rmtree("$backup_dir/pg_wal")
+ or BAIL_OUT("unable to unlink contents of $backup_dir/pg_wal");
+mkdir("$backup_dir/pg_wal");
+
+# Create a table that will be used to verify that recovery started at the
+# correct location, rather than a location recorded in the control file.
+$node_primary->safe_psql('postgres', "create table canary (id int)");
+
+# Advance the checkpoint location in pg_control past the location where the
+# backup started. Switch WAL to make it really clear that the location is
+# different and to put the checkpoint in a new WAL segment.
+my $segment_name = $node_primary->safe_psql('postgres',
+ "select pg_walfile_name(pg_switch_wal())");
+
+# Ensure that the segment just switched from is archived. The follow-up
+# tests depend on its presence to begin recovery.
+$node_primary->poll_query_until('postgres',
+ q{SELECT last_archived_wal FROM pg_stat_archiver},
+ $segment_name)
+ or die
+ "Timed out while waiting for archiving of switched segment to finish";
+
+$node_primary->safe_psql('postgres', "checkpoint");
+
+# Copy pg_control last so it contains the new checkpoint.
+copy($node_primary->data_dir . '/global/pg_control',
+ "$backup_dir/global/pg_control")
+ or BAIL_OUT("unable to copy global/pg_control");
+
+# Save the name segment that will be archived by pg_backup_stop().
+# This is copied to the pg_wal directory of the node whose recovery
+# is done without a backup_label.
+my $stop_segment_name = $node_primary->safe_psql('postgres',
+ 'SELECT pg_walfile_name(pg_current_wal_lsn())');
+
+# Stop backup and get backup_label, the last segment is archived.
+my $backup_label =
+ $psql->query_safe("select labelfile from pg_backup_stop()");
+
+$psql->quit;
+
+# Rather than writing out backup_label, try to recover the backup without
+# backup_label to demonstrate that recovery will not work correctly without it,
+# i.e. the canary table will be missing and the cluster will be corrupted.
+# Provide only the WAL segment that recovery will think it needs.
+#
+# The point of this test is to explicitly demonstrate that backup_label is
+# being used in a later test to get the correct recovery info.
+my $node_replica = PostgreSQL::Test::Cluster->new('replica_fail');
+$node_replica->init_from_backup($node_primary, $backup_name);
+$node_replica->append_conf('postgresql.conf', "archive_mode = off");
+
+my $canary_query = "select count(*) from pg_class where relname = 'canary'";
+
+copy(
+ $node_primary->archive_dir . "/$stop_segment_name",
+ $node_replica->data_dir . "/pg_wal/$stop_segment_name"
+) or BAIL_OUT("unable to copy $stop_segment_name");
+
+$node_replica->start;
+
+ok($node_replica->safe_psql('postgres', $canary_query) == 0,
+ 'canary is missing');
+
+# Check log to ensure that crash recovery was used as there is no
+# backup_label.
+ok( $node_replica->log_contains(
+ 'database system was not properly shut down; automatic recovery in progress'
+ ),
+ 'verify backup recovery performed with crash recovery');
+
+$node_replica->teardown_node;
+$node_replica->clean_node;
+
+# Save backup_label into the backup directory and recover using the primary's
+# archive. This time recovery will succeed and the canary table will be
+# present.
+open my $fh, ">>", "$backup_dir/backup_label"
+ or die "could not open backup_label";
+# Binary mode is required for Windows, as the backup_label parsing is not
+# able to cope with CRLFs.
+binmode $fh;
+print $fh $backup_label;
+close $fh;
+
+$node_replica = PostgreSQL::Test::Cluster->new('replica_success');
+$node_replica->init_from_backup($node_primary, $backup_name,
+ has_restoring => 1);
+$node_replica->start;
+
+ok($node_replica->safe_psql('postgres', $canary_query) == 1,
+ 'canary is present');
+
+# Check log to ensure that backup_label was used for recovery.
+ok($node_replica->log_contains('starting backup recovery with redo LSN'),
+ 'verify backup recovery performed with backup_label');
+
+done_testing();
--
2.43.0
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
@ 2024-03-14 20:40 ` David Steele <david@pgmasters.net>
2024-03-14 23:38 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 14+ messages in thread
From: David Steele @ 2024-03-14 20:40 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers@lists.postgresql.org
On 3/14/24 20:00, Michael Paquier wrote:
> On Thu, Mar 14, 2024 at 09:12:52AM +1300, David Steele wrote:
>> I think you are right that the start message is better since it can only
>> appear once when the backup_label is found. The completed message could in
>> theory appear after a restart, though the backup_label must have been found
>> at some point.
>
> So, I've given a try to this patch with 99b4a63bef94, to note that
> sidewinder failed because of a timing issue on Windows: the recovery
> of the node without backup_label, expected to fail, would try to
> backup the last segment it has replayed, because, as it has no
> backup_label, it behaves like the primary. It would try to use the
> same archive location as the primary, leading to a conflict failure on
> Windows. This one was easy to fix, by overwritting postgresql.conf on
> the node to not do archiving.
Hmmm, I wonder why this did not show up in the Windows tests on CI?
> Following that, I've noticed a second race condition: we don't wait
> for the segment after pg_switch_wal() to be archived. This one can be
> easily avoided with a poll on pg_stat_archiver.
Ugh, yeah, good change.
> After that, also because I've initially managed to, cough, forget an
> update of meson.build to list the new test, I've noticed a third
> failure on Windows for the case of the node that has a backup_label.
> Here is one of the failures:
> https://cirrus-ci.com/task/5245341683941376
Is the missing test in meson the reason we did not see test failures for
Windows in CI?
> regress_log_042_low_level_backup and
> 042_low_level_backup_replica_success.log have all the information
> needed, that can be summarized like that:
> The system cannot find the file specified.
> 2024-03-14 06:02:37.670 GMT [560][startup] FATAL: invalid data in file "backup_label"
>
> The first message is something new to me, that seems to point to a
> corruption failure of the file system. Why don't we see something
> similar in other tests, then? Leaving that aside..
>
> The second LOG is something that can be acted on. I've added some
> debugging to the parsing of the backup_label file in the backend, and
> noticed that the first fscanf() for START WAL LOCATION is failing
> because the last %c is detected as \r rather than \n. Tweaking the
> contents stored from pg_backend_stop() with a sed won't help, because
> the issue is that we write the CRLFs with append_to_file, and the
> startup process cannot cope with that. The simplest method I can
> think of is to use binmode, as of the attached.
Yeah, that makes sense.
> It is the first time that we'd take the contents received from a
> BackgroundPsql and write them to a file parsed by the backend, so
> perhaps we should try to do that in a more general way, but I'm not
> sure how, tbh, and the case of this test is special while adding
> handling for \r when reading the backup_label got discussed in the
> past but we were OK with what we are doing now on HEAD.
I think it makes sense to leave the parsing code as is and make the
change in the test. If we add more tests to this module we'll probably
need a function to avoid repeating code.
> On top of all that, note that I have removed remove_tree as I am not
> sure if this would be OK in all the buildfarm animals, added a quit()
> for BackgroundPsql, moved queries to use less BackgroundPsql, as well
> as a few other things like avoiding the hardcoded segment names.
> meson.build is.. Cough.. Updated now.
OK.
> I am attaching an updated patch with all that fixed, which is stable
> in the CI and any tests I've run. Do you have any comments about
These changes look good to me. Sure wish we had an easier to way to test
commits in the build farm.
Regards,
-David
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-14 20:40 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
@ 2024-03-14 23:38 ` Michael Paquier <michael@paquier.xyz>
2024-03-15 05:23 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-15 05:25 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 14+ messages in thread
From: Michael Paquier @ 2024-03-14 23:38 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: pgsql-hackers@lists.postgresql.org
On Fri, Mar 15, 2024 at 09:40:38AM +1300, David Steele wrote:
> Is the missing test in meson the reason we did not see test failures for
> Windows in CI?
The test has to be listed in src/test/recovery/meson.build or the CI
would ignore it.
>> The second LOG is something that can be acted on. I've added some
>> debugging to the parsing of the backup_label file in the backend, and
>> noticed that the first fscanf() for START WAL LOCATION is failing
>> because the last %c is detected as \r rather than \n. Tweaking the
>> contents stored from pg_backend_stop() with a sed won't help, because
>> the issue is that we write the CRLFs with append_to_file, and the
>> startup process cannot cope with that. The simplest method I can
>> think of is to use binmode, as of the attached.
>
> Yeah, that makes sense.
I am wondering if there is a better trick here that would not require
changes in the backend to make the backup_label parsing more flexible,
though.
>> I am attaching an updated patch with all that fixed, which is stable
>> in the CI and any tests I've run. Do you have any comments about
>
> These changes look good to me. Sure wish we had an easier to way to test
> commits in the build farm.
That's why these tests are not that easy, they can be racy. I've run
the test 5~10 times in the CI this time to gain more confidence, and
saw zero failures with the stability fixes in place including Windows.
I've applied it now, as I can still monitor the buildfarm for a few
more days. Let's see what happens, but that should be better.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-14 20:40 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 23:38 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
@ 2024-03-15 05:23 ` David Steele <david@pgmasters.net>
2024-03-15 05:32 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 14+ messages in thread
From: David Steele @ 2024-03-15 05:23 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers@lists.postgresql.org
On 3/15/24 12:38, Michael Paquier wrote:
> On Fri, Mar 15, 2024 at 09:40:38AM +1300, David Steele wrote:
>> Is the missing test in meson the reason we did not see test failures for
>> Windows in CI?
>
> The test has to be listed in src/test/recovery/meson.build or the CI
> would ignore it.
Right -- I will keep this in mind for the future.
>>> The second LOG is something that can be acted on. I've added some
>>> debugging to the parsing of the backup_label file in the backend, and
>>> noticed that the first fscanf() for START WAL LOCATION is failing
>>> because the last %c is detected as \r rather than \n. Tweaking the
>>> contents stored from pg_backend_stop() with a sed won't help, because
>>> the issue is that we write the CRLFs with append_to_file, and the
>>> startup process cannot cope with that. The simplest method I can
>>> think of is to use binmode, as of the attached.
>>
>> Yeah, that makes sense.
>
> I am wondering if there is a better trick here that would not require
> changes in the backend to make the backup_label parsing more flexible,
> though.
Well, this is what we recommend in the docs, i.e. using bin mode to save
backup_label, so it seems OK to me.
>>> I am attaching an updated patch with all that fixed, which is stable
>>> in the CI and any tests I've run. Do you have any comments about
>>
>> These changes look good to me. Sure wish we had an easier to way to test
>> commits in the build farm.
>
> That's why these tests are not that easy, they can be racy. I've run
> the test 5~10 times in the CI this time to gain more confidence, and
> saw zero failures with the stability fixes in place including Windows.
> I've applied it now, as I can still monitor the buildfarm for a few
> more days. Let's see what happens, but that should be better.
At least sidewinder is happy now -- and the build farm in general as far
as I can see.
Thank you for your help on this!
-David
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-14 20:40 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 23:38 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-15 05:23 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
@ 2024-03-15 05:32 ` Michael Paquier <michael@paquier.xyz>
2024-03-15 05:37 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2024-03-15 05:32 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: pgsql-hackers@lists.postgresql.org
On Fri, Mar 15, 2024 at 06:23:15PM +1300, David Steele wrote:
> Well, this is what we recommend in the docs, i.e. using bin mode to save
> backup_label, so it seems OK to me.
Indeed, I didn't notice that this is actually documented, so what I
did took the right angle. French flair, perhaps..
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-14 20:40 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 23:38 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-15 05:23 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-15 05:32 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
@ 2024-03-15 05:37 ` David Steele <david@pgmasters.net>
2024-03-15 23:43 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 14+ messages in thread
From: David Steele @ 2024-03-15 05:37 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers@lists.postgresql.org
On 3/15/24 18:32, Michael Paquier wrote:
> On Fri, Mar 15, 2024 at 06:23:15PM +1300, David Steele wrote:
>> Well, this is what we recommend in the docs, i.e. using bin mode to save
>> backup_label, so it seems OK to me.
>
> Indeed, I didn't notice that this is actually documented, so what I
> did took the right angle. French flair, perhaps..
This seems like a reasonable explanation to me.
-David
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-14 20:40 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 23:38 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-15 05:23 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-15 05:32 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-15 05:37 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
@ 2024-03-15 23:43 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 14+ messages in thread
From: Michael Paquier @ 2024-03-15 23:43 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: pgsql-hackers@lists.postgresql.org
On Fri, Mar 15, 2024 at 06:37:35PM +1300, David Steele wrote:
> This seems like a reasonable explanation to me.
FYI, drongo has just passed the test. fairywren uses TAP, does not
run the recovery tests.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Add basic tests for the low-level backup method.
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
2024-03-14 20:40 ` Re: Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-03-14 23:38 ` Re: Add basic tests for the low-level backup method. Michael Paquier <michael@paquier.xyz>
@ 2024-03-15 05:25 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 14+ messages in thread
From: Michael Paquier @ 2024-03-15 05:25 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: pgsql-hackers@lists.postgresql.org
On Fri, Mar 15, 2024 at 08:38:47AM +0900, Michael Paquier wrote:
> That's why these tests are not that easy, they can be racy. I've run
> the test 5~10 times in the CI this time to gain more confidence, and
> saw zero failures with the stability fixes in place including Windows.
> I've applied it now, as I can still monitor the buildfarm for a few
> more days. Let's see what happens, but that should be better.
So, it looks like the buildfarm is clear. sidewinder has reported a
green state, and the recent runs of the CFbot across all the patches
are looking stable as well on all platforms. There are still a few
buildfarm members on Windows that will take time more time before
running.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2024-03-15 23:43 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-11-11 19:21 Add basic tests for the low-level backup method. David Steele <david@pgmasters.net>
2024-02-28 21:30 ` David Steele <david@pgmasters.net>
2024-02-29 03:55 ` Michael Paquier <michael@paquier.xyz>
2024-03-13 00:12 ` David Steele <david@pgmasters.net>
2024-03-13 06:15 ` Michael Paquier <michael@paquier.xyz>
2024-03-13 20:12 ` David Steele <david@pgmasters.net>
2024-03-14 07:00 ` Michael Paquier <michael@paquier.xyz>
2024-03-14 20:40 ` David Steele <david@pgmasters.net>
2024-03-14 23:38 ` Michael Paquier <michael@paquier.xyz>
2024-03-15 05:23 ` David Steele <david@pgmasters.net>
2024-03-15 05:32 ` Michael Paquier <michael@paquier.xyz>
2024-03-15 05:37 ` David Steele <david@pgmasters.net>
2024-03-15 23:43 ` Michael Paquier <michael@paquier.xyz>
2024-03-15 05:25 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox