On Wed, 23 Aug 2023 at 14:21, Hayato Kuroda (Fujitsu)
<[email protected]> 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
