Re: Log a warning in pg_createsubscriber for max_slot_wal_keep_size

On Tue, Dec 31, 2024 at 4:29 AM Peter Smith <smithpb2250(at)gmail(dot)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.