On Fri, Jan 3, 2025 at 8:06 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Hi Shubham,
>
> Here are some review comments for patch v5-0001.
>
> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 1.
> + <para>
> + The source server must have <xref linkend="guc-max-slot-wal-keep-size"/>
> to
> + be set to <literal>-1</literal> to prevent the automatic removal of WAL
> + replication slots. Setting this parameter to files needed by a
> specific size
> + may lead to replication failures if required WAL files are
> + prematurely deleted.
> + </para>
>
> IIUC, this problem is all about the removal of WAL *files*, not "WAL
> replication slots".
>
> Also, the "Setting this parameter to files needed by a specific size"
> part sounds strange.
>
> I think this can be simplified anyhow like below.
>
> SUGGESTION:
> Replication failures can occur if required WAL files are prematurely
> deleted. To prevent this, the source server must set <xref
> linkend="guc-max-slot-wal-keep-size"/> to <literal>-1</literal>,
> ensuring WAL files are not automatically removed.
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> check_publisher:
>
> 2.
> + if (dry_run && max_slot_wal_keep_size != -1)
> + {
> + pg_log_warning("publisher requires WAL size must not be restricted");
> + pg_log_warning_detail("The max_slot_wal_keep_size parameter is
> currently set to a non-default value, which may lead to replication
> failures. "
> + "This parameter must be set to -1 to ensure that required WAL
> files are not prematurely removed.");
> + }
>
> 2a.
> Whenever you mention a GUC name in a PostgreSQL message then (by
> convention) it must be double-quoted.
>
> ~
>
> 2b.
> The detailed message seems verbose and repetitive to me.
>
> ~
>
> 2c.
> The other nearby messages use the terminology "configuration
> parameter", so this should be the same.
>
> ~
>
> 2d.
> The other nearby messages use \"%s\" substitution for the GUC name, so
> this should be the same.
>
> ~
>
> 2e.
> IMO advising the user to change a configuration parameter should be
> done using the pg_log_warning_hint function (e.g. _hint, not
> _details).
>
> ~~
>
> Result:
>
> CURRENT (pg_log_warning_details)
> The max_slot_wal_keep_size parameter is currently set to a non-default
> value, which may lead to replication failures. This parameter must be
> set to -1 to ensure that required WAL files are not prematurely
> removed.
>
> SUGGESTION (pg_log_warning_hint)
> Set the configuration parameter \"%s\" to -1 to ensure that required
> WAL files are not prematurely removed.
>I have fixed the given comments using your suggestions. Tha attached patch contains the suggested changes. Thanks and Regards, Shubham Khanna.
v6-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
