On Tue, Dec 31, 2024 at 4:29 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Hi Shubham. > > Here are some review comments for the patch v3-0001. > > ====== > Commit message. > > 1. > I can't pinpoint anything specifically wrong about this commit > message, but it seems to be repeating the same information over and > over. >
Updated the commit message by omitting the last line from the commit message.
> ======
> Missing pg_createsubscriber documentation?
>
> 2.
> I thought any prerequisite for 'max_slot_wal_keep_size' should be
> documented here [1] along with the other setting requirements.
>
Updated the 'pg_createsubscriber' documentation and added the
Prerequisite for 'max_slot_wal_keep_size'.
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 3.
> - " pg_catalog.current_setting('max_prepared_transactions')");
> + " pg_catalog.current_setting('max_prepared_transactions'),"
> + " pg_catalog.current_setting('max_slot_wal_keep_size')");
>
> The code comment above this SQL looks like it should also mention the
> 'max_slot_wal_keep_size' value requirement.
>
Added the 'max_slot_wal_keep_size' value requirement in the comments.
>
> 4.
> +/*
> + * Validate max_slot_wal_keep_size
> + * Logical replication requires max_slot_wal_keep_size to be set to -1 on the
> + * publisher to prevent the deletion of WAL files that are still needed by
> + * replication slots. If this parameter is set to a non-default value, it may
> + * cause replication failures due to required WAL files being
> + * prematurely removed.
> + */
> + if (dry_run && max_slot_wal_keep_size != -1)
> + {
> + pg_log_warning("publisher requires max_slot_wal_keep_size to be -1,
> but is set to %d",
> + max_slot_wal_keep_size);
> + pg_log_warning_detail("Change the configuration parameter \"%s\" on
> the publisher to %d.",
> + "max_slot_wal_keep_size", -1);
> + }
> +
>
> 4a.
> The code comment is not indented correctly.
Fixed.
> ~
>
> 4b.
> It seems strange that the 'max_slot_wal_keep_size' GUC name is not
> substituted in the pg_log_warning, but it is in the
> pg_log_warning_detail.
>
> Furthermore, GUC names appearing in messages should always be quoted.
>
Fixed.
>
> 4c.
> Was there some reason to make this only a warning, instead of an error?
>
> The risk "may cause replication failures" seems quite serious, so in
> that case it might be a poor user experience if we are effectively
> saying: "Too bad, it's all broken now; we warned you (only if you
> tried a dry run), but you did not listen".
>
> In other words, why not make this an error condition up-front to
> entirely remove any chance of this failure?
>
Changed the warning condition to the error condition and also updated
the test case accordingly.
> ======
The attached Patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v4-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
