Re: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

Here are some review comments for v23-0001

======
1. GENERAL -- git apply

The patch fails to apply cleanly. There are whitespace warnings.

[postgres@CentOS7-x64 oss_postgres_misc]$ git apply ../patches_misc/v23-0001-Always-persist-to-disk-logical-slots-during-a-sh.patch
../patches_misc/v23-0001-Always-persist-to-disk-logical-slots-during-a-sh.patch:102: trailing whitespace.
# SHUTDOWN_CHECKPOINT record.
warning: 1 line adds whitespace errors.

~~~

2. GENERAL -- which patch is the real one and which is the copy?

IMO this patch has become muddled.

Amit recently created a new thread [1] "persist logical slots to disk during shutdown checkpoint", which I thought was dedicated to the discussion/implementation of this 0001 patch. Therefore, I expected any 0001 patch changes to would be made only in that new thread from now on, (and maybe you would mirror them here in this thread).

But now I see there are v23-0001 patch changes here again. So, now the same patch is in 2 places and they are different. It is no longer clear to me which 0001 ("Always persist...") patch is the definitive one, and which one is the copy.

??

======
contrib/test_decoding/t/002_always_persist.pl

3.
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Test logical replication slots are always persist to disk during a shutdown
+# checkpoint.
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;


/always persist/always persisted/

~~~

4.
+
+# Test set-up
+my $node = PostgreSQL::Test::Cluster->new('test');
+$node->init(allows_streaming => 'logical');
+$node->append_conf('postgresql.conf', q{
+autovacuum = off
+checkpoint_timeout = 1h
+});
+
+$node->start;
+
+# Create table
+$node->safe_psql('postgres', "CREATE TABLE test (id int)");

Maybe it is better to call the table something different instead of the same name as the cluster. e.g. 'test_tbl' would be better.

~~~

5.
+# Shutdown the node once to do shutdown checkpoint
+$node->stop();
+

SUGGESTION
# Stop the node to cause a shutdown checkpoint

~~~

6.
+# Fetch checkPoint from the control file itself
+my ($stdout, $stderr) = run_command([ 'pg_controldata', $node->data_dir ]);
+my @control_data = split("\n", $stdout);
+my $latest_checkpoint = undef;
+foreach (@control_data)
+{
+ if ($_ =~ /^Latest checkpoint location:\s*(.*)$/mg)
+ {
+ $latest_checkpoint = $1;
+ last;
+ }
+}
+die "No checkPoint in control file found\n"
+  unless defined($latest_checkpoint);
+

6a.
/checkPoint/checkpoint/  (2x)

~

6b.
+die "No checkPoint in control file found\n"

SUGGESTION
"No checkpoint found in control file\n"

------
[1] https://www.postgresql.org/message-id/CAA4eK1JzJagMmb_E8D4au=GYQkxox0AfNBm1FbP7sy7t4YWXPQ@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia