persist logical slots to disk during shutdown checkpoint

On Sat, 19 Aug 2023, 14:16 Amit Kapila, <amit.kapila16@gmail.com> wrote:

It's entirely possible for a logical slot to have a confirmed_flush
LSN higher than the last value saved on disk while not being marked as
dirty. It's currently not a problem to lose that value during a clean
shutdown / restart cycle but to support the upgrade of logical slots
[1] (see latest patch at [2]), we seem to rely on that value being
properly persisted to disk. During the upgrade, we need to verify that
all the data prior to shudown_checkpoint for the logical slots has
been consumed, otherwise, the downstream may miss some data. Now, to
ensure the same, we are planning to compare the confirm_flush LSN
location with the latest shudown_checkpoint location which means that
the confirm_flush LSN should be updated after restart.

I think this is inefficient even without an upgrade because, after the
restart, this may lead to decoding some data again. Say, we process
some transactions for which we didn't send anything downstream (the
changes got filtered) but the confirm_flush LSN is updated due to
keepalives. As we don't flush the latest value of confirm_flush LSN,
it may lead to processing the same changes again.

In most cases there shouldn't be a lot of records to decode after restart,
but I agree it's better to avoid decoding those again.

The idea discussed in the thread [1] is to always persist logical

slots to disk during the shutdown checkpoint. I have extracted the
patch to achieve the same from that thread and attached it here. This
could lead to some overhead during shutdown (checkpoint) if there are
many slots but it is probably a one-time work.

I couldn't think of better ideas but another possibility is to mark
the slot as dirty when we update the confirm_flush LSN (see
LogicalConfirmReceivedLocation()). However, that would be a bigger
overhead in the running server as it could be a frequent operation and
could lead to more writes.

Yeah I didn't find any better option either at that time. I still think
that forcing persistence on shutdown is the best compromise. If we tried to
always mark the slot as dirty, we would be sure to add regular overhead but
we would probably end up persisting the slot on disk on shutdown anyway
most of the time, so I don't think it would be a good compromise.

My biggest concern was that some switchover scenario might be a bit slower
in some cases, but if that really is a problem it's hard to imagine what
workload would be possible without having to persist them anyway due to
continuous activity needing to be sent just before the shutdown.

On Sat, Aug 19, 2023 at 12:46 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Sat, 19 Aug 2023, 14:16 Amit Kapila, <amit.kapila16@gmail.com> wrote:

The idea discussed in the thread [1] is to always persist logical
slots to disk during the shutdown checkpoint. I have extracted the
patch to achieve the same from that thread and attached it here. This
could lead to some overhead during shutdown (checkpoint) if there are
many slots but it is probably a one-time work.

I couldn't think of better ideas but another possibility is to mark
the slot as dirty when we update the confirm_flush LSN (see
LogicalConfirmReceivedLocation()). However, that would be a bigger
overhead in the running server as it could be a frequent operation and
could lead to more writes.

Yeah I didn't find any better option either at that time. I still think that forcing persistence on shutdown is the best compromise. If we tried to always mark the slot as dirty, we would be sure to add regular overhead but we would probably end up persisting the slot on disk on shutdown anyway most of the time, so I don't think it would be a good compromise.

The other possibility is that we introduce yet another dirty flag for
slots, say dirty_for_shutdown_checkpoint which will be set when we
update confirmed_flush LSN. The flag will be cleared each time we
persist the slot but we won't persist if only this flag is set. We can
then use it during the shutdown checkpoint to decide whether to
persist the slot.

--
With Regards,
Amit Kapila.

On Sun, Aug 20, 2023 at 8:40 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Sat, Aug 19, 2023 at 12:46 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Sat, 19 Aug 2023, 14:16 Amit Kapila, <amit.kapila16@gmail.com> wrote:

The idea discussed in the thread [1] is to always persist logical
slots to disk during the shutdown checkpoint. I have extracted the
patch to achieve the same from that thread and attached it here. This
could lead to some overhead during shutdown (checkpoint) if there are
many slots but it is probably a one-time work.

I couldn't think of better ideas but another possibility is to mark
the slot as dirty when we update the confirm_flush LSN (see
LogicalConfirmReceivedLocation()). However, that would be a bigger
overhead in the running server as it could be a frequent operation and
could lead to more writes.

Yeah I didn't find any better option either at that time. I still think that forcing persistence on shutdown is the best compromise. If we tried to always mark the slot as dirty, we would be sure to add regular overhead but we would probably end up persisting the slot on disk on shutdown anyway most of the time, so I don't think it would be a good compromise.

The other possibility is that we introduce yet another dirty flag for
slots, say dirty_for_shutdown_checkpoint which will be set when we
update confirmed_flush LSN. The flag will be cleared each time we
persist the slot but we won't persist if only this flag is set. We can
then use it during the shutdown checkpoint to decide whether to
persist the slot.

There are already two booleans controlling dirty-ness of slot, dirty
and just_dirty. Adding third will created more confusion.

Another idea is to record the confirm_flush_lsn at the time of
persisting the slot. We can use it in two different ways 1. to mark a
slot dirty and persist if the last confirm_flush_lsn when slot was
persisted was too far from the current confirm_flush_lsn of the slot.
2. at shutdown checkpoint, persist all the slots which have these two
confirm_flush_lsns different.

--
Best Wishes,
Ashutosh Bapat

On Mon, Aug 21, 2023 at 6:36 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Sun, Aug 20, 2023 at 8:40 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

The other possibility is that we introduce yet another dirty flag for
slots, say dirty_for_shutdown_checkpoint which will be set when we
update confirmed_flush LSN. The flag will be cleared each time we
persist the slot but we won't persist if only this flag is set. We can
then use it during the shutdown checkpoint to decide whether to
persist the slot.

There are already two booleans controlling dirty-ness of slot, dirty
and just_dirty. Adding third will created more confusion.

Another idea is to record the confirm_flush_lsn at the time of
persisting the slot. We can use it in two different ways 1. to mark a
slot dirty and persist if the last confirm_flush_lsn when slot was
persisted was too far from the current confirm_flush_lsn of the slot.
2. at shutdown checkpoint, persist all the slots which have these two
confirm_flush_lsns different.

I think using it in the second (2) way sounds advantageous as compared
to storing another dirty flag because this requires us to update
last_persisted_confirm_flush_lsn only while writing the slot info.
OTOH, having a flag dirty_for_shutdown_checkpoint will require us to
update it each time we update confirm_flush_lsn under spinlock at
multiple places. But, I don't see the need of doing what you proposed
in (1) as the use case for it is very minor, basically this may
sometimes help us to avoid decoding after crash recovery.

--
With Regards,
Amit Kapila.

On Tue, Aug 22, 2023 at 9:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Another idea is to record the confirm_flush_lsn at the time of
persisting the slot. We can use it in two different ways 1. to mark a
slot dirty and persist if the last confirm_flush_lsn when slot was
persisted was too far from the current confirm_flush_lsn of the slot.
2. at shutdown checkpoint, persist all the slots which have these two
confirm_flush_lsns different.

I think using it in the second (2) way sounds advantageous as compared
to storing another dirty flag because this requires us to update
last_persisted_confirm_flush_lsn only while writing the slot info.
OTOH, having a flag dirty_for_shutdown_checkpoint will require us to
update it each time we update confirm_flush_lsn under spinlock at
multiple places. But, I don't see the need of doing what you proposed
in (1) as the use case for it is very minor, basically this may
sometimes help us to avoid decoding after crash recovery.

Once we have last_persisted_confirm_flush_lsn, (1) is just an
optimization on top of that. With that we take the opportunity to
persist confirmed_flush_lsn which is much farther than the current
persisted value and thus improving chances of updating restart_lsn and
catalog_xmin faster after a WAL sender restart. We need to keep that
in mind when implementing (2). The problem is if we don't implement
(1) right now, we might just forget to do that small incremental
change in future. My preference is 1. Do both (1) and (2) together 2.
Do (2) first and then (1) as a separate commit. 3. Just implement (2)
if we don't have time at all for first two options.

--
Best Wishes,
Ashutosh Bapat

On Tue, Aug 22, 2023 at 2:56 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, Aug 22, 2023 at 9:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Another idea is to record the confirm_flush_lsn at the time of
persisting the slot. We can use it in two different ways 1. to mark a
slot dirty and persist if the last confirm_flush_lsn when slot was
persisted was too far from the current confirm_flush_lsn of the slot.
2. at shutdown checkpoint, persist all the slots which have these two
confirm_flush_lsns different.

I think using it in the second (2) way sounds advantageous as compared
to storing another dirty flag because this requires us to update
last_persisted_confirm_flush_lsn only while writing the slot info.
OTOH, having a flag dirty_for_shutdown_checkpoint will require us to
update it each time we update confirm_flush_lsn under spinlock at
multiple places. But, I don't see the need of doing what you proposed
in (1) as the use case for it is very minor, basically this may
sometimes help us to avoid decoding after crash recovery.

Once we have last_persisted_confirm_flush_lsn, (1) is just an
optimization on top of that. With that we take the opportunity to
persist confirmed_flush_lsn which is much farther than the current
persisted value and thus improving chances of updating restart_lsn and
catalog_xmin faster after a WAL sender restart. We need to keep that
in mind when implementing (2). The problem is if we don't implement
(1) right now, we might just forget to do that small incremental
change in future. My preference is 1. Do both (1) and (2) together 2.
Do (2) first and then (1) as a separate commit. 3. Just implement (2)
if we don't have time at all for first two options.

I prefer one of (2) or (3). Anyway, it is better to do that
optimization (persist confirm_flush_lsn at a regular interval) as a
separate patch as we need to test and prove its value separately.

--
With Regards,
Amit Kapila.

On Tue, Aug 22, 2023 at 3:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Once we have last_persisted_confirm_flush_lsn, (1) is just an
optimization on top of that. With that we take the opportunity to
persist confirmed_flush_lsn which is much farther than the current
persisted value and thus improving chances of updating restart_lsn and
catalog_xmin faster after a WAL sender restart. We need to keep that
in mind when implementing (2). The problem is if we don't implement
(1) right now, we might just forget to do that small incremental
change in future. My preference is 1. Do both (1) and (2) together 2.
Do (2) first and then (1) as a separate commit. 3. Just implement (2)
if we don't have time at all for first two options.

I prefer one of (2) or (3). Anyway, it is better to do that
optimization (persist confirm_flush_lsn at a regular interval) as a
separate patch as we need to test and prove its value separately.

Fine with me.

--
Best Wishes,
Ashutosh Bapat

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]/messages/by-id/CAHut+Ptb=ZYTM_awoLy3sJ5m9Oxe=JYn6Gve5rSW9cUdThpsVA@mail.gmail.com.

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]: /messages/by-id/CAHut+Ptb=ZYTM_awoLy3sJ5m9Oxe=JYn6Gve5rSW9cUdThpsVA@mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, 22 Aug 2023 at 15:42, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Aug 22, 2023 at 2:56 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, Aug 22, 2023 at 9:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Another idea is to record the confirm_flush_lsn at the time of
persisting the slot. We can use it in two different ways 1. to mark a
slot dirty and persist if the last confirm_flush_lsn when slot was
persisted was too far from the current confirm_flush_lsn of the slot.
2. at shutdown checkpoint, persist all the slots which have these two
confirm_flush_lsns different.

I think using it in the second (2) way sounds advantageous as compared
to storing another dirty flag because this requires us to update
last_persisted_confirm_flush_lsn only while writing the slot info.
OTOH, having a flag dirty_for_shutdown_checkpoint will require us to
update it each time we update confirm_flush_lsn under spinlock at
multiple places. But, I don't see the need of doing what you proposed
in (1) as the use case for it is very minor, basically this may
sometimes help us to avoid decoding after crash recovery.

Once we have last_persisted_confirm_flush_lsn, (1) is just an
optimization on top of that. With that we take the opportunity to
persist confirmed_flush_lsn which is much farther than the current
persisted value and thus improving chances of updating restart_lsn and
catalog_xmin faster after a WAL sender restart. We need to keep that
in mind when implementing (2). The problem is if we don't implement
(1) right now, we might just forget to do that small incremental
change in future. My preference is 1. Do both (1) and (2) together 2.
Do (2) first and then (1) as a separate commit. 3. Just implement (2)
if we don't have time at all for first two options.

I prefer one of (2) or (3). Anyway, it is better to do that
optimization (persist confirm_flush_lsn at a regular interval) as a
separate patch as we need to test and prove its value separately.

Here is a patch to persist to disk logical slots during a shutdown
checkpoint if the updated confirmed_flush_lsn has not yet been
persisted.

Regards,
Vignesh

Dear Vignesh,

Here is a patch to persist to disk logical slots during a shutdown
checkpoint if the updated confirmed_flush_lsn has not yet been
persisted.

Thanks for making the patch with different approach! Here are comments.

01. RestoreSlotFromDisk

```
slot->candidate_xmin_lsn = InvalidXLogRecPtr;
slot->candidate_restart_lsn = InvalidXLogRecPtr;
slot->candidate_restart_valid = InvalidXLogRecPtr;
+ slot->last_persisted_confirmed_flush = InvalidXLogRecPtr;
```

last_persisted_confirmed_flush was set to InvalidXLogRecPtr, but isn't it better
to use cp.slotdata. confirmed_flush? Assuming that the server is shut down immediately,
your patch forces to save.

02. t/002_always_persist.pl

The original author of the patch is me, but I found that the test could pass
without your patch. This is because pg_logical_slot_get_changes()->
pg_logical_slot_get_changes_guts(confirm = true) always mark the slot as dirty.
IIUC we must use the logical replication system to verify the persistence.
Attached test can pass only when patch is applied.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, 23 Aug 2023 at 14:21, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Vignesh,

Here is a patch to persist to disk logical slots during a shutdown
checkpoint if the updated confirmed_flush_lsn has not yet been
persisted.

Thanks for making the patch with different approach! Here are comments.

01. RestoreSlotFromDisk

```
slot->candidate_xmin_lsn = InvalidXLogRecPtr;
slot->candidate_restart_lsn = InvalidXLogRecPtr;
slot->candidate_restart_valid = InvalidXLogRecPtr;
+ slot->last_persisted_confirmed_flush = InvalidXLogRecPtr;
```

last_persisted_confirmed_flush was set to InvalidXLogRecPtr, but isn't it better
to use cp.slotdata. confirmed_flush? Assuming that the server is shut down immediately,
your patch forces to save.

02. t/002_always_persist.pl

The original author of the patch is me, but I found that the test could pass
without your patch. This is because pg_logical_slot_get_changes()->
pg_logical_slot_get_changes_guts(confirm = true) always mark the slot as dirty.
IIUC we must use the logical replication system to verify the persistence.
Attached test can pass only when patch is applied.

Here are few other comments that I noticed:

1) I too noticed that the test passes both with and without patch:
diff --git a/contrib/test_decoding/meson.build
b/contrib/test_decoding/meson.build
index 7b05cc25a3..12afb9ea8c 100644
--- a/contrib/test_decoding/meson.build
+++ b/contrib/test_decoding/meson.build
@@ -72,6 +72,7 @@ tests += {
   'tap': {
     'tests': [
       't/001_repl_stats.pl',
+      't/002_always_persist.pl',

2) change checkPoint to checkpoint:
2.a) checkPoint should be checkpoint to maintain consistency across the file:
+# Shutdown the node once to do shutdown checkpoint
+$node->stop();
+
+# 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;

2.b) similarly here:
+die "No checkPoint in control file found\n"
+  unless defined($latest_checkpoint);

2.c) similarly here too:
+# Compare confirmed_flush_lsn and checkPoint
+ok($confirmed_flush eq $latest_checkpoint,
+       "Check confirmed_flush is same as latest checkpoint location");

3) change checkpoint to "Latest checkpoint location":
3.a) We should change "No checkPoint in control file found\n" to:
"Latest checkpoint location not found in control file\n" as there are
many checkpoint entries in control data

+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);

3.b) We should change "Fetch checkPoint from the control file itself" to:
"Fetch Latest checkpoint location from the control file"

+# 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)
+{

Regards,
Vignesh

On Wed, 23 Aug 2023 at 14:21, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Vignesh,

Here is a patch to persist to disk logical slots during a shutdown
checkpoint if the updated confirmed_flush_lsn has not yet been
persisted.

Thanks for making the patch with different approach! Here are comments.

01. RestoreSlotFromDisk

```
slot->candidate_xmin_lsn = InvalidXLogRecPtr;
slot->candidate_restart_lsn = InvalidXLogRecPtr;
slot->candidate_restart_valid = InvalidXLogRecPtr;
+ slot->last_persisted_confirmed_flush = InvalidXLogRecPtr;
```

last_persisted_confirmed_flush was set to InvalidXLogRecPtr, but isn't it better
to use cp.slotdata. confirmed_flush? Assuming that the server is shut down immediately,
your patch forces to save.

Modified

02. t/002_always_persist.pl

The original author of the patch is me, but I found that the test could pass
without your patch. This is because pg_logical_slot_get_changes()->
pg_logical_slot_get_changes_guts(confirm = true) always mark the slot as dirty.
IIUC we must use the logical replication system to verify the persistence.
Attached test can pass only when patch is applied.

Update the test based on your another_test with slight modifications.

Attached v4 version patch has the changes for the same.

Regards,
Vignesh

On Sat, 19 Aug 2023 at 11:53, Amit Kapila <amit.kapila16@gmail.com> wrote:

It's entirely possible for a logical slot to have a confirmed_flush
LSN higher than the last value saved on disk while not being marked as
dirty. It's currently not a problem to lose that value during a clean
shutdown / restart cycle but to support the upgrade of logical slots
[1] (see latest patch at [2]), we seem to rely on that value being
properly persisted to disk. During the upgrade, we need to verify that
all the data prior to shudown_checkpoint for the logical slots has
been consumed, otherwise, the downstream may miss some data. Now, to
ensure the same, we are planning to compare the confirm_flush LSN
location with the latest shudown_checkpoint location which means that
the confirm_flush LSN should be updated after restart.

I think this is inefficient even without an upgrade because, after the
restart, this may lead to decoding some data again. Say, we process
some transactions for which we didn't send anything downstream (the
changes got filtered) but the confirm_flush LSN is updated due to
keepalives. As we don't flush the latest value of confirm_flush LSN,
it may lead to processing the same changes again.

I was able to test and verify that we were not processing the same
changes again.
Note: The 0001-Add-logs-to-skip-transaction-filter-insert-operation.patch
has logs to print if a decode transaction is skipped and also a log to
mention if any operation is filtered.
The test.sh script has the steps for a) setting up logical replication
for a table b) perform insert on table that need to be published (this
will be replicated to the subscriber) c) perform insert on a table
that will not be published (this insert will be filtered, it will not
be replicated) d) sleep for 5 seconds e) stop the server f) start the
server
I used the following steps, do the following in HEAD:
a) Apply 0001-Add-logs-to-skip-transaction-filter-insert-operation.patch
patch in Head and build the binaries b) execute test.sh c) view N1.log
file to see that the insert operations were filtered again by seeing
the following logs:
LOG: Filter insert for table tbl2
...
===restart===
...
LOG: Skipping transaction 0/156AD10 as start decode at is greater 0/156AE40
...
LOG: Filter insert for table tbl2

We can see that the insert operations on tbl2 which was filtered
before server was stopped is again filtered after restart too in HEAD.

Lets see that the same changes were not processed again with patch:
a) Apply v4-0001-Persist-to-disk-logical-slots-during-a-shutdown-c.patch
from [1]/messages/by-id/CALDaNm0VrAt24e2FxbOX6eJQ-G_tZ0gVpsFBjzQM99NxG0hZfg@mail.gmail.com also apply
0001-Add-logs-to-skip-transaction-filter-insert-operation.patch patch
and build the binaries b) execute test.sh c) view N1.log file to see
that the insert operations were skipped after restart of server by
seeing the following logs:
LOG: Filter insert for table tbl2
...
===restart===
...
Skipping transaction 0/156AD10 as start decode at is greater 0/156AFB0
...
Skipping transaction 0/156AE80 as start decode at is greater 0/156AFB0

We can see that the insert operations on tbl2 are not processed again
after restart with the patch.

[1]: /messages/by-id/CALDaNm0VrAt24e2FxbOX6eJQ-G_tZ0gVpsFBjzQM99NxG0hZfg@mail.gmail.com

Regards,
Vignesh

On Fri, 25 Aug 2023 at 17:40, vignesh C <vignesh21@gmail.com> wrote:

On Sat, 19 Aug 2023 at 11:53, Amit Kapila <amit.kapila16@gmail.com> wrote:

It's entirely possible for a logical slot to have a confirmed_flush
LSN higher than the last value saved on disk while not being marked as
dirty. It's currently not a problem to lose that value during a clean
shutdown / restart cycle but to support the upgrade of logical slots
[1] (see latest patch at [2]), we seem to rely on that value being
properly persisted to disk. During the upgrade, we need to verify that
all the data prior to shudown_checkpoint for the logical slots has
been consumed, otherwise, the downstream may miss some data. Now, to
ensure the same, we are planning to compare the confirm_flush LSN
location with the latest shudown_checkpoint location which means that
the confirm_flush LSN should be updated after restart.

I think this is inefficient even without an upgrade because, after the
restart, this may lead to decoding some data again. Say, we process
some transactions for which we didn't send anything downstream (the
changes got filtered) but the confirm_flush LSN is updated due to
keepalives. As we don't flush the latest value of confirm_flush LSN,
it may lead to processing the same changes again.

I was able to test and verify that we were not processing the same
changes again.
Note: The 0001-Add-logs-to-skip-transaction-filter-insert-operation.patch
has logs to print if a decode transaction is skipped and also a log to
mention if any operation is filtered.
The test.sh script has the steps for a) setting up logical replication
for a table b) perform insert on table that need to be published (this
will be replicated to the subscriber) c) perform insert on a table
that will not be published (this insert will be filtered, it will not
be replicated) d) sleep for 5 seconds e) stop the server f) start the
server
I used the following steps, do the following in HEAD:
a) Apply 0001-Add-logs-to-skip-transaction-filter-insert-operation.patch
patch in Head and build the binaries b) execute test.sh c) view N1.log
file to see that the insert operations were filtered again by seeing
the following logs:
LOG: Filter insert for table tbl2
...
===restart===
...
LOG: Skipping transaction 0/156AD10 as start decode at is greater 0/156AE40
...
LOG: Filter insert for table tbl2

We can see that the insert operations on tbl2 which was filtered
before server was stopped is again filtered after restart too in HEAD.

Lets see that the same changes were not processed again with patch:
a) Apply v4-0001-Persist-to-disk-logical-slots-during-a-shutdown-c.patch
from [1] also apply
0001-Add-logs-to-skip-transaction-filter-insert-operation.patch patch
and build the binaries b) execute test.sh c) view N1.log file to see
that the insert operations were skipped after restart of server by
seeing the following logs:
LOG: Filter insert for table tbl2
...
===restart===
...
Skipping transaction 0/156AD10 as start decode at is greater 0/156AFB0
...
Skipping transaction 0/156AE80 as start decode at is greater 0/156AFB0

We can see that the insert operations on tbl2 are not processed again
after restart with the patch.

Here is another way to test using pg_replslotdata approach that was
proposed earlier at [1]/messages/by-id/CALj2ACW0rV5gWK8A3m6_X62qH+Vfaq5hznC=i0R5Wojt5+yhyw@mail.gmail.com.
I have rebased this on top of HEAD and the v5 version for the same is attached.

We can use the same test as test.sh shared at [2]/messages/by-id/CALDaNm2BboFuFVYxyzP4wkv7=8+_TwsD+ugyGhtibTSF4m4XRg@mail.gmail.com.
When executed with HEAD, it was noticed that confirmed_flush points to
WAL location before both the transaction:
slot_name slot_type datoid persistency xmin catalog_xmin
restart_lsn confirmed_flush two_phase_at two_phase
plugin
--------- --------- ------ ---------- ----
----------- ----------- ---------------
------------ --------- ------
sub logical 5 persistent 0
735 0/1531E28 0/1531E60 0/0
0 pgoutput

WAL record information generated using pg_walinspect for various
records at and after confirmed_flush WAL 0/1531E60:
row_number | start_lsn | end_lsn | prev_lsn | xid |
resource_manager | record_type | record_length |
main_data_length | fpi_length |
description
|
block_ref
------------+-----------+-----------+-----------+-----+------------------+---------------------+---------------+------------------+------------+-------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------+---------------------------------------------
1 | 0/1531E60 | 0/1531EA0 | 0/1531E28 | 0 | Heap2
| PRUNE | 57 | 9 |
0 | snapshotConflictHorizon: 0, nredirected: 0, ndead: 1, nunused: 0,
redirected: [], dead: [1]/messages/by-id/CALj2ACW0rV5gWK8A3m6_X62qH+Vfaq5hznC=i0R5Wojt5+yhyw@mail.gmail.com, unused: []
|
blkref #0: rel 1663/5/1255 fork main blk 58
2 | 0/1531EA0 | 0/1531EE0 | 0/1531E60 | 735 | Heap
| INSERT+INIT | 59 | 3 |
0 | off: 1, flags: 0x08

|
blkref #0: rel 1663/5/16384 fork main blk 0
3 | 0/1531EE0 | 0/1531F20 | 0/1531EA0 | 735 | Heap
| INSERT | 59 | 3 |
0 | off: 2, flags: 0x08

|
blkref #0: rel 1663/5/16384 fork main blk 0
4 | 0/1531F20 | 0/1531F60 | 0/1531EE0 | 735 | Heap
| INSERT | 59 | 3 |
0 | off: 3, flags: 0x08

|
blkref #0: rel 1663/5/16384 fork main blk 0
5 | 0/1531F60 | 0/1531FA0 | 0/1531F20 | 735 | Heap
| INSERT | 59 | 3 |
0 | off: 4, flags: 0x08

|
blkref #0: rel 1663/5/16384 fork main blk 0
6 | 0/1531FA0 | 0/1531FE0 | 0/1531F60 | 735 | Heap
| INSERT | 59 | 3 |
0 | off: 5, flags: 0x08

|
blkref #0: rel 1663/5/16384 fork main blk 0
7 | 0/1531FE0 | 0/1532028 | 0/1531FA0 | 735 | Transaction
| COMMIT | 46 | 20 |
0 | 2023-08-27 23:22:17.161215+05:30

|
8 | 0/1532028 | 0/1532068 | 0/1531FE0 | 736 | Heap
| INSERT+INIT | 59 | 3 |
0 | off: 1, flags: 0x08

|
blkref #0: rel 1663/5/16387 fork main blk 0
9 | 0/1532068 | 0/15320A8 | 0/1532028 | 736 | Heap
| INSERT | 59 | 3 |
0 | off: 2, flags: 0x08

|
blkref #0: rel 1663/5/16387 fork main blk 0
10 | 0/15320A8 | 0/15320E8 | 0/1532068 | 736 | Heap
| INSERT | 59 | 3 |
0 | off: 3, flags: 0x08

|
blkref #0: rel 1663/5/16387 fork main blk 0
11 | 0/15320E8 | 0/1532128 | 0/15320A8 | 736 | Heap
| INSERT | 59 | 3 |
0 | off: 4, flags: 0x08

|
blkref #0: rel 1663/5/16387 fork main blk 0
12 | 0/1532128 | 0/1532168 | 0/15320E8 | 736 | Heap
| INSERT | 59 | 3 |
0 | off: 5, flags: 0x08

|
blkref #0: rel 1663/5/16387 fork main blk 0
13 | 0/1532168 | 0/1532198 | 0/1532128 | 736 | Transaction
| COMMIT | 46 | 20 |
0 | 2023-08-27 23:22:17.174756+05:30

|
14 | 0/1532198 | 0/1532210 | 0/1532168 | 0 | XLOG
| CHECKPOINT_SHUTDOWN | 114 | 88 |
0 | redo 0/1532198; tli 1; prev tli 1; fpw true; xid 0:737; oid 16399;
multi 1; offset 0; oldest xid 723 in DB 1; oldest multi 1 in DB 1;
oldest/newest commit timestamp xid: 0/0; oldest running xid 0;
shutdown |

Whereas the same test executed with the patch applied shows that
confirmed_flush points to CHECKPOINT_SHUTDOWN record:
slot_name slot_type datoid persistency xmin catalog_xmin
restart_lsn confirmed_flush two_phase_at two_phase
plugin
--------- --------- ------ ----------- ---
----------- ----------- ---------------
----------- --------- ------
sub logical 5 persistent 0 735
0/1531E28 0/1532198 0/0 0
pgoutput

WAL record information generated using pg_walinspect for various
records at and after confirmed_flush WAL 0/1532198:
row_number | start_lsn | end_lsn | prev_lsn | xid |
resource_manager | record_type | record_length |
main_data_length | fpi_length |
description
|
block_ref
------------+-----------+-----------+-----------+-----+------------------+---------------------+---------------+------------------+------------+-------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------+-----------
1 | 0/1532198 | 0/1532210 | 0/1532168 | 0 | XLOG
| CHECKPOINT_SHUTDOWN | 114 | 88 |
0 | redo 0/1532198; tli 1; prev tli 1; fpw true; xid 0:737; oid 16399;
multi 1; offset 0; oldest xid 723 in DB 1; oldest multi 1 in DB 1;
oldest/newest commit timestamp xid: 0/0; oldest running xid 0;
shutdown |
(1 row)

[1]: /messages/by-id/CALj2ACW0rV5gWK8A3m6_X62qH+Vfaq5hznC=i0R5Wojt5+yhyw@mail.gmail.com
[2]: /messages/by-id/CALDaNm2BboFuFVYxyzP4wkv7=8+_TwsD+ugyGhtibTSF4m4XRg@mail.gmail.com

Regards,
Vignesh

Dear hackers,

I also tested for logical slots on the physical standby. PSA the script.
confirmed_flush_lsn for such slots were successfully persisted.

# Topology

In this test nodes are connected each other.

node1 --(physical replication)-->node2--(logical replication)-->node3

# Test method

An attached script did following steps

1. constructed above configurations
2. Inserted data on node1
3. read confirmed_flush_lsn on node2 (a)
4. restarted node2
5. read confirmed_flush_lsn again on node2 (b)
6. compare (a) and (b)

# result

Before patching, (a) and (b) were different value, which meant that logical
slots on physical standby were not saved at shutdown.

```
slot_name | confirmed_flush_lsn
-----------+---------------------
sub | 0/30003E8
(1 row)

waiting for server to shut down.... done
server stopped
waiting for server to start.... done
server started
slot_name | confirmed_flush_lsn
-----------+---------------------
sub | 0/30000D8
(1 row)
```

After patching, (a) and (b) became the same value. The v4 patch worked well even
if the node is physical standby.

```
slot_name | confirmed_flush_lsn
-----------+---------------------
sub | 0/30003E8
(1 row)

waiting for server to shut down.... done
server stopped
waiting for server to start.... done
server started
slot_name | confirmed_flush_lsn
-----------+---------------------
sub | 0/30003E8
(1 row)
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Aug 24, 2023 at 11:44 AM vignesh C <vignesh21@gmail.com> wrote:

The patch looks mostly good to me. I have made minor changes which are
as follows: (a) removed the autovacuum =off and
wal_receiver_status_interval = 0 setting as those doesn't seem to be
required for the test; (b) changed a few comments and variable names
in the code and test;

Shall we change the test file name from always_persist to
save_logical_slots_shutdown and move to recovery/t/ as this test is
about verification after the restart of the server?

--
With Regards,
Amit Kapila.

On Mon, 28 Aug 2023 at 18:56, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 24, 2023 at 11:44 AM vignesh C <vignesh21@gmail.com> wrote:

The patch looks mostly good to me. I have made minor changes which are
as follows: (a) removed the autovacuum =off and
wal_receiver_status_interval = 0 setting as those doesn't seem to be
required for the test; (b) changed a few comments and variable names
in the code and test;

Shall we change the test file name from always_persist to
save_logical_slots_shutdown and move to recovery/t/ as this test is
about verification after the restart of the server?

That makes sense. The attached v6 version has the changes for the
same, apart from this I have also fixed a) pgindent issues b) perltidy
issues c) one variable change (flush_lsn_changed to
confirmed_flush_has_changed) d) corrected few comments in the test
file. Thanks to Peter for providing few offline comments.

Regards,
Vignesh

On Tue, Aug 29, 2023 at 10:16 AM vignesh C <vignesh21@gmail.com> wrote:

That makes sense. The attached v6 version has the changes for the
same, apart from this I have also fixed a) pgindent issues b) perltidy
issues c) one variable change (flush_lsn_changed to
confirmed_flush_has_changed) d) corrected few comments in the test
file. Thanks to Peter for providing few offline comments.

The latest version looks good to me. Julien, Ashutosh, and others,
unless you have more comments or suggestions, I would like to push
this in a day or two.

--
With Regards,
Amit Kapila.

On Tue, Aug 29, 2023 at 2:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Aug 29, 2023 at 10:16 AM vignesh C <vignesh21@gmail.com> wrote:

That makes sense. The attached v6 version has the changes for the
same, apart from this I have also fixed a) pgindent issues b) perltidy
issues c) one variable change (flush_lsn_changed to
confirmed_flush_has_changed) d) corrected few comments in the test
file. Thanks to Peter for providing few offline comments.

The latest version looks good to me. Julien, Ashutosh, and others,
unless you have more comments or suggestions, I would like to push
this in a day or two.

I am looking at it. If you can wait till the end of the week, that
will be great.

--
Best Wishes,
Ashutosh Bapat

Hi,

On Tue, Aug 29, 2023 at 02:21:15PM +0530, Amit Kapila wrote:

On Tue, Aug 29, 2023 at 10:16 AM vignesh C <vignesh21@gmail.com> wrote:

That makes sense. The attached v6 version has the changes for the
same, apart from this I have also fixed a) pgindent issues b) perltidy
issues c) one variable change (flush_lsn_changed to
confirmed_flush_has_changed) d) corrected few comments in the test
file. Thanks to Peter for providing few offline comments.

The latest version looks good to me. Julien, Ashutosh, and others,
unless you have more comments or suggestions, I would like to push
this in a day or two.

Unfortunately I'm currently swamped with some internal escalations so I
couldn't keep up closely with the latest activity here.

I think I recall that you wanted to
change the timing at which logical slots are shutdown, I'm assuming that this
change won't lead to always have a difference between the LSN and latest
persisted LSN being different? Otherwise saving the latest persisted LSN to
try to avoid persisting again all logical slots on shutdown seems reasonable to
me.

On Wed, Aug 30, 2023 at 9:03 AM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Tue, Aug 29, 2023 at 02:21:15PM +0530, Amit Kapila wrote:

On Tue, Aug 29, 2023 at 10:16 AM vignesh C <vignesh21@gmail.com> wrote:

That makes sense. The attached v6 version has the changes for the
same, apart from this I have also fixed a) pgindent issues b) perltidy
issues c) one variable change (flush_lsn_changed to
confirmed_flush_has_changed) d) corrected few comments in the test
file. Thanks to Peter for providing few offline comments.

The latest version looks good to me. Julien, Ashutosh, and others,
unless you have more comments or suggestions, I would like to push
this in a day or two.

Unfortunately I'm currently swamped with some internal escalations so I
couldn't keep up closely with the latest activity here.

I think I recall that you wanted to
change the timing at which logical slots are shutdown, I'm assuming that this
change won't lead to always have a difference between the LSN and latest
persisted LSN being different?

I think here by LSN you are referring to confirmed_flush LSN. If so,
this doesn't create any new difference between the values for the
confirmed_flush LSN in memory and in disk. We just remember the last
persisted value to avoid writes of slots at shutdown time.

Otherwise saving the latest persisted LSN to
try to avoid persisting again all logical slots on shutdown seems reasonable to
me.

Thanks for responding.

--
With Regards,
Amit Kapila.

On Tue, Aug 29, 2023 at 5:40 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

I am looking at it. If you can wait till the end of the week, that
will be great.

     /*
      * Successfully wrote, unset dirty bit, unless somebody dirtied again
-     * already.
+     * already and remember the confirmed_flush LSN value.
      */
     SpinLockAcquire(&slot->mutex);
     if (!slot->just_dirtied)
         slot->dirty = false;
+    slot->last_saved_confirmed_flush = slot->data.confirmed_flush;

If the slot->data.confirmed_flush gets updated concurrently between copying it
to be written to the disk and when it's written to last_saved_confirmed_flush,
we will miss one update. I think we need to update last_saved_confirmed_flush
based on the cp.slotdata.confirmed_flush rather than
slot->data.confirmed_flush.

We are setting last_saved_confirmed_flush for all types of slots but using it
only when the slot is logical. Should we also set it only for logical slots?

/* first check whether there's something to write out */
SpinLockAcquire(&slot->mutex);
was_dirty = slot->dirty;
slot->just_dirtied = false;
+ confirmed_flush_has_changed = (slot->data.confirmed_flush !=
slot->last_saved_confirmed_flush);

The confirmed_flush LSN should always move forward, otherwise there may not be
enough WAL retained for the slot to work. I am wondering whether we should take
an opportunity to make sure
Assert(slot->data.confirmed_flush <= slot->last_saved_confirmed_flush)

-    /* and don't do anything if there's nothing to write */
-    if (!was_dirty)
+    /* Don't do anything if there's nothing to write. See ReplicationSlot. */
+    if (!was_dirty &&
+        !(is_shutdown && SlotIsLogical(slot) && confirmed_flush_has_changed))

Rather than complicating this condition, I wonder whether it's better to just
set was_dirty = true when is_shutdown && SlotIsLogical(slot) &&
confirmed_flush_has_changed) or even slot->dirty = true. See also the note at
the end of the email.

+
+    /*
+     * We won't ensure that the slot is persisted after the confirmed_flush
+     * LSN is updated as that could lead to frequent writes.  However, we need
+     * to ensure that we do persist the slots at the time of shutdown whose
+     * confirmed_flush LSN is changed since we last saved the slot to disk.
+     * This will help in avoiding retreat of the confirmed_flush LSN after
+     * restart.  This variable is used to track the last saved confirmed_flush
+     * LSN value.
+     */

This comment makes more sense in SaveSlotToPath() than here. We may decide to
use last_saved_confirmed_flush for something else in future.
+
+sub compare_confirmed_flush
+{
+    my ($node, $confirmed_flush_from_log) = @_;
+
+    # Fetch Latest checkpoint location from the control file
+    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 "Latest checkpoint location not found in control file\n"
+      unless defined($latest_checkpoint);
+
+    # Is it same as the value read from log?
+    ok( $latest_checkpoint eq $confirmed_flush_from_log,
+        "Check that the slot's confirmed_flush LSN is the same as the
latest_checkpoint location"
+    );

This function assumes that the subscriber will receive and confirm WAL upto
checkpoint location and publisher's WAL sender will update it in the slot.
Where is the code to ensure that? Does the WAL sender process wait for
checkpoint
LSN to be confirmed when shutting down?
+
+# Restart the publisher to ensure that the slot will be persisted if required
+$node_publisher->restart();

Can we add this test comparing LSNs after publisher restart, to an existing
test itself - like basic replication. That's the only extra thing that this
test does beyond usual replication stuff.

+
+# Wait until the walsender creates decoding context
+$node_publisher->wait_for_log(
+    qr/Streaming transactions committing after
([A-F0-9]+\/[A-F0-9]+), reading WAL from ([A-F0-9]+\/[A-F0-9]+)./,
+    $offset);
+
+# Extract confirmed_flush from the logfile
+my $log_contents = slurp_file($node_publisher->logfile, $offset);
+$log_contents =~
+  qr/Streaming transactions committing after ([A-F0-9]+\/[A-F0-9]+),
reading WAL from ([A-F0-9]+\/[A-F0-9]+)./
+  or die "could not get confirmed_flush_lsn";

Why are we figuring out the LSN from the log file? Is it not available from
pg_replication_slots view? If we do so, I think this test will fail is the slot
gets written after the restart because of concurrent activity on the publisher
(like autovacuum, or other things that cause empty transaction to be
replicated) and subscriber. A very rare chance but not 0 probability one. I
think we should shut down subscriber, restart publisher and then make this
check based on the contents of the replication slot instead of server log.
Shutting down subscriber will ensure that the subscriber won't send any new
confirmed flush location to the publisher after restart.

All the places which call ReplicationSlotSave() mark the slot as dirty. All
the places where SaveSlotToPath() is called, the slot is marked dirty except
when calling from CheckPointReplicationSlots(). So I am wondering whether we
should be marking the slot dirty in CheckPointReplicationSlots() and avoid
passing down is_shutdown flag to SaveSlotToPath().

Unrelated to this patch, I noticed that all the callers of SaveSlotToPath()
have the same code to craft replication slot file path. I wonder if that needs
to be macro'ised or added to some common function or to be pushed into
SaveSlotToPath() itself to make sure that any changes to the path in future are
consistent for all callers of SaveSlotToPath(). Interestingly slot->data.name
is accessed without a lock here. Name of the slot does not change after
creation so this isn't a problem right now. But generally against the principle
of accessing data protected by a mutex.

--
Best Wishes,
Ashutosh Bapat

On Wed, Aug 30, 2023 at 6:33 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, Aug 29, 2023 at 5:40 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

I am looking at it. If you can wait till the end of the week, that
will be great.

/*
* Successfully wrote, unset dirty bit, unless somebody dirtied again
-     * already.
+     * already and remember the confirmed_flush LSN value.
*/
SpinLockAcquire(&slot->mutex);
if (!slot->just_dirtied)
slot->dirty = false;
+    slot->last_saved_confirmed_flush = slot->data.confirmed_flush;

If the slot->data.confirmed_flush gets updated concurrently between copying it
to be written to the disk and when it's written to last_saved_confirmed_flush,
we will miss one update. I think we need to update last_saved_confirmed_flush
based on the cp.slotdata.confirmed_flush rather than
slot->data.confirmed_flush.

Yeah, this appears to be a problem.

We are setting last_saved_confirmed_flush for all types of slots but using it
only when the slot is logical. Should we also set it only for logical slots?

We can do that but not sure if there is any advantage of it other than
adding extra condition. BTW, won't even confirmed_flush LSN be used
only for logical slots?

/* first check whether there's something to write out */
SpinLockAcquire(&slot->mutex);
was_dirty = slot->dirty;
slot->just_dirtied = false;
+ confirmed_flush_has_changed = (slot->data.confirmed_flush !=
slot->last_saved_confirmed_flush);

The confirmed_flush LSN should always move forward, otherwise there may not be
enough WAL retained for the slot to work. I am wondering whether we should take
an opportunity to make sure
Assert(slot->data.confirmed_flush <= slot->last_saved_confirmed_flush)

Theoretically, what you are saying makes sense to me but we don't have
such a protection while updating the confirmed_flush LSN. It would be
better to first add such a protection for confirmed_flush LSN update
as a separate patch.

-    /* and don't do anything if there's nothing to write */
-    if (!was_dirty)
+    /* Don't do anything if there's nothing to write. See ReplicationSlot. */
+    if (!was_dirty &&
+        !(is_shutdown && SlotIsLogical(slot) && confirmed_flush_has_changed))

Rather than complicating this condition, I wonder whether it's better to just
set was_dirty = true when is_shutdown && SlotIsLogical(slot) &&
confirmed_flush_has_changed) or even slot->dirty = true.

I think it is better to keep the slot's dirty property separate. But
we can introduce another variable that can be the result of both
was_dirty and other checks together, however, that doesn't seem much
better than the current code.

+
+    /*
+     * We won't ensure that the slot is persisted after the confirmed_flush
+     * LSN is updated as that could lead to frequent writes.  However, we need
+     * to ensure that we do persist the slots at the time of shutdown whose
+     * confirmed_flush LSN is changed since we last saved the slot to disk.
+     * This will help in avoiding retreat of the confirmed_flush LSN after
+     * restart.  This variable is used to track the last saved confirmed_flush
+     * LSN value.
+     */

This comment makes more sense in SaveSlotToPath() than here. We may decide to
use last_saved_confirmed_flush for something else in future.

I have kept it here because it contains some information that is not
specific to SaveSlotToPath. So, it seems easier to follow the whole
theory if we keep it at the central place in the structure and then
add the reference wherever required but I am fine if you and others
feel strongly about moving this to SaveSlotToPath().

+
+sub compare_confirmed_flush
+{
+    my ($node, $confirmed_flush_from_log) = @_;
+
+    # Fetch Latest checkpoint location from the control file
+    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 "Latest checkpoint location not found in control file\n"
+      unless defined($latest_checkpoint);
+
+    # Is it same as the value read from log?
+    ok( $latest_checkpoint eq $confirmed_flush_from_log,
+        "Check that the slot's confirmed_flush LSN is the same as the
latest_checkpoint location"
+    );

This function assumes that the subscriber will receive and confirm WAL upto
checkpoint location and publisher's WAL sender will update it in the slot.
Where is the code to ensure that? Does the WAL sender process wait for
checkpoint
LSN to be confirmed when shutting down?

Note, that we need to compare if all the WAL before the
shutdown_checkpoint WAL record is sent. Before the clean shutdown, we
do ensure that all the pending WAL is confirmed back. See the use of
WalSndDone() in WalSndLoop().

+
+# Restart the publisher to ensure that the slot will be persisted if required
+$node_publisher->restart();

Can we add this test comparing LSNs after publisher restart, to an existing
test itself - like basic replication. That's the only extra thing that this
test does beyond usual replication stuff.

As this is a test after the restart of the server, I thought to keep
it with recovery tests. However, I think once the upgrade (of
publisher nodes) patch is ready, we should keep this test with those
tests or somehow merge it with those tests but till that patch is
ready, let's keep this as a separate test.

+
+# Wait until the walsender creates decoding context
+$node_publisher->wait_for_log(
+    qr/Streaming transactions committing after
([A-F0-9]+\/[A-F0-9]+), reading WAL from ([A-F0-9]+\/[A-F0-9]+)./,
+    $offset);
+
+# Extract confirmed_flush from the logfile
+my $log_contents = slurp_file($node_publisher->logfile, $offset);
+$log_contents =~
+  qr/Streaming transactions committing after ([A-F0-9]+\/[A-F0-9]+),
reading WAL from ([A-F0-9]+\/[A-F0-9]+)./
+  or die "could not get confirmed_flush_lsn";

Why are we figuring out the LSN from the log file? Is it not available from
pg_replication_slots view? If we do so, I think this test will fail is the slot
gets written after the restart because of concurrent activity on the publisher
(like autovacuum, or other things that cause empty transaction to be
replicated) and subscriber. A very rare chance but not 0 probability one.

Yes, that is a possibility.

I
think we should shut down subscriber, restart publisher and then make this
check based on the contents of the replication slot instead of server log.
Shutting down subscriber will ensure that the subscriber won't send any new
confirmed flush location to the publisher after restart.

But if we shutdown the subscriber before the publisher there is no
guarantee that the publisher has sent all outstanding logs up to the
shutdown checkpoint record (i.e., the latest record). Such a guarantee
can only be there if we do a clean shutdown of the publisher before
the subscriber.

All the places which call ReplicationSlotSave() mark the slot as dirty. All
the places where SaveSlotToPath() is called, the slot is marked dirty except
when calling from CheckPointReplicationSlots(). So I am wondering whether we
should be marking the slot dirty in CheckPointReplicationSlots() and avoid
passing down is_shutdown flag to SaveSlotToPath().

I feel that will add another spinlock acquire/release pair without
much benefit. Sure, it may not be performance-sensitive but still
adding another pair of lock/release doesn't seem like a better idea.

--
With Regards,
Amit Kapila.

On Thu, Aug 31, 2023 at 12:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

+
+    /*
+     * We won't ensure that the slot is persisted after the confirmed_flush
+     * LSN is updated as that could lead to frequent writes.  However, we need
+     * to ensure that we do persist the slots at the time of shutdown whose
+     * confirmed_flush LSN is changed since we last saved the slot to disk.
+     * This will help in avoiding retreat of the confirmed_flush LSN after
+     * restart.  This variable is used to track the last saved confirmed_flush
+     * LSN value.
+     */

This comment makes more sense in SaveSlotToPath() than here. We may decide to
use last_saved_confirmed_flush for something else in future.

I have kept it here because it contains some information that is not
specific to SaveSlotToPath. So, it seems easier to follow the whole
theory if we keep it at the central place in the structure and then
add the reference wherever required but I am fine if you and others
feel strongly about moving this to SaveSlotToPath().

Saving slot to disk happens only in SaveSlotToPath, so except the last
sentence rest of the comment makes sense in SaveSlotToPath().

This function assumes that the subscriber will receive and confirm WAL upto
checkpoint location and publisher's WAL sender will update it in the slot.
Where is the code to ensure that? Does the WAL sender process wait for
checkpoint
LSN to be confirmed when shutting down?

Note, that we need to compare if all the WAL before the
shutdown_checkpoint WAL record is sent. Before the clean shutdown, we
do ensure that all the pending WAL is confirmed back. See the use of
WalSndDone() in WalSndLoop().

Ok. Thanks for pointing that out to me.

I
think we should shut down subscriber, restart publisher and then make this
check based on the contents of the replication slot instead of server log.
Shutting down subscriber will ensure that the subscriber won't send any new
confirmed flush location to the publisher after restart.

But if we shutdown the subscriber before the publisher there is no
guarantee that the publisher has sent all outstanding logs up to the
shutdown checkpoint record (i.e., the latest record). Such a guarantee
can only be there if we do a clean shutdown of the publisher before
the subscriber.

So the sequence is shutdown publisher node, shutdown subscriber node,
start publisher node and carry out the checks.

All the places which call ReplicationSlotSave() mark the slot as dirty. All
the places where SaveSlotToPath() is called, the slot is marked dirty except
when calling from CheckPointReplicationSlots(). So I am wondering whether we
should be marking the slot dirty in CheckPointReplicationSlots() and avoid
passing down is_shutdown flag to SaveSlotToPath().

I feel that will add another spinlock acquire/release pair without
much benefit. Sure, it may not be performance-sensitive but still
adding another pair of lock/release doesn't seem like a better idea.

We call ReplicatioinSlotMarkDirty() followed by ReplicationSlotSave()
at all the places, even those which are more frequent than this. So I
think it's better to stick to that protocol rather than adding a new
flag.

--
Best Wishes,
Ashutosh Bapat

On Thu, Aug 31, 2023 at 12:25 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 12:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

+
+    /*
+     * We won't ensure that the slot is persisted after the confirmed_flush
+     * LSN is updated as that could lead to frequent writes.  However, we need
+     * to ensure that we do persist the slots at the time of shutdown whose
+     * confirmed_flush LSN is changed since we last saved the slot to disk.
+     * This will help in avoiding retreat of the confirmed_flush LSN after
+     * restart.  This variable is used to track the last saved confirmed_flush
+     * LSN value.
+     */

This comment makes more sense in SaveSlotToPath() than here. We may decide to
use last_saved_confirmed_flush for something else in future.

I have kept it here because it contains some information that is not
specific to SaveSlotToPath. So, it seems easier to follow the whole
theory if we keep it at the central place in the structure and then
add the reference wherever required but I am fine if you and others
feel strongly about moving this to SaveSlotToPath().

Saving slot to disk happens only in SaveSlotToPath, so except the last
sentence rest of the comment makes sense in SaveSlotToPath().

This function assumes that the subscriber will receive and confirm WAL upto
checkpoint location and publisher's WAL sender will update it in the slot.
Where is the code to ensure that? Does the WAL sender process wait for
checkpoint
LSN to be confirmed when shutting down?

Note, that we need to compare if all the WAL before the
shutdown_checkpoint WAL record is sent. Before the clean shutdown, we
do ensure that all the pending WAL is confirmed back. See the use of
WalSndDone() in WalSndLoop().

Ok. Thanks for pointing that out to me.

I
think we should shut down subscriber, restart publisher and then make this
check based on the contents of the replication slot instead of server log.
Shutting down subscriber will ensure that the subscriber won't send any new
confirmed flush location to the publisher after restart.

But if we shutdown the subscriber before the publisher there is no
guarantee that the publisher has sent all outstanding logs up to the
shutdown checkpoint record (i.e., the latest record). Such a guarantee
can only be there if we do a clean shutdown of the publisher before
the subscriber.

So the sequence is shutdown publisher node, shutdown subscriber node,
start publisher node and carry out the checks.

This can probably work but I still prefer the current approach as that
will be closer to the ideal values on the disk instead of comparison
with a later in-memory value of confirmed_flush LSN. Ideally, if we
would have a tool like pg_replslotdata which can read the on-disk
state of slots that would be better but missing that, the current one
sounds like the next best possibility. Do you see any problem with the
current approach of test?

BTW, I think we can keep autovacuum = off for this test just to avoid
any extra record generation even though that doesn't matter for the
purpose of test.

All the places which call ReplicationSlotSave() mark the slot as dirty. All
the places where SaveSlotToPath() is called, the slot is marked dirty except
when calling from CheckPointReplicationSlots(). So I am wondering whether we
should be marking the slot dirty in CheckPointReplicationSlots() and avoid
passing down is_shutdown flag to SaveSlotToPath().

I feel that will add another spinlock acquire/release pair without
much benefit. Sure, it may not be performance-sensitive but still
adding another pair of lock/release doesn't seem like a better idea.

We call ReplicatioinSlotMarkDirty() followed by ReplicationSlotSave()
at all the places, even those which are more frequent than this.

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

--
With Regards,
Amit Kapila.

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

I
think we should shut down subscriber, restart publisher and then make this
check based on the contents of the replication slot instead of server log.
Shutting down subscriber will ensure that the subscriber won't send any new
confirmed flush location to the publisher after restart.

But if we shutdown the subscriber before the publisher there is no
guarantee that the publisher has sent all outstanding logs up to the
shutdown checkpoint record (i.e., the latest record). Such a guarantee
can only be there if we do a clean shutdown of the publisher before
the subscriber.

So the sequence is shutdown publisher node, shutdown subscriber node,
start publisher node and carry out the checks.

This can probably work but I still prefer the current approach as that
will be closer to the ideal values on the disk instead of comparison
with a later in-memory value of confirmed_flush LSN. Ideally, if we
would have a tool like pg_replslotdata which can read the on-disk
state of slots that would be better but missing that, the current one
sounds like the next best possibility. Do you see any problem with the
current approach of test?

+ qr/Streaming transactions committing after ([A-F0-9]+\/[A-F0-9]+),
reading WAL from ([A-F0-9]+\/[A-F0-9]+)./

I don't think the LSN reported in this message is guaranteed to be the
confirmed_flush LSN of the slot. It's usually confirmed_flush but not
always. It's the LSN that snapshot builder computes based on factors
including confirmed_flush. There's a chance that this test will fail
sometimes because of this behaviour. Reading directly from
replication slot is better that this. pg_replslotdata might help if we
read replication slot content between shutdown and restart of
publisher.

BTW, I think we can keep autovacuum = off for this test just to avoid
any extra record generation even though that doesn't matter for the
purpose of test.

Autovacuum is one thing, but we can't guarantee the absence of any
concurrent activity forever.

All the places which call ReplicationSlotSave() mark the slot as dirty. All
the places where SaveSlotToPath() is called, the slot is marked dirty except
when calling from CheckPointReplicationSlots(). So I am wondering whether we
should be marking the slot dirty in CheckPointReplicationSlots() and avoid
passing down is_shutdown flag to SaveSlotToPath().

I feel that will add another spinlock acquire/release pair without
much benefit. Sure, it may not be performance-sensitive but still
adding another pair of lock/release doesn't seem like a better idea.

We call ReplicatioinSlotMarkDirty() followed by ReplicationSlotSave()
at all the places, even those which are more frequent than this.

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

yes

I didn't see this entry in commitfest. Since we are discussing it and
the next CF is about to begin, probably it's good to add one there.
--
Best Wishes,
Ashutosh Bapat

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

I
think we should shut down subscriber, restart publisher and then make this
check based on the contents of the replication slot instead of server log.
Shutting down subscriber will ensure that the subscriber won't send any new
confirmed flush location to the publisher after restart.

But if we shutdown the subscriber before the publisher there is no
guarantee that the publisher has sent all outstanding logs up to the
shutdown checkpoint record (i.e., the latest record). Such a guarantee
can only be there if we do a clean shutdown of the publisher before
the subscriber.

So the sequence is shutdown publisher node, shutdown subscriber node,
start publisher node and carry out the checks.

This can probably work but I still prefer the current approach as that
will be closer to the ideal values on the disk instead of comparison
with a later in-memory value of confirmed_flush LSN. Ideally, if we
would have a tool like pg_replslotdata which can read the on-disk
state of slots that would be better but missing that, the current one
sounds like the next best possibility. Do you see any problem with the
current approach of test?

+ qr/Streaming transactions committing after ([A-F0-9]+\/[A-F0-9]+),
reading WAL from ([A-F0-9]+\/[A-F0-9]+)./

I don't think the LSN reported in this message is guaranteed to be the
confirmed_flush LSN of the slot. It's usually confirmed_flush but not
always. It's the LSN that snapshot builder computes based on factors
including confirmed_flush. There's a chance that this test will fail
sometimes because of this behaviour.

I think I am missing something here because as per my understanding,
the LOG referred by the test is generated in CreateDecodingContext()
before which we shouldn't be changing the slot's confirmed_flush LSN.
The LOG [1]errdetail("Streaming transactions committing after %X/%X, reading WAL from %X/%X.", LSN_FORMAT_ARGS(slot->data.confirmed_flush), refers to the slot's persistent value for confirmed_flush,
so how it could be different from what the test is expecting.

[1]: errdetail("Streaming transactions committing after %X/%X, reading WAL from %X/%X.", LSN_FORMAT_ARGS(slot->data.confirmed_flush),
errdetail("Streaming transactions committing after %X/%X, reading WAL
from %X/%X.",
LSN_FORMAT_ARGS(slot->data.confirmed_flush),

--
With Regards,
Amit Kapila.

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

--
With Regards,
Amit Kapila.

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath b) The comments were moved
from ReplicationSlot and moved to CheckPointReplicationSlots c) Tests
will be run in autovacuum = off d) updating last_saved_confirmed_flush
based on cp.slotdata.confirmed_flush rather than
slot->data.confirmed_flush.
I have also added the commitfest entry for this at [1]https://commitfest.postgresql.org/44/4536/.

Thanks to Ashutosh/Amit for the feedback.
Attached v7 version patch has the changes for the same.
[1]: https://commitfest.postgresql.org/44/4536/

Regards,
Vignesh

On Thu, Aug 31, 2023 at 7:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

I
think we should shut down subscriber, restart publisher and then make this
check based on the contents of the replication slot instead of server log.
Shutting down subscriber will ensure that the subscriber won't send any new
confirmed flush location to the publisher after restart.

But if we shutdown the subscriber before the publisher there is no
guarantee that the publisher has sent all outstanding logs up to the
shutdown checkpoint record (i.e., the latest record). Such a guarantee
can only be there if we do a clean shutdown of the publisher before
the subscriber.

So the sequence is shutdown publisher node, shutdown subscriber node,
start publisher node and carry out the checks.

This can probably work but I still prefer the current approach as that
will be closer to the ideal values on the disk instead of comparison
with a later in-memory value of confirmed_flush LSN. Ideally, if we
would have a tool like pg_replslotdata which can read the on-disk
state of slots that would be better but missing that, the current one
sounds like the next best possibility. Do you see any problem with the
current approach of test?

+ qr/Streaming transactions committing after ([A-F0-9]+\/[A-F0-9]+),
reading WAL from ([A-F0-9]+\/[A-F0-9]+)./

I don't think the LSN reported in this message is guaranteed to be the
confirmed_flush LSN of the slot. It's usually confirmed_flush but not
always. It's the LSN that snapshot builder computes based on factors
including confirmed_flush. There's a chance that this test will fail
sometimes because of this behaviour.

I think I am missing something here because as per my understanding,
the LOG referred by the test is generated in CreateDecodingContext()
before which we shouldn't be changing the slot's confirmed_flush LSN.
The LOG [1] refers to the slot's persistent value for confirmed_flush,
so how it could be different from what the test is expecting.

[1]
errdetail("Streaming transactions committing after %X/%X, reading WAL
from %X/%X.",
LSN_FORMAT_ARGS(slot->data.confirmed_flush),

I was afraid that we may move confirmed_flush while creating the
snapshot builder when creating the decoding context. But I don't see
any code doing that. So may be we are safe. But if the log message
changes, this test would fail - depending upon the log message looks a
bit fragile, esp. when we have a way to access the data directly
reliably.

--
Best Wishes,
Ashutosh Bapat

On Fri, Sep 1, 2023 at 1:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 7:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

+ qr/Streaming transactions committing after ([A-F0-9]+\/[A-F0-9]+),
reading WAL from ([A-F0-9]+\/[A-F0-9]+)./

I don't think the LSN reported in this message is guaranteed to be the
confirmed_flush LSN of the slot. It's usually confirmed_flush but not
always. It's the LSN that snapshot builder computes based on factors
including confirmed_flush. There's a chance that this test will fail
sometimes because of this behaviour.

I think I am missing something here because as per my understanding,
the LOG referred by the test is generated in CreateDecodingContext()
before which we shouldn't be changing the slot's confirmed_flush LSN.
The LOG [1] refers to the slot's persistent value for confirmed_flush,
so how it could be different from what the test is expecting.

[1]
errdetail("Streaming transactions committing after %X/%X, reading WAL
from %X/%X.",
LSN_FORMAT_ARGS(slot->data.confirmed_flush),

I was afraid that we may move confirmed_flush while creating the
snapshot builder when creating the decoding context. But I don't see
any code doing that. So may be we are safe.

We are safe in that respect. As far as I understand there is no reason
to be worried.

But if the log message

changes, this test would fail - depending upon the log message looks a
bit fragile, esp. when we have a way to access the data directly
reliably.

This message is there from the very begining (b89e1510) and I can't
forsee a reason to change such a message. But even if we change, we
can always change the test output or test accordingly, if required. I
think it is a matter of preference to which way we can write the test,
so let's not argue too much on this. I find current way slightly more
reliable but we can change it if we see any problem.

--
With Regards,
Amit Kapila.

On Fri, Sep 1, 2023 at 10:50 AM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath

+ if (is_shutdown && SlotIsLogical(s))
+ {
+ SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush)
+ s->dirty = true;

I think it is better to use ReplicationSlotMarkDirty() as that would
be consistent with all other usages.

--
With Regards,
Amit Kapila.

On Mon, 4 Sept 2023 at 15:20, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Sep 1, 2023 at 10:50 AM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath

+ if (is_shutdown && SlotIsLogical(s))
+ {
+ SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush)
+ s->dirty = true;

I think it is better to use ReplicationSlotMarkDirty() as that would
be consistent with all other usages.

ReplicationSlotMarkDirty works only on MyReplicationSlot whereas
CheckpointReplicationSlots loops through all the slots and marks the
appropriate slot as dirty, we might have to change
ReplicationSlotMarkDirty to take the slot as input parameter and all
caller should pass MyReplicationSlot. Another thing is we have already
taken spin lock to access last_confirmed_flush_lsn from
CheckpointReplicationSlots, we could set dirty flag here itself, else
we will have to release the lock and call ReplicationSlotMarkDirty
which will take lock again. Instead shall we set just_dirtied also in
CheckpointReplicationSlots?
Thoughts?

Regards,
Vignesh

On Monday, September 4, 2023 6:15 PM vignesh C <vignesh21@gmail.com> wrote:

On Mon, 4 Sept 2023 at 15:20, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Sep 1, 2023 at 10:50 AM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila

<amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to
indicate that we will actually write/flush the contents at a
later point (except when required for correctness) as even
indicated in the comments atop ReplicatioinSlotMarkDirty().
However, I see your point that we use that protocol at all the current

places including CreateSlotOnDisk().

So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath

+ if (is_shutdown && SlotIsLogical(s)) { SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush) dirty =
+ s->true;

I think it is better to use ReplicationSlotMarkDirty() as that would
be consistent with all other usages.

ReplicationSlotMarkDirty works only on MyReplicationSlot whereas
CheckpointReplicationSlots loops through all the slots and marks the
appropriate slot as dirty, we might have to change ReplicationSlotMarkDirty to
take the slot as input parameter and all caller should pass MyReplicationSlot.

Personally, I feel if we want to centralize the code of marking dirty into a
function, we can introduce a new static function MarkSlotDirty(slot) to mark
passed slot dirty and let ReplicationSlotMarkDirty and
CheckpointReplicationSlots call it. Like:

void
ReplicationSlotMarkDirty(void)
{
MarkSlotDirty(MyReplicationSlot);
}

+static void
+MarkSlotDirty(ReplicationSlot *slot)
+{
+	Assert(slot != NULL);
+
+	SpinLockAcquire(&slot->mutex);
+	slot->just_dirtied = true;
+	slot->dirty = true;
+	SpinLockRelease(&slot->mutex);
+}

This is somewhat similar to the relation between ReplicationSlotSave(serialize
my backend's replications slot) and SaveSlotToPath(save the passed slot).

Another thing is we have already taken spin lock to access
last_confirmed_flush_lsn from CheckpointReplicationSlots, we could set dirty
flag here itself, else we will have to release the lock and call
ReplicationSlotMarkDirty which will take lock again.

Yes, this is unavoidable, but maybe it's not a big problem as
we only do it at shutdown.

Instead shall we set just_dirtied also in CheckpointReplicationSlots?
Thoughts?

I agree we'd better set just_dirtied to true to ensure we will serialize slot
info here, because if some other processes just serialized the slot, the dirty
flag will be reset to false if we don't set just_dirtied to true in
CheckpointReplicationSlots(), this race condition may not exists for now, but
seems better to completely forbid it by setting just_dirtied.

Best Regards,
Hou zj

On Tue, Sep 5, 2023 at 7:54 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Monday, September 4, 2023 6:15 PM vignesh C <vignesh21@gmail.com> wrote:

On Mon, 4 Sept 2023 at 15:20, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Sep 1, 2023 at 10:50 AM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila

<amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to
indicate that we will actually write/flush the contents at a
later point (except when required for correctness) as even
indicated in the comments atop ReplicatioinSlotMarkDirty().
However, I see your point that we use that protocol at all the current

places including CreateSlotOnDisk().

So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath

+ if (is_shutdown && SlotIsLogical(s)) { SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush) dirty =
+ s->true;

I think it is better to use ReplicationSlotMarkDirty() as that would
be consistent with all other usages.

ReplicationSlotMarkDirty works only on MyReplicationSlot whereas
CheckpointReplicationSlots loops through all the slots and marks the
appropriate slot as dirty, we might have to change ReplicationSlotMarkDirty to
take the slot as input parameter and all caller should pass MyReplicationSlot.

Personally, I feel if we want to centralize the code of marking dirty into a
function, we can introduce a new static function MarkSlotDirty(slot) to mark
passed slot dirty and let ReplicationSlotMarkDirty and
CheckpointReplicationSlots call it. Like:

void
ReplicationSlotMarkDirty(void)
{
MarkSlotDirty(MyReplicationSlot);
}

+static void
+MarkSlotDirty(ReplicationSlot *slot)
+{
+       Assert(slot != NULL);
+
+       SpinLockAcquire(&slot->mutex);
+       slot->just_dirtied = true;
+       slot->dirty = true;
+       SpinLockRelease(&slot->mutex);
+}

This is somewhat similar to the relation between ReplicationSlotSave(serialize
my backend's replications slot) and SaveSlotToPath(save the passed slot).

Another thing is we have already taken spin lock to access
last_confirmed_flush_lsn from CheckpointReplicationSlots, we could set dirty
flag here itself, else we will have to release the lock and call
ReplicationSlotMarkDirty which will take lock again.

Yes, this is unavoidable, but maybe it's not a big problem as
we only do it at shutdown.

True but still it doesn't look elegant. I also thought about having a
probably inline function that marks both just_dirty and dirty fields.
However, that requires us to assert that the caller has already
acquired a spinlock. I see a macro SpinLockFree() that might help but
it didn't seem to be used anywhere in the code so not sure if we can
rely on it.

Instead shall we set just_dirtied also in CheckpointReplicationSlots?
Thoughts?

I agree we'd better set just_dirtied to true to ensure we will serialize slot
info here, because if some other processes just serialized the slot, the dirty
flag will be reset to false if we don't set just_dirtied to true in
CheckpointReplicationSlots(), this race condition may not exists for now, but
seems better to completely forbid it by setting just_dirtied.

Agreed, and it is better to close any such possibility because we
can't say with certainty about manual slots. This seems better than
the other ideas we discussed.

--
With Regards,
Amit Kapila.

On Fri, Sep 1, 2023 at 12:16 PM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath b) The comments were moved
from ReplicationSlot and moved to CheckPointReplicationSlots c) Tests
will be run in autovacuum = off d) updating last_saved_confirmed_flush
based on cp.slotdata.confirmed_flush rather than
slot->data.confirmed_flush.
I have also added the commitfest entry for this at [1].

The overall idea looks fine to me

+
+ /*
+ * We won't ensure that the slot is persisted after the
+ * confirmed_flush LSN is updated as that could lead to frequent
+ * writes.  However, we need to ensure that we do persist the slots at
+ * the time of shutdown whose confirmed_flush LSN is changed since we
+ * last saved the slot to disk. This will help in avoiding retreat of
+ * the confirmed_flush LSN after restart.
+ */
+ if (is_shutdown && SlotIsLogical(s))
+ {
+ SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush)
+ s->dirty = true;
+ SpinLockRelease(&s->mutex);
+ }

The comments don't mention anything about why we are just flushing at
the shutdown checkpoint. I mean the checkpoint is not that frequent
and we already perform a lot of I/O during checkpoints so isn't it
wise to flush during every checkpoint. We may argue that there is no
extra advantage of that as we are not optimizing for crash recovery
but OTOH there is no reason for not doing so for other checkpoints or
we are worried about the concurrency with parallel walsender running
during non shutdown checkpoint if so then better we explain that as
well? If it is already discussed in the thread and we have a
conclusion on this then maybe we can mention this in comments?

/*
  * Flush all replication slots to disk.
  *
- * This needn't actually be part of a checkpoint, but it's a convenient
- * location.
+ * is_shutdown is true in case of a shutdown checkpoint.
  */
 void
-CheckPointReplicationSlots(void)
+CheckPointReplicationSlots(bool is_shutdown)

It seems we have removed two lines from the function header comments,
is this intentional or accidental?

Other than this patch LGTM.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Sep 5, 2023 at 9:10 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Fri, Sep 1, 2023 at 12:16 PM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath b) The comments were moved
from ReplicationSlot and moved to CheckPointReplicationSlots c) Tests
will be run in autovacuum = off d) updating last_saved_confirmed_flush
based on cp.slotdata.confirmed_flush rather than
slot->data.confirmed_flush.
I have also added the commitfest entry for this at [1].

The overall idea looks fine to me

+
+ /*
+ * We won't ensure that the slot is persisted after the
+ * confirmed_flush LSN is updated as that could lead to frequent
+ * writes.  However, we need to ensure that we do persist the slots at
+ * the time of shutdown whose confirmed_flush LSN is changed since we
+ * last saved the slot to disk. This will help in avoiding retreat of
+ * the confirmed_flush LSN after restart.
+ */
+ if (is_shutdown && SlotIsLogical(s))
+ {
+ SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush)
+ s->dirty = true;
+ SpinLockRelease(&s->mutex);
+ }

The comments don't mention anything about why we are just flushing at
the shutdown checkpoint. I mean the checkpoint is not that frequent
and we already perform a lot of I/O during checkpoints so isn't it
wise to flush during every checkpoint. We may argue that there is no
extra advantage of that as we are not optimizing for crash recovery
but OTOH there is no reason for not doing so for other checkpoints or
we are worried about the concurrency with parallel walsender running
during non shutdown checkpoint if so then better we explain that as
well? If it is already discussed in the thread and we have a
conclusion on this then maybe we can mention this in comments?

The point is that at the time of non-shutdown checkpoints, it is not
clear that there is an extra advantage but we will definitely add
extra I/O for this. Because at other times, we will already be saving
the slot from time to time as the replication makes progress. And, we
also need to flush such slots during shutdown for correctness for some
use cases like upgrades. We can probably add something like: "At other
times, the walsender keeps saving the slot from time to time as the
replication progresses, so there is no clear advantage of flushing
additional slots at the time of checkpoint". Will that work for you?
Having said that, I am not opposed to doing it for non-shutdown
checkpoints if one makes a separate case for it.

--
With Regards,
Amit Kapila.

On Tue, Sep 5, 2023 at 9:58 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Sep 5, 2023 at 9:10 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

The comments don't mention anything about why we are just flushing at
the shutdown checkpoint. I mean the checkpoint is not that frequent
and we already perform a lot of I/O during checkpoints so isn't it
wise to flush during every checkpoint. We may argue that there is no
extra advantage of that as we are not optimizing for crash recovery
but OTOH there is no reason for not doing so for other checkpoints or
we are worried about the concurrency with parallel walsender running
during non shutdown checkpoint if so then better we explain that as
well? If it is already discussed in the thread and we have a
conclusion on this then maybe we can mention this in comments?

The point is that at the time of non-shutdown checkpoints, it is not
clear that there is an extra advantage but we will definitely add
extra I/O for this. Because at other times, we will already be saving
the slot from time to time as the replication makes progress. And, we
also need to flush such slots during shutdown for correctness for some
use cases like upgrades. We can probably add something like: "At other
times, the walsender keeps saving the slot from time to time as the
replication progresses, so there is no clear advantage of flushing
additional slots at the time of checkpoint". Will that work for you?

Yeah that comments will work out, my only concern was because we added
an explicit condition that it should be synced only during shutdown
checkpoint so better comments also explicitly explains the reason.
Anyway I am fine with either way whether we sync at the shutdown
checkpoint or all the checkpoint. Because I/O for slot sync during
checkpoint time should not be a real worry and with that if we can
avoid additional code with extra conditions then it's better because
such code branches will be frequently hit and I think for testability
pov we prefer to add code in common path unless there is some overhead
or it is specifically meant for that branch only.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, 5 Sept 2023 at 09:10, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Fri, Sep 1, 2023 at 12:16 PM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to indicate that
we will actually write/flush the contents at a later point (except
when required for correctness) as even indicated in the comments atop
ReplicatioinSlotMarkDirty(). However, I see your point that we use
that protocol at all the current places including CreateSlotOnDisk().
So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath b) The comments were moved
from ReplicationSlot and moved to CheckPointReplicationSlots c) Tests
will be run in autovacuum = off d) updating last_saved_confirmed_flush
based on cp.slotdata.confirmed_flush rather than
slot->data.confirmed_flush.
I have also added the commitfest entry for this at [1].

The overall idea looks fine to me

+
+ /*
+ * We won't ensure that the slot is persisted after the
+ * confirmed_flush LSN is updated as that could lead to frequent
+ * writes.  However, we need to ensure that we do persist the slots at
+ * the time of shutdown whose confirmed_flush LSN is changed since we
+ * last saved the slot to disk. This will help in avoiding retreat of
+ * the confirmed_flush LSN after restart.
+ */
+ if (is_shutdown && SlotIsLogical(s))
+ {
+ SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush)
+ s->dirty = true;
+ SpinLockRelease(&s->mutex);
+ }

The comments don't mention anything about why we are just flushing at
the shutdown checkpoint. I mean the checkpoint is not that frequent
and we already perform a lot of I/O during checkpoints so isn't it
wise to flush during every checkpoint. We may argue that there is no
extra advantage of that as we are not optimizing for crash recovery
but OTOH there is no reason for not doing so for other checkpoints or
we are worried about the concurrency with parallel walsender running
during non shutdown checkpoint if so then better we explain that as
well? If it is already discussed in the thread and we have a
conclusion on this then maybe we can mention this in comments?

I felt it is better to do this only during the shutdown checkpoint as
in other cases it is being saved periodically as and when the
replication happens. Added comments for the same.

/*
* Flush all replication slots to disk.
*
- * This needn't actually be part of a checkpoint, but it's a convenient
- * location.
+ * is_shutdown is true in case of a shutdown checkpoint.
*/
void
-CheckPointReplicationSlots(void)
+CheckPointReplicationSlots(bool is_shutdown)

It seems we have removed two lines from the function header comments,
is this intentional or accidental?

Modified.
The updated v8 version patch has the changes for the same.

Regards,
Vignesh

On Tue, Sep 5, 2023 at 10:12 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Sep 5, 2023 at 7:54 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Monday, September 4, 2023 6:15 PM vignesh C <vignesh21@gmail.com> wrote:

On Mon, 4 Sept 2023 at 15:20, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Sep 1, 2023 at 10:50 AM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila

<amit.kapila16@gmail.com> wrote:

All but one. Normally, the idea of marking dirty is to
indicate that we will actually write/flush the contents at a
later point (except when required for correctness) as even
indicated in the comments atop ReplicatioinSlotMarkDirty().
However, I see your point that we use that protocol at all the current

places including CreateSlotOnDisk().

So, we can probably do it here as well.

yes

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
because we don't allow decoding from such slots, so we shouldn't
include those.

Added this check.

Apart from this I have also fixed the following issues that were
agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
instead of setting it in SaveSlotToPath

+ if (is_shutdown && SlotIsLogical(s)) { SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush) dirty =
+ s->true;

I think it is better to use ReplicationSlotMarkDirty() as that would
be consistent with all other usages.

ReplicationSlotMarkDirty works only on MyReplicationSlot whereas
CheckpointReplicationSlots loops through all the slots and marks the
appropriate slot as dirty, we might have to change ReplicationSlotMarkDirty to
take the slot as input parameter and all caller should pass MyReplicationSlot.

Personally, I feel if we want to centralize the code of marking dirty into a
function, we can introduce a new static function MarkSlotDirty(slot) to mark
passed slot dirty and let ReplicationSlotMarkDirty and
CheckpointReplicationSlots call it. Like:

void
ReplicationSlotMarkDirty(void)
{
MarkSlotDirty(MyReplicationSlot);
}

+static void
+MarkSlotDirty(ReplicationSlot *slot)
+{
+       Assert(slot != NULL);
+
+       SpinLockAcquire(&slot->mutex);
+       slot->just_dirtied = true;
+       slot->dirty = true;
+       SpinLockRelease(&slot->mutex);
+}

This is somewhat similar to the relation between ReplicationSlotSave(serialize
my backend's replications slot) and SaveSlotToPath(save the passed slot).

Another thing is we have already taken spin lock to access
last_confirmed_flush_lsn from CheckpointReplicationSlots, we could set dirty
flag here itself, else we will have to release the lock and call
ReplicationSlotMarkDirty which will take lock again.

Yes, this is unavoidable, but maybe it's not a big problem as
we only do it at shutdown.

True but still it doesn't look elegant. I also thought about having a
probably inline function that marks both just_dirty and dirty fields.
However, that requires us to assert that the caller has already
acquired a spinlock. I see a macro SpinLockFree() that might help but
it didn't seem to be used anywhere in the code so not sure if we can
rely on it.

Can't we just have code like this? I mean we will have to make
ReplicationSlotMarkDirty take slot as an argument or have another
version which takes slot as an argument and that would be called by us
as well as by ReplicationSlotMarkDirty(). I mean why do we need these
checks (s-(data.invalidated == RS_INVAL_NONE &&
s->data.confirmed_flush != s->last_saved_confirmed_flush) under the
mutex? Walsender is shutdown so confirmed flush LSN can not move
concurrently and slot can not be invalidated as well because that is
done by checkpointer and we are in checkpointer?

+ if (is_shutdown && SlotIsLogical(s))
+ {
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush)
+ {
+ ReplicationSlotMarkDirty(s);
+ }
+
+ SpinLockRelease(&s->mutex);
+ }

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tuesday, September 5, 2023 4:15 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Hi,

On Tue, Sep 5, 2023 at 10:12 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Tue, Sep 5, 2023 at 7:54 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Monday, September 4, 2023 6:15 PM vignesh C

<vignesh21@gmail.com> wrote:

On Mon, 4 Sept 2023 at 15:20, Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Fri, Sep 1, 2023 at 10:50 AM vignesh C <vignesh21@gmail.com>

wrote:

On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com>

wrote:

I think we should also ensure that slots are not invalidated
(slot.data.invalidated != RS_INVAL_NONE) before marking them
dirty because we don't allow decoding from such slots, so we
shouldn't include those.

Added this check.

Apart from this I have also fixed the following issues that
were agreed on: a) Setting slots to dirty in
CheckPointReplicationSlots instead of setting it in
SaveSlotToPath

+ if (is_shutdown && SlotIsLogical(s)) {
+ SpinLockAcquire(&s->mutex); if (s->data.invalidated ==
+ RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush)
+ s->dirty = true;

I think it is better to use ReplicationSlotMarkDirty() as that
would be consistent with all other usages.

ReplicationSlotMarkDirty works only on MyReplicationSlot whereas
CheckpointReplicationSlots loops through all the slots and marks
the appropriate slot as dirty, we might have to change
ReplicationSlotMarkDirty to take the slot as input parameter and all caller

should pass MyReplicationSlot.

Personally, I feel if we want to centralize the code of marking
dirty into a function, we can introduce a new static function
MarkSlotDirty(slot) to mark passed slot dirty and let
ReplicationSlotMarkDirty and CheckpointReplicationSlots call it. Like:

void
ReplicationSlotMarkDirty(void)
{
MarkSlotDirty(MyReplicationSlot); }

+static void
+MarkSlotDirty(ReplicationSlot *slot) {
+       Assert(slot != NULL);
+
+       SpinLockAcquire(&slot->mutex);
+       slot->just_dirtied = true;
+       slot->dirty = true;
+       SpinLockRelease(&slot->mutex); }

This is somewhat similar to the relation between
ReplicationSlotSave(serialize my backend's replications slot) and

SaveSlotToPath(save the passed slot).

Another thing is we have already taken spin lock to access
last_confirmed_flush_lsn from CheckpointReplicationSlots, we could
set dirty flag here itself, else we will have to release the lock
and call ReplicationSlotMarkDirty which will take lock again.

Yes, this is unavoidable, but maybe it's not a big problem as we
only do it at shutdown.

True but still it doesn't look elegant. I also thought about having a
probably inline function that marks both just_dirty and dirty fields.
However, that requires us to assert that the caller has already
acquired a spinlock. I see a macro SpinLockFree() that might help but
it didn't seem to be used anywhere in the code so not sure if we can
rely on it.

Can't we just have code like this? I mean we will have to make
ReplicationSlotMarkDirty take slot as an argument or have another version
which takes slot as an argument and that would be called by us as well as by
ReplicationSlotMarkDirty(). I mean why do we need these checks
(s-(data.invalidated == RS_INVAL_NONE &&
s->data.confirmed_flush != s->last_saved_confirmed_flush) under the
mutex? Walsender is shutdown so confirmed flush LSN can not move
concurrently and slot can not be invalidated as well because that is done by
checkpointer and we are in checkpointer?

I agree with your analysis that the lock may be unnecessary for now and the
code will work, but I personally feel we'd better take the spinlock.

Firstly, considering our discussion on the potential extension of persisting
the slot for online checkpoints in the future, we anyway need the lock at that
time, so taking the lock here could avoid overlooking the need to update it
later. And the lock also won't cause any performance or concurrency issue.

Additionally, if we don't take the lock, we rely on the assumption that the
walsender will exit before the shutdown checkpoint, currently, that's true for
logical walsender, but physical walsender can exit later than checkpointer. So,
I am slight woirred that if we change the logical walsender's exit timing in
the future, the assumption may not hold.

Besides, for non-built-in logical replication, if someone creates their own
walsender or other processes to send the changes and the process doesn't exit
before the shutdown checkpoint, it may also be a problem. Although I don't have
exsiting examples about these extensions, but I feel taking the lock would make
it more robust.

Best Regards,
Hou zj

On Tue, Sep 5, 2023 at 5:04 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Can't we just have code like this? I mean we will have to make
ReplicationSlotMarkDirty take slot as an argument or have another version
which takes slot as an argument and that would be called by us as well as by
ReplicationSlotMarkDirty(). I mean why do we need these checks
(s-(data.invalidated == RS_INVAL_NONE &&
s->data.confirmed_flush != s->last_saved_confirmed_flush) under the
mutex? Walsender is shutdown so confirmed flush LSN can not move
concurrently and slot can not be invalidated as well because that is done by
checkpointer and we are in checkpointer?

I agree with your analysis that the lock may be unnecessary for now and the
code will work, but I personally feel we'd better take the spinlock.

Firstly, considering our discussion on the potential extension of persisting
the slot for online checkpoints in the future, we anyway need the lock at that
time, so taking the lock here could avoid overlooking the need to update it
later. And the lock also won't cause any performance or concurrency issue.

If we think that we might plan to persist on the online checkpoint as
well then better to do it now, because this is not a extension of the
feature instead we are thinking that it is wise to just persist on the
shutdown checkpoint and I think that's what the conclusion at this
point and if thats the conclusion then no point to right code in
assumption that we will change our conclusion in future.

Additionally, if we don't take the lock, we rely on the assumption that the
walsender will exit before the shutdown checkpoint, currently, that's true for
logical walsender, but physical walsender can exit later than checkpointer. So,
I am slight woirred that if we change the logical walsender's exit timing in
the future, the assumption may not hold.

Besides, for non-built-in logical replication, if someone creates their own
walsender or other processes to send the changes and the process doesn't exit
before the shutdown checkpoint, it may also be a problem. Although I don't have
exsiting examples about these extensions, but I feel taking the lock would make
it more robust.

I think our all logic is based on that the walsender is existed
already. If not then even if you check under the mutex that the
confirmed flush LSN is not changed then it can changed right after you
release the lock and then we will not be flushing the latest update of
the confirmed flush lsn to the disk and our logic of comparing
checkpoint.redo with the confirmed flush lsn might not work?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Sep 5, 2023 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Sep 5, 2023 at 5:04 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Can't we just have code like this? I mean we will have to make
ReplicationSlotMarkDirty take slot as an argument or have another version
which takes slot as an argument and that would be called by us as well as by
ReplicationSlotMarkDirty(). I mean why do we need these checks
(s-(data.invalidated == RS_INVAL_NONE &&
s->data.confirmed_flush != s->last_saved_confirmed_flush) under the
mutex? Walsender is shutdown so confirmed flush LSN can not move
concurrently and slot can not be invalidated as well because that is done by
checkpointer and we are in checkpointer?

...

Additionally, if we don't take the lock, we rely on the assumption that the
walsender will exit before the shutdown checkpoint, currently, that's true for
logical walsender, but physical walsender can exit later than checkpointer. So,
I am slight woirred that if we change the logical walsender's exit timing in
the future, the assumption may not hold.

Besides, for non-built-in logical replication, if someone creates their own
walsender or other processes to send the changes and the process doesn't exit
before the shutdown checkpoint, it may also be a problem. Although I don't have
exsiting examples about these extensions, but I feel taking the lock would make
it more robust.

I think our all logic is based on that the walsender is existed
already. If not then even if you check under the mutex that the
confirmed flush LSN is not changed then it can changed right after you
release the lock and then we will not be flushing the latest update of
the confirmed flush lsn to the disk and our logic of comparing
checkpoint.redo with the confirmed flush lsn might not work?

Right, it can change and in that case, the check related to
confirm_flush LSN will fail during the upgrade. However, the point is
that if we don't take spinlock, we need to properly write comments on
why unlike in other places it is safe here to check these values
without spinlock. We can do that but I feel we have to be careful for
all future usages of these variables, so, having spinlock makes them
follow the normal coding pattern which I feel makes it more robust.
Yes, marking dirty via common function also has merits but personally,
I find it better to follow the normal coding practice of checking the
required fields under spinlock. The other possibility is to first
check if we need to mark the slot dirty under spinlock, then release
the spinlock, and then call the common MarkDirty function, but again
that will be under the assumption that these flags won't change.

--
With Regards,
Amit Kapila.

On Wed, Sep 6, 2023 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Sep 5, 2023 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Right, it can change and in that case, the check related to
confirm_flush LSN will fail during the upgrade. However, the point is
that if we don't take spinlock, we need to properly write comments on
why unlike in other places it is safe here to check these values
without spinlock.

I agree with that, but now also it is not true that we are alway
reading this under the spin lock for example[1]StartLogicalReplication { /* * Report the location after which we'll send out further commits as the * current sentPtr. */ sentPtr = MyReplicationSlot->data.confirmed_flush; }[2]LogicalIncreaseRestartDecodingForSlot { /* candidates are already valid with the current flush position, apply */ if (updated_lsn) LogicalConfirmReceivedLocation(slot->data.confirmed_flush); }, we can see we are
reading this without spin lock.
[1]: StartLogicalReplication { /* * Report the location after which we'll send out further commits as the * current sentPtr. */ sentPtr = MyReplicationSlot->data.confirmed_flush; }
StartLogicalReplication
{
/*
* Report the location after which we'll send out further commits as the
* current sentPtr.
*/
sentPtr = MyReplicationSlot->data.confirmed_flush;
}
[2]: LogicalIncreaseRestartDecodingForSlot { /* candidates are already valid with the current flush position, apply */ if (updated_lsn) LogicalConfirmReceivedLocation(slot->data.confirmed_flush); }
LogicalIncreaseRestartDecodingForSlot
{
/* candidates are already valid with the current flush position, apply */
if (updated_lsn)
LogicalConfirmReceivedLocation(slot->data.confirmed_flush);
}

We can do that but I feel we have to be careful for

all future usages of these variables, so, having spinlock makes them
follow the normal coding pattern which I feel makes it more robust.
Yes, marking dirty via common function also has merits but personally,
I find it better to follow the normal coding practice of checking the
required fields under spinlock. The other possibility is to first
check if we need to mark the slot dirty under spinlock, then release
the spinlock, and then call the common MarkDirty function, but again
that will be under the assumption that these flags won't change.

Thats true, but we are already making the assumption because now also
we are taking the spinlock and taking a decision of marking the slot
dirty. And after that we are releasing the spin lock and if we do not
have guarantee that it can not concurrently change the many things can
go wrong no?

Anyway said that, I do not have any strong objection against what we
are doing now. There were discussion around making the code so that
it can use common function and I was suggesting how it could be
achieved but I am not against the current way either.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 6, 2023 at 9:57 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Wed, Sep 6, 2023 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Sep 5, 2023 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Right, it can change and in that case, the check related to
confirm_flush LSN will fail during the upgrade. However, the point is
that if we don't take spinlock, we need to properly write comments on
why unlike in other places it is safe here to check these values
without spinlock.

I agree with that, but now also it is not true that we are alway
reading this under the spin lock for example[1][2], we can see we are
reading this without spin lock.
[1]
StartLogicalReplication
{
/*
* Report the location after which we'll send out further commits as the
* current sentPtr.
*/
sentPtr = MyReplicationSlot->data.confirmed_flush;
}
[2]
LogicalIncreaseRestartDecodingForSlot
{
/* candidates are already valid with the current flush position, apply */
if (updated_lsn)
LogicalConfirmReceivedLocation(slot->data.confirmed_flush);
}

These are accessed only in walsender and confirmed_flush is always
updated by walsender. So, this is clearly okay.

We can do that but I feel we have to be careful for

all future usages of these variables, so, having spinlock makes them
follow the normal coding pattern which I feel makes it more robust.
Yes, marking dirty via common function also has merits but personally,
I find it better to follow the normal coding practice of checking the
required fields under spinlock. The other possibility is to first
check if we need to mark the slot dirty under spinlock, then release
the spinlock, and then call the common MarkDirty function, but again
that will be under the assumption that these flags won't change.

Thats true, but we are already making the assumption because now also
we are taking the spinlock and taking a decision of marking the slot
dirty. And after that we are releasing the spin lock and if we do not
have guarantee that it can not concurrently change the many things can
go wrong no?

Also, note that invalidated field could be updated by startup process
but that is only possible on standby, so it is safe but again that
would be another assumption.

Anyway said that, I do not have any strong objection against what we
are doing now. There were discussion around making the code so that
it can use common function and I was suggesting how it could be
achieved but I am not against the current way either.

Okay, thanks for looking into it.

--
With Regards,
Amit Kapila.

On Wed, Sep 6, 2023 at 12:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Sep 6, 2023 at 9:57 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Wed, Sep 6, 2023 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Sep 5, 2023 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Right, it can change and in that case, the check related to
confirm_flush LSN will fail during the upgrade. However, the point is
that if we don't take spinlock, we need to properly write comments on
why unlike in other places it is safe here to check these values
without spinlock.

I agree with that, but now also it is not true that we are alway
reading this under the spin lock for example[1][2], we can see we are
reading this without spin lock.
[1]
StartLogicalReplication
{
/*
* Report the location after which we'll send out further commits as the
* current sentPtr.
*/
sentPtr = MyReplicationSlot->data.confirmed_flush;
}
[2]
LogicalIncreaseRestartDecodingForSlot
{
/* candidates are already valid with the current flush position, apply */
if (updated_lsn)
LogicalConfirmReceivedLocation(slot->data.confirmed_flush);
}

These are accessed only in walsender and confirmed_flush is always
updated by walsender. So, this is clearly okay.

Hmm, that's a valid point.

We can do that but I feel we have to be careful for

all future usages of these variables, so, having spinlock makes them
follow the normal coding pattern which I feel makes it more robust.
Yes, marking dirty via common function also has merits but personally,
I find it better to follow the normal coding practice of checking the
required fields under spinlock. The other possibility is to first
check if we need to mark the slot dirty under spinlock, then release
the spinlock, and then call the common MarkDirty function, but again
that will be under the assumption that these flags won't change.

Thats true, but we are already making the assumption because now also
we are taking the spinlock and taking a decision of marking the slot
dirty. And after that we are releasing the spin lock and if we do not
have guarantee that it can not concurrently change the many things can
go wrong no?

Also, note that invalidated field could be updated by startup process
but that is only possible on standby, so it is safe but again that
would be another assumption.

Okay, so I also agree to go with the current patch. Because as you
said above if we access this without a spin lock outside walsender
then we will be making a new exception and I agree with that decision
of not making the new exception.

Other than that the patch LGTM.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, 6 Sept 2023 at 12:09, Dilip Kumar <dilipbalaut@gmail.com> wrote:

Other than that the patch LGTM.

pgindent reported that the new comments added need to be re-adjusted,
here is an updated patch for the same.
I also verified the following: a) patch applies neatly on HEAD b) make
check-world passes c) pgindent looks good d) pgperltiy was fine e)
meson test runs were successful. Also checked that CFBot run was fine
for the last patch.

Regards,
Vignesh

On Thu, Sep 7, 2023 at 10:13 AM vignesh C <vignesh21@gmail.com> wrote:

On Wed, 6 Sept 2023 at 12:09, Dilip Kumar <dilipbalaut@gmail.com> wrote:

Other than that the patch LGTM.

pgindent reported that the new comments added need to be re-adjusted,
here is an updated patch for the same.

Thanks, the patch looks good to me as well.

--
With Regards,
Amit Kapila.

On Thu, Sep 07, 2023 at 11:56:28AM +0530, Amit Kapila wrote:

Thanks, the patch looks good to me as well.

+   /* This is used to track the last saved confirmed_flush LSN value */
+   XLogRecPtr  last_saved_confirmed_flush;

This does not feel sufficient, as the comment explaining what this
variable does uses the same terms as the variable name (aka it is the
last save of the confirmed_lsn). Why it it here and why it is useful?
In which context and/or code paths is it used? Okay, there are some
explanations when saving a slot, restoring a slot or when a checkpoint
processes slots, but it seems important to me to document more things
in ReplicationSlot where this is defined.

(Just passing by, I have not checked the patch logic in details but
that looks under-documented to me.)
--
Michael

On Thu, Sep 7, 2023 at 1:18 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Sep 07, 2023 at 11:56:28AM +0530, Amit Kapila wrote:

Thanks, the patch looks good to me as well.

+   /* This is used to track the last saved confirmed_flush LSN value */
+   XLogRecPtr  last_saved_confirmed_flush;

This does not feel sufficient, as the comment explaining what this
variable does uses the same terms as the variable name (aka it is the
last save of the confirmed_lsn). Why it it here and why it is useful?
In which context and/or code paths is it used? Okay, there are some
explanations when saving a slot, restoring a slot or when a checkpoint
processes slots, but it seems important to me to document more things
in ReplicationSlot where this is defined.

Hmm, this is quite debatable as different people feel differently
about this. The patch author kept it where it is now but in one of my
revisions, I rewrote and added it in the ReplicationSlot. Then
Ashutosh argued that it is better to keep it near where we are saving
the slot (aka where the patch has) [1]/messages/by-id/CAExHW5uXq_CU80XJtmWbPJinRjJx54kbQJ9DT=UFySKXpjVwrw@mail.gmail.com. Anyway, as I also preferred
the core part of the theory about this variable to be in
ReplicationSlot, so I'll move it there before commit unless someone
argues against it or has any other comments.

[1]: /messages/by-id/CAExHW5uXq_CU80XJtmWbPJinRjJx54kbQJ9DT=UFySKXpjVwrw@mail.gmail.com

--
With Regards,
Amit Kapila.

On Thu, Sep 7, 2023 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Sep 7, 2023 at 1:18 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Sep 07, 2023 at 11:56:28AM +0530, Amit Kapila wrote:

Thanks, the patch looks good to me as well.

+   /* This is used to track the last saved confirmed_flush LSN value */
+   XLogRecPtr  last_saved_confirmed_flush;

This does not feel sufficient, as the comment explaining what this
variable does uses the same terms as the variable name (aka it is the
last save of the confirmed_lsn). Why it it here and why it is useful?
In which context and/or code paths is it used? Okay, there are some
explanations when saving a slot, restoring a slot or when a checkpoint
processes slots, but it seems important to me to document more things
in ReplicationSlot where this is defined.

Hmm, this is quite debatable as different people feel differently
about this. The patch author kept it where it is now but in one of my
revisions, I rewrote and added it in the ReplicationSlot. Then
Ashutosh argued that it is better to keep it near where we are saving
the slot (aka where the patch has) [1]. Anyway, as I also preferred
the core part of the theory about this variable to be in
ReplicationSlot, so I'll move it there before commit unless someone
argues against it or has any other comments.

If we want it to be in ReplicationSlot, I suggest we just say, - saves
last confirmed flush LSN to detect any divergence in the in-memory and
on-disk confirmed flush LSN cheaply.

When to detect that divergence and what to do when there is divergence
should be document at relevant places in the code. In future if we
expand the When and How we use this variable, the comment in
ReplicationSlot will be insufficient.

We follow this commenting style at several places e.g.
/* any outstanding modifications? */
bool just_dirtied;
bool dirty;

how and when these variables are used is commented upon in the relevant code.

  * This needn't actually be part of a checkpoint, but it's a convenient
- * location.
+ * location. is_shutdown is true in case of a shutdown checkpoint.

Relying on the first sentence, if we decide to not persist the
replication slot at the time of checkpoint, would that be OK? It
doesn't look like a convenience thing to me any more.

--
Best Wishes,
Ashutosh Bapat

On Thu, Sep 7, 2023 at 3:38 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

* This needn't actually be part of a checkpoint, but it's a convenient
- * location.
+ * location. is_shutdown is true in case of a shutdown checkpoint.

Relying on the first sentence, if we decide to not persist the
replication slot at the time of checkpoint, would that be OK? It
doesn't look like a convenience thing to me any more.

Instead of removing that comment, how about something like this: "This
needn't actually be part of a checkpoint except for shutdown
checkpoint, but it's a convenient location."?

--
With Regards,
Amit Kapila.

On Thu, Sep 7, 2023 at 4:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Sep 7, 2023 at 3:38 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

* This needn't actually be part of a checkpoint, but it's a convenient
- * location.
+ * location. is_shutdown is true in case of a shutdown checkpoint.

Relying on the first sentence, if we decide to not persist the
replication slot at the time of checkpoint, would that be OK? It
doesn't look like a convenience thing to me any more.

Instead of removing that comment, how about something like this: "This
needn't actually be part of a checkpoint except for shutdown
checkpoint, but it's a convenient location."?

I find the wording a bit awkward. My version would be "Checkpoint is a
convenient location to persist all the slots. But in a shutdown
checkpoint, indicated by is_shutdown = true, we also update
confirmed_flush." But please feel free to choose whichever version you
are comfortable with.

--
Best Wishes,
Ashutosh Bapat

On Thu, Sep 7, 2023 at 4:30 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Sep 7, 2023 at 4:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Sep 7, 2023 at 3:38 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

* This needn't actually be part of a checkpoint, but it's a convenient
- * location.
+ * location. is_shutdown is true in case of a shutdown checkpoint.

Relying on the first sentence, if we decide to not persist the
replication slot at the time of checkpoint, would that be OK? It
doesn't look like a convenience thing to me any more.

Instead of removing that comment, how about something like this: "This
needn't actually be part of a checkpoint except for shutdown
checkpoint, but it's a convenient location."?

I find the wording a bit awkward. My version would be "Checkpoint is a
convenient location to persist all the slots. But in a shutdown
checkpoint, indicated by is_shutdown = true, we also update
confirmed_flush." But please feel free to choose whichever version you
are comfortable with.

I think saying we also update confirmed_flush appears unclear to me.
So, I tried another version by changing the entire comment to:
"Normally, we can flush dirty replication slots at regular intervals
by any background process like bgwriter but checkpoint is a convenient
location to persist. Additionally, in case of a shutdown checkpoint,
we also identify the slots for which confirmed_flush has been updated
since the last time it persisted and flush them."

--
With Regards,
Amit Kapila.

On Thu, Sep 7, 2023 at 3:38 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Sep 7, 2023 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Sep 7, 2023 at 1:18 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Sep 07, 2023 at 11:56:28AM +0530, Amit Kapila wrote:

Thanks, the patch looks good to me as well.

+   /* This is used to track the last saved confirmed_flush LSN value */
+   XLogRecPtr  last_saved_confirmed_flush;

This does not feel sufficient, as the comment explaining what this
variable does uses the same terms as the variable name (aka it is the
last save of the confirmed_lsn). Why it it here and why it is useful?
In which context and/or code paths is it used? Okay, there are some
explanations when saving a slot, restoring a slot or when a checkpoint
processes slots, but it seems important to me to document more things
in ReplicationSlot where this is defined.

Hmm, this is quite debatable as different people feel differently
about this. The patch author kept it where it is now but in one of my
revisions, I rewrote and added it in the ReplicationSlot. Then
Ashutosh argued that it is better to keep it near where we are saving
the slot (aka where the patch has) [1]. Anyway, as I also preferred
the core part of the theory about this variable to be in
ReplicationSlot, so I'll move it there before commit unless someone
argues against it or has any other comments.

If we want it to be in ReplicationSlot, I suggest we just say, - saves
last confirmed flush LSN to detect any divergence in the in-memory and
on-disk confirmed flush LSN cheaply.

I have added something on these lines and also changed the other
comment pointed out by you. In the passing, I made minor cosmetic
changes as well.

--
With Regards,
Amit Kapila.

On Fri, Sep 08, 2023 at 09:04:43AM +0530, Amit Kapila wrote:

I have added something on these lines and also changed the other
comment pointed out by you. In the passing, I made minor cosmetic
changes as well.

+ * We can flush dirty replication slots at regular intervals by any
+ * background process like bgwriter but checkpoint is a convenient location.

I don't see a need to refer to the bgwriter here. On the contrary, it
can be confusing as one could think that this flush happens in the
bgwriter, but that's not the case currently as only the checkpointer
does that.

+ * We won't ensure that the slot is persisted after the

s/persisted/flushed/? Or just refer to the "slot's data being
flushed", or refer to "the slot's data is made durable" instead? The
use of "persist" here is confusing, because a slot's persistence
refers to it as being a *candidate* for flush (compared to an
ephemeral slot), and it does not refer to the *fact* of flushing its
data to make sure that it survives a crash. In the context of this
patch, the LSN value tracked in the slot's in-memory data refers to
the last point where the slot's data has been flushed.

+    /*
+     * This is used to track the last persisted confirmed_flush LSN value to
+     * detect any divergence in the in-memory and on-disk values for the same.
+     */

"This value tracks is the last LSN where this slot's data has been
flushed to disk. This is used during a checkpoint shutdown to decide
if a logical slot's data should be forcibly flushed or not."

Hmm. WAL senders are shut down *after* the checkpointer and *after*
the shutdown checkpoint is finished (see PM_SHUTDOWN and
PM_SHUTDOWN_2) because we want the WAL senders to acknowledge the
checkpoint record before shutting down the primary. In order to limit
the number of records to work on after a restart, what this patch is
proposing is an improvement. Perhaps it would be better to document
that we don't care about the potential concurrent activity of logical
WAL senders in this case and that the LSN we are saving at is a best
effort, meaning that last_saved_confirmed_flush is just here to reduce
the damage on a follow-up restart? The comment added in
CheckPointReplicationSlots() goes in this direction, but perhaps this
potential concurrent activity should be mentioned?
--
Michael

On Fri, Sep 8, 2023 at 10:08 AM Michael Paquier <michael@paquier.xyz> wrote:

+    /*
+     * This is used to track the last persisted confirmed_flush LSN value to
+     * detect any divergence in the in-memory and on-disk values for the same.
+     */

"This value tracks is the last LSN where this slot's data has been
flushed to disk.

This makes the comment vague as this sounds like we are saving a slot
corresponding to some LSN which is not the case. If you prefer this
tone then we can instead say: "This value tracks the last
confirmed_flush LSN flushed which is used during a checkpoint shutdown
to decide if a logical slot's data should be forcibly flushed or not."

This is used during a checkpoint shutdown to decide
if a logical slot's data should be forcibly flushed or not."

Hmm. WAL senders are shut down *after* the checkpointer and *after*
the shutdown checkpoint is finished (see PM_SHUTDOWN and
PM_SHUTDOWN_2) because we want the WAL senders to acknowledge the
checkpoint record before shutting down the primary.

As per my understanding, this is not true for logical walsenders. As
per code, while HandleCheckpointerInterrupts(), we call ShutdownXLOG()
which sends a signal to walsender to stop and waits for it to stop.
And only after that, did it write a shutdown checkpoint WAL record.
After getting the InitStopping signal, walsender sets got_STOPPING
flag. Then *logical* walsender ensures that it sends all the pending
WAL and exits. What you have quoted is probably true for physical
walsenders.

In order to limit
the number of records to work on after a restart, what this patch is
proposing is an improvement. Perhaps it would be better to document
that we don't care about the potential concurrent activity of logical
WAL senders in this case and that the LSN we are saving at is a best
effort, meaning that last_saved_confirmed_flush is just here to reduce
the damage on a follow-up restart?

Unless I am wrong, there shouldn't be any concurrent activity for
logical walsenders. IIRC, it is a mandatory requirement for logical
walsenders to stop before shutdown checkpointer to avoid panic error.
We do handle logical walsnders differently because they can generate
WAL during decoding.

The comment added in
CheckPointReplicationSlots() goes in this direction, but perhaps this
potential concurrent activity should be mentioned?

Sure, we can change it if required.

--
With Regards,
Amit Kapila.

On Fri, Sep 08, 2023 at 11:50:37AM +0530, Amit Kapila wrote:

On Fri, Sep 8, 2023 at 10:08 AM Michael Paquier <michael@paquier.xyz> wrote:

+    /*
+     * This is used to track the last persisted confirmed_flush LSN value to
+     * detect any divergence in the in-memory and on-disk values for the same.
+     */

"This value tracks is the last LSN where this slot's data has been
flushed to disk.

This makes the comment vague as this sounds like we are saving a slot
corresponding to some LSN which is not the case. If you prefer this
tone then we can instead say: "This value tracks the last
confirmed_flush LSN flushed which is used during a checkpoint shutdown
to decide if a logical slot's data should be forcibly flushed or not."

Okay, that looks like an improvement over the term "persisted".

This is used during a checkpoint shutdown to decide
if a logical slot's data should be forcibly flushed or not."

Hmm. WAL senders are shut down *after* the checkpointer and *after*
the shutdown checkpoint is finished (see PM_SHUTDOWN and
PM_SHUTDOWN_2) because we want the WAL senders to acknowledge the
checkpoint record before shutting down the primary.

As per my understanding, this is not true for logical walsenders. As
per code, while HandleCheckpointerInterrupts(), we call ShutdownXLOG()
which sends a signal to walsender to stop and waits for it to stop.
And only after that, did it write a shutdown checkpoint WAL record.
After getting the InitStopping signal, walsender sets got_STOPPING
flag. Then *logical* walsender ensures that it sends all the pending
WAL and exits. What you have quoted is probably true for physical
walsenders.

Hm, reminding me about this area.. This roots down to the handling of
WalSndCaughtUp in the send_data callback for logical or physical.
This is switched to true for logical WAL senders much earlier than
physical WAL senders, aka before the shutdown checkpoint begins in the
latter. What was itching me a bit is that the postmaster logic could
be made more solid. Logical and physical WAL senders are both marked
as BACKEND_TYPE_WALSND, but we don't actually check that the WAL
senders remaining at the end of PM_SHUTDOWN_2 are *not* connected to a
database. This would require a new BACKEND_TYPE_* perhaps, or perhaps
we're fine with the current state because we'll catch up problems in
the checkpointer if any WAL is generated while the shutdown checkpoint
is running anyway. Just something I got in mind, unrelated to this
patch.

In order to limit
the number of records to work on after a restart, what this patch is
proposing is an improvement. Perhaps it would be better to document
that we don't care about the potential concurrent activity of logical
WAL senders in this case and that the LSN we are saving at is a best
effort, meaning that last_saved_confirmed_flush is just here to reduce
the damage on a follow-up restart?

Unless I am wrong, there shouldn't be any concurrent activity for
logical walsenders. IIRC, it is a mandatory requirement for logical
walsenders to stop before shutdown checkpointer to avoid panic error.
We do handle logical walsnders differently because they can generate
WAL during decoding.

Yeah. See above.

The comment added in
CheckPointReplicationSlots() goes in this direction, but perhaps this
potential concurrent activity should be mentioned?

Sure, we can change it if required.

+ * We can flush dirty replication slots at regular intervals by any
+ * background process like bgwriter but checkpoint is a convenient location.

I still don't quite see a need to mention the bgwriter at all here..
That's just unrelated.

The comment block in CheckPointReplicationSlots() from v10 uses
"persist", but you mean "flush", I guess..
--
Michael

On Mon, Sep 11, 2023 at 12:08 PM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Sep 08, 2023 at 11:50:37AM +0530, Amit Kapila wrote:

On Fri, Sep 8, 2023 at 10:08 AM Michael Paquier <michael@paquier.xyz> wrote:

+    /*
+     * This is used to track the last persisted confirmed_flush LSN value to
+     * detect any divergence in the in-memory and on-disk values for the same.
+     */

"This value tracks is the last LSN where this slot's data has been
flushed to disk.

This makes the comment vague as this sounds like we are saving a slot
corresponding to some LSN which is not the case. If you prefer this
tone then we can instead say: "This value tracks the last
confirmed_flush LSN flushed which is used during a checkpoint shutdown
to decide if a logical slot's data should be forcibly flushed or not."

Okay, that looks like an improvement over the term "persisted".

Changed accordingly.

This is used during a checkpoint shutdown to decide
if a logical slot's data should be forcibly flushed or not."

Hmm. WAL senders are shut down *after* the checkpointer and *after*
the shutdown checkpoint is finished (see PM_SHUTDOWN and
PM_SHUTDOWN_2) because we want the WAL senders to acknowledge the
checkpoint record before shutting down the primary.

As per my understanding, this is not true for logical walsenders. As
per code, while HandleCheckpointerInterrupts(), we call ShutdownXLOG()
which sends a signal to walsender to stop and waits for it to stop.
And only after that, did it write a shutdown checkpoint WAL record.
After getting the InitStopping signal, walsender sets got_STOPPING
flag. Then *logical* walsender ensures that it sends all the pending
WAL and exits. What you have quoted is probably true for physical
walsenders.

Hm, reminding me about this area.. This roots down to the handling of
WalSndCaughtUp in the send_data callback for logical or physical.
This is switched to true for logical WAL senders much earlier than
physical WAL senders, aka before the shutdown checkpoint begins in the
latter. What was itching me a bit is that the postmaster logic could
be made more solid. Logical and physical WAL senders are both marked
as BACKEND_TYPE_WALSND, but we don't actually check that the WAL
senders remaining at the end of PM_SHUTDOWN_2 are *not* connected to a
database. This would require a new BACKEND_TYPE_* perhaps, or perhaps
we're fine with the current state because we'll catch up problems in
the checkpointer if any WAL is generated while the shutdown checkpoint
is running anyway. Just something I got in mind, unrelated to this
patch.

I don't know if the difference is worth inventing a new BACKEND_TYPE_*
but if you think so then we can probably discuss this in a new thread.
I think we may want to improve some comments as a separate patch to
make this evident.

+ * We can flush dirty replication slots at regular intervals by any
+ * background process like bgwriter but checkpoint is a convenient location.

I still don't quite see a need to mention the bgwriter at all here..
That's just unrelated.

I don't disagree with it, so changed it in the attached patch.

The comment block in CheckPointReplicationSlots() from v10 uses
"persist", but you mean "flush", I guess..

This point is not very clear to me. Can you please quote the exact
comment if you think something needs to be changed?

--
With Regards,
Amit Kapila.

On Mon, Sep 11, 2023 at 02:49:49PM +0530, Amit Kapila wrote:

I don't know if the difference is worth inventing a new BACKEND_TYPE_*
but if you think so then we can probably discuss this in a new thread.
I think we may want to improve some comments as a separate patch to
make this evident.

The comments in postmaster.c could be improved, at least. There is no
need to discuss that here.

This point is not very clear to me. Can you please quote the exact
comment if you think something needs to be changed?

Hmm. Don't think that's it yet..

Please see the v11 attached, that rewords all the places of the patch
that need clarifications IMO. I've found that the comment additions
in CheckPointReplicationSlots() to be overcomplicated:
- The key point to force a flush of a slot if its confirmed_lsn has
moved ahead of the last LSN where it was saved is to make the follow
up restart more responsive.
- Not sure that there is any point to mention the other code paths in
the tree where ReplicationSlotSave() can be called, and a slot can be
saved in other processes than just WAL senders (like slot
manipulations in normal backends, for one). This was the last
sentence in v10.
- Persist is incorrect in this context in the tests, slot.c and
slot.h, as it should refer to the slot's data being flushed, saved or
just "made durable" because this is what the new last saved LSN is
here for. Persistence is a slot property, and does not refer to the
fact of flushing the data IMO.

+           if (s->data.invalidated == RS_INVAL_NONE &&
+               s->data.confirmed_flush != s->last_saved_confirmed_flush)

Actually this is incorrect, no? Shouldn't we make sure that the
confirmed_flush is strictly higher than the last saved LSN?
--
Michael

On Tue, Sep 12, 2023 at 10:55 AM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Sep 11, 2023 at 02:49:49PM +0530, Amit Kapila wrote:

Please see the v11 attached, that rewords all the places of the patch
that need clarifications IMO. I've found that the comment additions
in CheckPointReplicationSlots() to be overcomplicated:
- The key point to force a flush of a slot if its confirmed_lsn has
moved ahead of the last LSN where it was saved is to make the follow
up restart more responsive.

I don't think it will become more responsive in any way, not sure what
made you think like that. The key idea is that after restart we want
to ensure that all the WAL data up to the shutdown checkpoint record
is sent to downstream. As mentioned in the commit message, this will
help in ensuring that upgrades don't miss any data and then there is
another small advantage as mentioned in the commit message.

- Not sure that there is any point to mention the other code paths in
the tree where ReplicationSlotSave() can be called, and a slot can be
saved in other processes than just WAL senders (like slot
manipulations in normal backends, for one). This was the last
sentence in v10.

The point was the earlier sentence is no longer true and keeping it as
it is could be wrong or at least misleading. For example, earlier it
is okay to say, "This needn't actually be part of a checkpoint, ..."
but now that is no longer true as we want to invoke this at the time
of shutdown checkpoint for correctness. If we want to be precise, we
can say, "It is convenient to flush dirty replication slots at the
time of checkpoint. Additionally, .."

+           if (s->data.invalidated == RS_INVAL_NONE &&
+               s->data.confirmed_flush != s->last_saved_confirmed_flush)

Actually this is incorrect, no? Shouldn't we make sure that the
confirmed_flush is strictly higher than the last saved LSN?

I can't see why it is incorrect. Do you see how (in what scenario) it
could go wrong? As per my understanding, confirmed_flush LSN will
always be greater than equal to last_saved_confirmed_flush but we
don't want to ensure that point here because we just want if the
latest value is not the same then we should mark the slot dirty and
flush it as that will be location we have ensured to update before
walsender shutdown. I think it is better to add an assert if you are
worried about any such case and we had thought of adding it as well
but then didn't do it because we don't have matching asserts to ensure
that we never assign prior LSN value to consfirmed_flush LSN.

+ /*
+ * LSN used to track the last confirmed_flush LSN where the slot's data
+ * has been flushed to disk.
+ */
+ XLogRecPtr last_saved_confirmed_flush;

I don't want to argue on such a point because it is a little bit of a
matter of personal choice but I find this comment unclear. It seems to
read that confirmed_flush LSN is some LSN position which is where we
flushed the slot's data and that is not true. I found the last comment
in the patch sent by me: "This value tracks the last confirmed_flush
LSN flushed which is used during a shutdown checkpoint to decide if
logical's slot data should be forcibly flushed or not." which I feel
we agreed upon is better.

--
With Regards,
Amit Kapila.

On Tue, Sep 12, 2023 at 03:15:44PM +0530, Amit Kapila wrote:

I don't think it will become more responsive in any way, not sure what
made you think like that. The key idea is that after restart we want
to ensure that all the WAL data up to the shutdown checkpoint record
is sent to downstream. As mentioned in the commit message, this will
help in ensuring that upgrades don't miss any data and then there is
another small advantage as mentioned in the commit message.

Good thing I did not use the term "responsive" in the previous patch I
posted. My apologies if you found that confusing. Let's say, "to
prevent unnecessary retreat", then ;)

- Not sure that there is any point to mention the other code paths in
the tree where ReplicationSlotSave() can be called, and a slot can be
saved in other processes than just WAL senders (like slot
manipulations in normal backends, for one). This was the last
sentence in v10.

The point was the earlier sentence is no longer true and keeping it as
it is could be wrong or at least misleading. For example, earlier it
is okay to say, "This needn't actually be part of a checkpoint, ..."
but now that is no longer true as we want to invoke this at the time
of shutdown checkpoint for correctness. If we want to be precise, we

How so? This is just a reference about the fact that using a
checkpoint path for this stuff is useful. A shutdown checkpoint is
still a checkpoint, done by the checkpointer. The background writer
is not concerned by that.

can say, "It is convenient to flush dirty replication slots at the
time of checkpoint. Additionally, .."

Okay by mr to reword the top comment of CheckPointReplicationSlots()
to use these terms, if you feel strongly about it.

+           if (s->data.invalidated == RS_INVAL_NONE &&
+               s->data.confirmed_flush != s->last_saved_confirmed_flush)

Actually this is incorrect, no? Shouldn't we make sure that the
confirmed_flush is strictly higher than the last saved LSN?

I can't see why it is incorrect. Do you see how (in what scenario) it
could go wrong? As per my understanding, confirmed_flush LSN will
always be greater than equal to last_saved_confirmed_flush but we
don't want to ensure that point here because we just want if the
latest value is not the same then we should mark the slot dirty and
flush it as that will be location we have ensured to update before
walsender shutdown. I think it is better to add an assert if you are
worried about any such case and we had thought of adding it as well
but then didn't do it because we don't have matching asserts to ensure
that we never assign prior LSN value to consfirmed_flush LSN.

Because that's just safer in the long run, and I don't see why we
cannot just do that? Imagine, for instance, that a bug doing an
incorrect manipulation of a logical slot's data does an incorrect
computation of this field, and that we finish with in-memory data
that's older than what was previously saved. The code may cause a
flush at an incorrect, past, position. That's just an assumption from
my side, of course.

I don't want to argue on such a point because it is a little bit of a
matter of personal choice but I find this comment unclear. It seems to
read that confirmed_flush LSN is some LSN position which is where we
flushed the slot's data and that is not true. I found the last comment
in the patch sent by me: "This value tracks the last confirmed_flush
LSN flushed which is used during a shutdown checkpoint to decide if
logical's slot data should be forcibly flushed or not." which I feel
we agreed upon is better.

Okay, fine by me here.
--
Michael

On Wed, Sep 13, 2023 at 10:57 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Sep 12, 2023 at 03:15:44PM +0530, Amit Kapila wrote:

- Not sure that there is any point to mention the other code paths in
the tree where ReplicationSlotSave() can be called, and a slot can be
saved in other processes than just WAL senders (like slot
manipulations in normal backends, for one). This was the last
sentence in v10.

The point was the earlier sentence is no longer true and keeping it as
it is could be wrong or at least misleading. For example, earlier it
is okay to say, "This needn't actually be part of a checkpoint, ..."
but now that is no longer true as we want to invoke this at the time
of shutdown checkpoint for correctness. If we want to be precise, we

How so?

Consider if we move this call to bgwriter (aka flushing slots is no
longer part of a checkpoint), Would that be okay? Previously, I think
it was okay but not now. I see an argument to keep that as it is as
well because we have already mentioned the special shutdown checkpoint
case. By the way, I have changed this because Ashutosh felt it is no
longer correct to keep the first sentence as it is. See his email[1]
(Relying on the first sentence, ...). It happened previously as well
that different reviewers working in this area have different views on
this sort of thing. I am trying my best to address the review
comments, especially from experienced hackers but personally, I feel
this is a minor nitpick and isn't worth too much argument, either way,
should be okay.

+           if (s->data.invalidated == RS_INVAL_NONE &&
+               s->data.confirmed_flush != s->last_saved_confirmed_flush)

Actually this is incorrect, no? Shouldn't we make sure that the
confirmed_flush is strictly higher than the last saved LSN?

I can't see why it is incorrect. Do you see how (in what scenario) it
could go wrong? As per my understanding, confirmed_flush LSN will
always be greater than equal to last_saved_confirmed_flush but we
don't want to ensure that point here because we just want if the
latest value is not the same then we should mark the slot dirty and
flush it as that will be location we have ensured to update before
walsender shutdown. I think it is better to add an assert if you are
worried about any such case and we had thought of adding it as well
but then didn't do it because we don't have matching asserts to ensure
that we never assign prior LSN value to consfirmed_flush LSN.

Because that's just safer in the long run, and I don't see why we
cannot just do that? Imagine, for instance, that a bug doing an
incorrect manipulation of a logical slot's data does an incorrect
computation of this field, and that we finish with in-memory data
that's older than what was previously saved. The code may cause a
flush at an incorrect, past, position. That's just an assumption from
my side, of course.

If you are worried about such bugs, it would be better to have an
Assert as suggested previously rather than greater than check because
we will at least catch such bugs otherwise it can go unnoticed or in
the worst case will lead to unknown consequences. I am saying this
because if there are such bugs (or got introduced later) then the slot
can be flushed with a prior confirmed_flush location even from other
code paths. Just for reference, we don't have any check ensuring that
confirmed_flush LSN can move backward in function
LogicalConfirmReceivedLocation(), see and also another place where we
update it:
else
{
SpinLockAcquire(&MyReplicationSlot->mutex);
MyReplicationSlot->data.confirmed_flush = lsn;
SpinLockRelease(&MyReplicationSlot->mutex);
}

As other places don't have an assert, I didn't add one here but we can
add one here.

I don't want to argue on such a point because it is a little bit of a
matter of personal choice but I find this comment unclear. It seems to
read that confirmed_flush LSN is some LSN position which is where we
flushed the slot's data and that is not true. I found the last comment
in the patch sent by me: "This value tracks the last confirmed_flush
LSN flushed which is used during a shutdown checkpoint to decide if
logical's slot data should be forcibly flushed or not." which I feel
we agreed upon is better.

Okay, fine by me here.

Thanks, will change once we agree on the remaining points.

--
With Regards,
Amit Kapila.

On Wed, Sep 13, 2023 at 12:07:12PM +0530, Amit Kapila wrote:

Consider if we move this call to bgwriter (aka flushing slots is no
longer part of a checkpoint), Would that be okay? Previously, I think
it was okay but not now. I see an argument to keep that as it is as
well because we have already mentioned the special shutdown checkpoint
case. By the way, I have changed this because Ashutosh felt it is no
longer correct to keep the first sentence as it is. See his email[1]
(Relying on the first sentence, ...).

Hmmm.. Okay..

As other places don't have an assert, I didn't add one here but we can
add one here.

I'd be OK with an assertion here at the end, though I'd still choose a
stricter run-time check if I were to apply that myself.
--
Michael

On Wed, Sep 13, 2023 at 12:45 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Sep 13, 2023 at 12:07:12PM +0530, Amit Kapila wrote:

Consider if we move this call to bgwriter (aka flushing slots is no
longer part of a checkpoint), Would that be okay? Previously, I think
it was okay but not now. I see an argument to keep that as it is as
well because we have already mentioned the special shutdown checkpoint
case. By the way, I have changed this because Ashutosh felt it is no
longer correct to keep the first sentence as it is. See his email[1]
(Relying on the first sentence, ...).

Hmmm.. Okay..

The patch is updated as per recent discussion.

--
With Regards,
Amit Kapila.

On Wed, Sep 13, 2023 at 04:20:37PM +0530, Amit Kapila wrote:

The patch is updated as per recent discussion.

WFM. Thanks for the updated version.
--
Michael

On Thu, Sep 14, 2023 at 7:20 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Sep 13, 2023 at 04:20:37PM +0530, Amit Kapila wrote:

The patch is updated as per recent discussion.

WFM. Thanks for the updated version.

Pushed.

--
With Regards,
Amit Kapila.

On Thu, Sep 14, 2023 at 2:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Sep 14, 2023 at 7:20 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Sep 13, 2023 at 04:20:37PM +0530, Amit Kapila wrote:

The patch is updated as per recent discussion.

WFM. Thanks for the updated version.

Pushed.

Commitfest entry "https://commitfest.postgresql.org/44/4536/ is in
"Ready for committer" state. Is there something remaining here? We
should probably set it as "committed".

--
Best Wishes,
Ashutosh Bapat

On Wed, Sep 20, 2023 at 04:48:00PM +0530, Ashutosh Bapat wrote:

Commitfest entry "https://commitfest.postgresql.org/44/4536/ is in
"Ready for committer" state. Is there something remaining here? We
should probably set it as "committed".

Thanks, I've switched that to "Committed".
--
Michael