On Wed, Oct 27, 2021 at 9:37 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.
Thanks for the updated version. The patch is much better than before
except needing minor changes to the test description that testing
recovery_end_command_file before promotion, I did the same in the
attached version.
Regards,
Amul
