RE: persist logical slots to disk during shutdown checkpoint - Mailing list pgsql-hackers

Dear hackers,

Thanks for forking the thread! I think we would choose another design, but I wanted
to post the updated version once with the current approach. All comments came
from the parent thread [1].

> 1. GENERAL -- git apply
>
> The patch fails to apply cleanly. There are whitespace warnings.
>
> [postgres(at)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.

There was an extra blank, removed.

> 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.

Attached one in another thread is just copy to make cfbot happy, it could be
ignored.

> 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/

Fixed.

> 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.

Changed to 'test_tbl'.

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

Fixed.

> 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"

Hmm, these notations were followed the test recovery/t/016_min_consistency.pl,
it uses the word "minRecoveryPoint". So I preferred current one.

[1]: https://www.postgresql.org/message-id/CAHut%2BPtb%3DZYTM_awoLy3sJ5m9Oxe%3DJYn6Gve5rSW9cUdThpsVA%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED