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=gyqkxox0afnbm1fbp7sy7t4yw...@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
