Re: TAP test for recovery_end_command

On Mon, Oct 25, 2021 at 02:42:28PM +0530, Amul Sul wrote:
> Understood, moved tests to 002_archiving.pl in the attached version.

Thanks for the new patch.  I have reviewed its contents, and there
were a couple of things that caught my attention while putting my
hands on it.

+$node_standby->append_conf('postgresql.conf',
+    "archive_cleanup_command = 'echo archive_cleanuped > $archive_cleanup_command_file'");
+$node_standby->append_conf('postgresql.conf',
+    "recovery_end_command = 'echo recovery_ended > $recovery_end_command_file'");
This can be formatted with a single append_conf() call and qq() to
have the correct set of quotes.

+$node_primary->safe_psql('postgres', "CHECKPOINT");
 my $current_lsn =
   $node_primary->safe_psql('postgres', "SELECT pg_current_wal_lsn();");
This had better say that the checkpoint is necessary because we need
one before switching to a new segment on the primary, as much as the
checkpoint on the first standby is needed to trigger the command whose
execution is checked.

+$node_standby2->append_conf('postgresql.conf',
+    "archive_cleanup_command = 'echo xyz > $data_dir/unexisting_dir/xyz.file'");
+$node_standby2->append_conf('postgresql.conf',
+    "recovery_end_command = 'echo xyz > $data_dir/unexisting_dir/xyz.file'");
[...]
+# Failing to execute archive_cleanup_command and/or recovery_end_command does
+# not affect promotion.
+is($node_standby2->safe_psql( 'postgres', q{SELECT pg_is_in_recovery()}), 'f',
+    "standby promoted successfully despite incorrect archive_cleanup_command and recovery_end_command");

This SQL test is mostly useless IMO, as the promote() call done above
ensures that this state is reached properly, and the same thing could
be with the removals of RECOVERYHISTORY and RECOVERYXLOG.  I think
that it would be better to check directly if the commands are run or
not.  This is simple to test: look at the logs from a position just
before the promotion, slurp the log file of $standby2 from this
position, and finally compare its contents with a regex of your
choice.  I have chosen a simple "qr/WARNING:.*recovery_end_command/s"
for the purpose of this test.  Having a test for
archive_cleanup_command here would be nice, but that would be much
more costly than the end-of-recovery command, so I have left that
out.  Perhaps we could just append it in the conf as a dummy, as you
did, though, but its execution is not deterministic in this test so we
are better without for now IMO.

perltidy was also complaining a bit, this is fixed as of the attached.
I have checked things on my own Windows dev box, while on it.
--
Michael