Log a warning in pg_createsubscriber for max_slot_wal_keep_size

Hi,

Currently, there is a risk that pg_createsubscriber may fail to
complete successfully when the max_slot_wal_keep_size value is set too
low. This can occur if the WAL is removed before the standby using the
replication slot is able to complete replication, as the required WAL
files are no longer available.

I was able to reproduce this issue using the following steps:
Set up a streaming replication environment.
Run pg_createsubscriber in a debugger.
Pause pg_createsubscriber at the setup_recovery stage.
Perform several operations on the primary node to generate a large
volume of WAL, causing older WAL segments to be removed due to the low
max_slot_wal_keep_size setting.
Once the necessary WAL segments are deleted, continue the execution of
pg_createsubscriber.
At this point, pg_createsubscriber fails with the following error:
2024-12-29 01:21:37.590 IST [427353] FATAL: could not receive data
from WAL stream: ERROR: requested WAL segment
000000010000000000000003 has already been removed
2024-12-29 01:21:37.592 IST [427345] LOG: waiting for WAL to become
available at 0/3000110
2024-12-29 01:21:42.593 IST [427358] LOG: started streaming WAL from
primary at 0/3000000 on timeline 1
2024-12-29 01:21:42.593 IST [427358] FATAL: could not receive data
from WAL stream: ERROR: requested WAL segment
000000010000000000000003 has already been removed

This issue was previously reported in [1], with a suggestion to raise
a warning in [2]. I’ve implemented a patch that logs a warning in
dry-run mode. This will give users the opportunity to adjust the
max_slot_wal_keep_size value before running the command.

Thoughts?

[1] - https://www.postgresql.org/message-id/TY3PR01MB9889FEDBBF74A9F79CA7CC87F57A2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[2] - https://www.postgresql.org/message-id/be92c57b-82e1-4920-ac31-a8a04206db7b%40app.fastmail.com

Thanks and regards,
Shubham Khanna.

On Mon, 30 Dec 2024 at 09:34, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> Hi,
>
> Currently, there is a risk that pg_createsubscriber may fail to
> complete successfully when the max_slot_wal_keep_size value is set too
> low. This can occur if the WAL is removed before the standby using the
> replication slot is able to complete replication, as the required WAL
> files are no longer available.
>
> I was able to reproduce this issue using the following steps:
> Set up a streaming replication environment.
> Run pg_createsubscriber in a debugger.
> Pause pg_createsubscriber at the setup_recovery stage.
> Perform several operations on the primary node to generate a large
> volume of WAL, causing older WAL segments to be removed due to the low
> max_slot_wal_keep_size setting.
> Once the necessary WAL segments are deleted, continue the execution of
> pg_createsubscriber.
> At this point, pg_createsubscriber fails with the following error:
> 2024-12-29 01:21:37.590 IST [427353] FATAL: could not receive data
> from WAL stream: ERROR: requested WAL segment
> 000000010000000000000003 has already been removed
> 2024-12-29 01:21:37.592 IST [427345] LOG: waiting for WAL to become
> available at 0/3000110
> 2024-12-29 01:21:42.593 IST [427358] LOG: started streaming WAL from
> primary at 0/3000000 on timeline 1
> 2024-12-29 01:21:42.593 IST [427358] FATAL: could not receive data
> from WAL stream: ERROR: requested WAL segment
> 000000010000000000000003 has already been removed
>
> This issue was previously reported in [1], with a suggestion to raise
> a warning in [2]. I’ve implemented a patch that logs a warning in
> dry-run mode. This will give users the opportunity to adjust the
> max_slot_wal_keep_size value before running the command.
>
> Thoughts?

+1 for throwing a warning in dry-run mode

Few comments:
1) We can have this check only in dry-run mode, it is not required in
non dry-run mode as there is nothing much user can do once the tool is
running, we can change this:
+ if (max_slot_wal_keep_size != -1)
+ {
+ pg_log_warning("publisher requires
'max_slot_wal_keep_size = -1', but only %d remain",
+ max_slot_wal_keep_size);
+ pg_log_warning_detail("Change the
'max_slot_wal_keep_size' configuration on the publisher to -1.");
+ }

to:
+ if (dry_run && max_slot_wal_keep_size != -1)
+ {
+ pg_log_warning("publisher requires
'max_slot_wal_keep_size = -1', but only %d remain",
+ max_slot_wal_keep_size);
+ pg_log_warning_detail("Change the
'max_slot_wal_keep_size' configuration on the publisher to -1.");
+ }

2) This error message is not quite right, can we change it to
"publisher requires max_slot_wal_keep_size to be -1, but is set to %d"
+ if (max_slot_wal_keep_size != -1)
+ {
+ pg_log_warning("publisher requires
'max_slot_wal_keep_size = -1', but only %d remain",
+ max_slot_wal_keep_size);
+ pg_log_warning_detail("Change the
'max_slot_wal_keep_size' configuration on the publisher to -1.");
+ }

3) Also the configuration could be specified in format specifier like
it is specified in the earlier case

Regards,
Vignesh

On Mon, Dec 30, 2024 at 10:10 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 30 Dec 2024 at 09:34, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > Currently, there is a risk that pg_createsubscriber may fail to
> > complete successfully when the max_slot_wal_keep_size value is set too
> > low. This can occur if the WAL is removed before the standby using the
> > replication slot is able to complete replication, as the required WAL
> > files are no longer available.
> >
> > I was able to reproduce this issue using the following steps:
> > Set up a streaming replication environment.
> > Run pg_createsubscriber in a debugger.
> > Pause pg_createsubscriber at the setup_recovery stage.
> > Perform several operations on the primary node to generate a large
> > volume of WAL, causing older WAL segments to be removed due to the low
> > max_slot_wal_keep_size setting.
> > Once the necessary WAL segments are deleted, continue the execution of
> > pg_createsubscriber.
> > At this point, pg_createsubscriber fails with the following error:
> > 2024-12-29 01:21:37.590 IST [427353] FATAL: could not receive data
> > from WAL stream: ERROR: requested WAL segment
> > 000000010000000000000003 has already been removed
> > 2024-12-29 01:21:37.592 IST [427345] LOG: waiting for WAL to become
> > available at 0/3000110
> > 2024-12-29 01:21:42.593 IST [427358] LOG: started streaming WAL from
> > primary at 0/3000000 on timeline 1
> > 2024-12-29 01:21:42.593 IST [427358] FATAL: could not receive data
> > from WAL stream: ERROR: requested WAL segment
> > 000000010000000000000003 has already been removed
> >
> > This issue was previously reported in [1], with a suggestion to raise
> > a warning in [2]. I’ve implemented a patch that logs a warning in
> > dry-run mode. This will give users the opportunity to adjust the
> > max_slot_wal_keep_size value before running the command.
> >
> > Thoughts?
>
> +1 for throwing a warning in dry-run mode
>
> Few comments:
> 1) We can have this check only in dry-run mode, it is not required in
> non dry-run mode as there is nothing much user can do once the tool is
> running, we can change this:
> + if (max_slot_wal_keep_size != -1)
> + {
> + pg_log_warning("publisher requires
> 'max_slot_wal_keep_size = -1', but only %d remain",
> + max_slot_wal_keep_size);
> + pg_log_warning_detail("Change the
> 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> + }
>
> to:
> + if (dry_run && max_slot_wal_keep_size != -1)
> + {
> + pg_log_warning("publisher requires
> 'max_slot_wal_keep_size = -1', but only %d remain",
> + max_slot_wal_keep_size);
> + pg_log_warning_detail("Change the
> 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> + }
>
> 2) This error message is not quite right, can we change it to
> "publisher requires max_slot_wal_keep_size to be -1, but is set to %d"
> + if (max_slot_wal_keep_size != -1)
> + {
> + pg_log_warning("publisher requires
> 'max_slot_wal_keep_size = -1', but only %d remain",
> + max_slot_wal_keep_size);
> + pg_log_warning_detail("Change the
> 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> + }
>
> 3) Also the configuration could be specified in format specifier like
> it is specified in the earlier case
>

I have fixed the given comments. The attached patch contains the
suggested changes.

Thanks and regards,
Shubham Khanna.

On Mon, 30 Dec 2024 at 12:04, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Mon, Dec 30, 2024 at 10:10 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Mon, 30 Dec 2024 at 09:34, Shubham Khanna
> > <khannashubham1197(at)gmail(dot)com> wrote:
> > >
> > > Hi,
> > >
> > > Currently, there is a risk that pg_createsubscriber may fail to
> > > complete successfully when the max_slot_wal_keep_size value is set too
> > > low. This can occur if the WAL is removed before the standby using the
> > > replication slot is able to complete replication, as the required WAL
> > > files are no longer available.
> > >
> > > I was able to reproduce this issue using the following steps:
> > > Set up a streaming replication environment.
> > > Run pg_createsubscriber in a debugger.
> > > Pause pg_createsubscriber at the setup_recovery stage.
> > > Perform several operations on the primary node to generate a large
> > > volume of WAL, causing older WAL segments to be removed due to the low
> > > max_slot_wal_keep_size setting.
> > > Once the necessary WAL segments are deleted, continue the execution of
> > > pg_createsubscriber.
> > > At this point, pg_createsubscriber fails with the following error:
> > > 2024-12-29 01:21:37.590 IST [427353] FATAL: could not receive data
> > > from WAL stream: ERROR: requested WAL segment
> > > 000000010000000000000003 has already been removed
> > > 2024-12-29 01:21:37.592 IST [427345] LOG: waiting for WAL to become
> > > available at 0/3000110
> > > 2024-12-29 01:21:42.593 IST [427358] LOG: started streaming WAL from
> > > primary at 0/3000000 on timeline 1
> > > 2024-12-29 01:21:42.593 IST [427358] FATAL: could not receive data
> > > from WAL stream: ERROR: requested WAL segment
> > > 000000010000000000000003 has already been removed
> > >
> > > This issue was previously reported in [1], with a suggestion to raise
> > > a warning in [2]. I’ve implemented a patch that logs a warning in
> > > dry-run mode. This will give users the opportunity to adjust the
> > > max_slot_wal_keep_size value before running the command.
> > >
> > > Thoughts?
> >
> > +1 for throwing a warning in dry-run mode
> >
> > Few comments:
> > 1) We can have this check only in dry-run mode, it is not required in
> > non dry-run mode as there is nothing much user can do once the tool is
> > running, we can change this:
> > + if (max_slot_wal_keep_size != -1)
> > + {
> > + pg_log_warning("publisher requires
> > 'max_slot_wal_keep_size = -1', but only %d remain",
> > + max_slot_wal_keep_size);
> > + pg_log_warning_detail("Change the
> > 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> > + }
> >
> > to:
> > + if (dry_run && max_slot_wal_keep_size != -1)
> > + {
> > + pg_log_warning("publisher requires
> > 'max_slot_wal_keep_size = -1', but only %d remain",
> > + max_slot_wal_keep_size);
> > + pg_log_warning_detail("Change the
> > 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> > + }
> >
> > 2) This error message is not quite right, can we change it to
> > "publisher requires max_slot_wal_keep_size to be -1, but is set to %d"
> > + if (max_slot_wal_keep_size != -1)
> > + {
> > + pg_log_warning("publisher requires
> > 'max_slot_wal_keep_size = -1', but only %d remain",
> > + max_slot_wal_keep_size);
> > + pg_log_warning_detail("Change the
> > 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> > + }
> >
> > 3) Also the configuration could be specified in format specifier like
> > it is specified in the earlier case
> >
>
> I have fixed the given comments. The attached patch contains the
> suggested changes.

Few comments:
1) Since this configuration will be updated after reload, you can
reload instead of restarting:
+# Configure 'max_slot_wal_keep_size = 1' on the publisher and verify the
+# warning message
+$node_p->append_conf('postgresql.conf', 'max_slot_wal_keep_size = 1');
+$node_p->restart;

2) You can reset max_slot_wal_keep_size configuration after this test:
+ 0,
+ [qr/./],
+ [
+ qr/pg_createsubscriber: warning: publisher requires
max_slot_wal_keep_size to be -1/,
+ qr/Change the configuration parameter
"max_slot_wal_keep_size" on the publisher to -1./,
+ ],
+ 'Validate warning for misconfigured max_slot_wal_keep_size on
the publisher'
+);

3) We could update the comment to also mention the possibility of
required wal files being deleted with non-default
max_slot_wal_keep_size.
+ /* Validate max_slot_wal_keep_size */
+ 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);
+ }

4) You can update the commit message saying "the possibility of
required wal files being deleted with non-default
max_slot_wal_keep_size" and also mention that this warning will be
raised in dry_run mode.

Regards,
Vignesh

On Mon, Dec 30, 2024 at 4:03 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 30 Dec 2024 at 12:04, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Mon, Dec 30, 2024 at 10:10 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Mon, 30 Dec 2024 at 09:34, Shubham Khanna
> > > <khannashubham1197(at)gmail(dot)com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Currently, there is a risk that pg_createsubscriber may fail to
> > > > complete successfully when the max_slot_wal_keep_size value is set too
> > > > low. This can occur if the WAL is removed before the standby using the
> > > > replication slot is able to complete replication, as the required WAL
> > > > files are no longer available.
> > > >
> > > > I was able to reproduce this issue using the following steps:
> > > > Set up a streaming replication environment.
> > > > Run pg_createsubscriber in a debugger.
> > > > Pause pg_createsubscriber at the setup_recovery stage.
> > > > Perform several operations on the primary node to generate a large
> > > > volume of WAL, causing older WAL segments to be removed due to the low
> > > > max_slot_wal_keep_size setting.
> > > > Once the necessary WAL segments are deleted, continue the execution of
> > > > pg_createsubscriber.
> > > > At this point, pg_createsubscriber fails with the following error:
> > > > 2024-12-29 01:21:37.590 IST [427353] FATAL: could not receive data
> > > > from WAL stream: ERROR: requested WAL segment
> > > > 000000010000000000000003 has already been removed
> > > > 2024-12-29 01:21:37.592 IST [427345] LOG: waiting for WAL to become
> > > > available at 0/3000110
> > > > 2024-12-29 01:21:42.593 IST [427358] LOG: started streaming WAL from
> > > > primary at 0/3000000 on timeline 1
> > > > 2024-12-29 01:21:42.593 IST [427358] FATAL: could not receive data
> > > > from WAL stream: ERROR: requested WAL segment
> > > > 000000010000000000000003 has already been removed
> > > >
> > > > This issue was previously reported in [1], with a suggestion to raise
> > > > a warning in [2]. I’ve implemented a patch that logs a warning in
> > > > dry-run mode. This will give users the opportunity to adjust the
> > > > max_slot_wal_keep_size value before running the command.
> > > >
> > > > Thoughts?
> > >
> > > +1 for throwing a warning in dry-run mode
> > >
> > > Few comments:
> > > 1) We can have this check only in dry-run mode, it is not required in
> > > non dry-run mode as there is nothing much user can do once the tool is
> > > running, we can change this:
> > > + if (max_slot_wal_keep_size != -1)
> > > + {
> > > + pg_log_warning("publisher requires
> > > 'max_slot_wal_keep_size = -1', but only %d remain",
> > > + max_slot_wal_keep_size);
> > > + pg_log_warning_detail("Change the
> > > 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> > > + }
> > >
> > > to:
> > > + if (dry_run && max_slot_wal_keep_size != -1)
> > > + {
> > > + pg_log_warning("publisher requires
> > > 'max_slot_wal_keep_size = -1', but only %d remain",
> > > + max_slot_wal_keep_size);
> > > + pg_log_warning_detail("Change the
> > > 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> > > + }
> > >
> > > 2) This error message is not quite right, can we change it to
> > > "publisher requires max_slot_wal_keep_size to be -1, but is set to %d"
> > > + if (max_slot_wal_keep_size != -1)
> > > + {
> > > + pg_log_warning("publisher requires
> > > 'max_slot_wal_keep_size = -1', but only %d remain",
> > > + max_slot_wal_keep_size);
> > > + pg_log_warning_detail("Change the
> > > 'max_slot_wal_keep_size' configuration on the publisher to -1.");
> > > + }
> > >
> > > 3) Also the configuration could be specified in format specifier like
> > > it is specified in the earlier case
> > >
> >
> > I have fixed the given comments. The attached patch contains the
> > suggested changes.
>
> Few comments:
> 1) Since this configuration will be updated after reload, you can
> reload instead of restarting:
> +# Configure 'max_slot_wal_keep_size = 1' on the publisher and verify the
> +# warning message
> +$node_p->append_conf('postgresql.conf', 'max_slot_wal_keep_size = 1');
> +$node_p->restart;
>
> 2) You can reset max_slot_wal_keep_size configuration after this test:
> + 0,
> + [qr/./],
> + [
> + qr/pg_createsubscriber: warning: publisher requires
> max_slot_wal_keep_size to be -1/,
> + qr/Change the configuration parameter
> "max_slot_wal_keep_size" on the publisher to -1./,
> + ],
> + 'Validate warning for misconfigured max_slot_wal_keep_size on
> the publisher'
> +);
>
> 3) We could update the comment to also mention the possibility of
> required wal files being deleted with non-default
> max_slot_wal_keep_size.
> + /* Validate max_slot_wal_keep_size */
> + 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);
> + }
>
> 4) You can update the commit message saying "the possibility of
> required wal files being deleted with non-default
> max_slot_wal_keep_size" and also mention that this warning will be
> raised in dry_run mode.
>

I have fixed the given comments. The attached patch contains the
suggested changes.

Thanks and regards,
Shubham Khanna.

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.

======
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.

======
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.

~~~

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.

~

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.

~

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?

======
[1] https://www.postgresql.org/docs/17/app-pgcreatesubscriber.html

Kind Regards,
Peter Smith.
Fujitsu Australia

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.

Hi Shubham.

Here are some review comments for the patch v4-0001.

======
Commit message.

1.
The 'pg_createsubscriber' utility is updated to fetch and validate the
'max_slot_wal_keep_size' setting from the publisher. A warning is raised during
the '--dry-run' mode if the configuration is set to a non-default value.

~

This says a "warning is raised", but patch v4 changed the warning to
an error, so this description is incompatible with the code.

======
doc/src/sgml/ref/pg_createsubscriber.sgml

2.
+ <para>
+ The 'max_slot_wal_keep_size' must be set to -1 to prevent the automatic
+ removal of WAL files needed by replication slots. Setting this parameter to
+ a specific size may lead to replication failures if required WAL files are
+ prematurely deleted.
+ </para>

The 'max_slot_wal_keep_size' should not be single-quoted like that. It
should use the same markup as the other nearby GUC names and also have
a link like those others do.

======
src/bin/pg_basebackup/pg_createsubscriber.c

3.
+/*
+ * 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.
+ */

3a.
Not fixed? For patch v3 I had already posted [1-#4a] that this entire
comment is wrongly indented.

~

3b.
That first sentence looks like it is missing a period and a following
blank line. OTOH, maybe the first sentence is not even necessary.

~~~

4.
+ if (dry_run && max_slot_wal_keep_size != -1)
+ {
+ pg_log_error("publisher requires max_slot_wal_keep_size to be -1,
but only %d remain",
+ max_slot_wal_keep_size);
+ pg_log_error_hint("Change the configuration parameter \"%s\" on the
publisher to %d.",
+ "max_slot_wal_keep_size", -1);
+ failed = true;
+ }
+

4a.
Not fixed? For patch v3 I had already posted [1-#4b] that it seemed
strange you did not use the \"%s\" substitution for the GUC name of
the first message (unlike the 2nd message).

I also think it is strange that the 1st message uses a hardwired -1,
but the 2nd message uses a parameter for the -1.

~~

4b.
I don't think "but only %d remain" is the appropriate wording for this
GUC. It looks like a cut/paste mistake from a previous message.
Anyway, maybe this message should be something quite different so it
is not just duplicating the same information as the error_hint. e.g.
see below for one idea. This could also resolve the previous review
comment.

SUGGESTION
"publisher requires WAL size must not be restricted"

~

4c.
I saw that this only emits the message in dry-mode, which is
apparently based on the suggestion from Vignesh [2-#1]. Meanwhile, all
the comments/docs say "it may cause replication failures", so I felt
it might be better to always stop everything up-front if there is a
wrong configuration, instead of waiting for potential failures to
happen -- that's why I had suggested using error instead of a warning,
but maybe I am in the minority.

IIUC there are two ways to address this configuration problem:

A. Give warnings, but only in dry-run mode. If the warnings are
ignored (or if the dry-run was not even done), there may be
replication failures later, but we HOPE it will not happen.

B. Give errors in all modes. The early config error prevents any
chance of later replication errors.

So, I think the implementation needs to choose either A or B.
Currently, it seems a mixture.

======
[1] my v3 review -
https://www.postgresql.org/message-id/CAHut%2BPsgEphCa-P-3Q7cORA%3D1TRv15UUsaxN9ZkX56M1-J_QRg%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm09cRzke52UN5zx33PT390whU92oXY4gfOSZEo17CLPjw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Jan 2, 2025 at 4:20 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham.
>
> Here are some review comments for the patch v4-0001.
>
> ======
> Commit message.
>
> 1.
> The 'pg_createsubscriber' utility is updated to fetch and validate the
> 'max_slot_wal_keep_size' setting from the publisher. A warning is raised during
> the '--dry-run' mode if the configuration is set to a non-default value.
>
>
> This says a "warning is raised", but patch v4 changed the warning to
> an error, so this description is incompatible with the code.
>

Since, this patch now raises a warning instead of an error so I think
this should be the same as before.

> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 2.
> + <para>
> + The 'max_slot_wal_keep_size' must be set to -1 to prevent the automatic
> + removal of WAL files needed by replication slots. Setting this parameter to
> + a specific size may lead to replication failures if required WAL files are
> + prematurely deleted.
> + </para>
>
> The 'max_slot_wal_keep_size' should not be single-quoted like that. It
> should use the same markup as the other nearby GUC names and also have
> a link like those others do.
>

Added the link for 'max_slot_wal_keep_size' and updated the
'pg_createsubscriber' documentation.

> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 3.
> +/*
> + * 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.
> + */
>
> 3a.
> Not fixed? For patch v3 I had already posted [1-#4a] that this entire
> comment is wrongly indented.
>

Fixed.

>
> 3b.
> That first sentence looks like it is missing a period and a following
> blank line. OTOH, maybe the first sentence is not even necessary.
>

Fixed.

>
> 4.
> + if (dry_run && max_slot_wal_keep_size != -1)
> + {
> + pg_log_error("publisher requires max_slot_wal_keep_size to be -1,
> but only %d remain",
> + max_slot_wal_keep_size);
> + pg_log_error_hint("Change the configuration parameter \"%s\" on the
> publisher to %d.",
> + "max_slot_wal_keep_size", -1);
> + failed = true;
> + }
> +
>
> 4a.
> Not fixed? For patch v3 I had already posted [1-#4b] that it seemed
> strange you did not use the \"%s\" substitution for the GUC name of
> the first message (unlike the 2nd message).
>
> I also think it is strange that the 1st message uses a hardwired -1,
> but the 2nd message uses a parameter for the -1.
>

Updated the warning message and warning detail.

>
> 4b.
> I don't think "but only %d remain" is the appropriate wording for this
> GUC. It looks like a cut/paste mistake from a previous message.
> Anyway, maybe this message should be something quite different so it
> is not just duplicating the same information as the error_hint. e.g.
> see below for one idea. This could also resolve the previous review
> comment.
>
> SUGGESTION
> "publisher requires WAL size must not be restricted"
>

I have used your suggestion in the latest patch.

>
> 4c.
> I saw that this only emits the message in dry-mode, which is
> apparently based on the suggestion from Vignesh [2-#1]. Meanwhile, all
> the comments/docs say "it may cause replication failures", so I felt
> it might be better to always stop everything up-front if there is a
> wrong configuration, instead of waiting for potential failures to
> happen -- that's why I had suggested using error instead of a warning,
> but maybe I am in the minority.
>
> IIUC there are two ways to address this configuration problem:
>
> A. Give warnings, but only in dry-run mode. If the warnings are
> ignored (or if the dry-run was not even done), there may be
> replication failures later, but we HOPE it will not happen.
>
> B. Give errors in all modes. The early config error prevents any
> chance of later replication errors.
>
> So, I think the implementation needs to choose either A or B.
> Currently, it seems a mixture.
>
> ======
> [1] my v3 review -
> https://www.postgresql.org/message-id/CAHut%2BPsgEphCa-P-3Q7cORA%3D1TRv15UUsaxN9ZkX56M1-J_QRg%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/CALDaNm09cRzke52UN5zx33PT390whU92oXY4gfOSZEo17CLPjw%40mail.gmail.com

If 'max_slot_wal_keep_size' is not set to -1, there is no surety that
'pg_createsubscriber' will function as expected and the removal of WAL
files will start to occur. Therefore, a warning is raised instead of
an error to alert users about the potential configuration issue.
If 'max_slot_wal_keep_size' is -1 (the default), replication slots may
retain an unlimited amount of WAL files.

The attached Patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Jan 3, 2025 at 8:06 AM Peter Smith <smithpb2250(at)gmail(dot)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.

Hi Shubham.

The patch v6-0001 LGTM.

OTOH, if you want to be picky, the docs wording could be slightly
modified to be more consistent with the coded warning message.

CURRENT
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.

SUGGESTION
Replication failures can occur if required WAL files are missing. To
prevent this, the source server must set <xref
linkend="guc-max-slot-wal-keep-size"/> to <literal>-1</literal> to
ensure that required WAL files are not prematurely removed.

~~~

See the attached NITPICKS diff if you want to make this change.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Shubham,

Thanks for creating a patch. I have one comment about it.

check_publisher() assumed that the SQL function `pg_catalog.current_setting('max_slot_wal_keep_size')`
will return the numeric, but it just return the text representation. I.e., if the parameter is
set to 10MB, the function returns like below:

```
postgres=# SELECT * FROM pg_catalog.current_setting('max_slot_wal_keep_size');
current_setting
-----------------
10MB
(1 row)
```

Your patch can work well because atoi() ignores the latter part of the string,
e.g., "10MB" is converted to "10", but this is not clean. I suggest either of
1) accepting the value as the string, or 2) using an SQL function pg_size_bytes()
to get max_slot_wal_keep_size.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Jan 6, 2025 at 3:22 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham.
>
> The patch v6-0001 LGTM.
>
> OTOH, if you want to be picky, the docs wording could be slightly
> modified to be more consistent with the coded warning message.
>
> CURRENT
> 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.
>
> SUGGESTION
> Replication failures can occur if required WAL files are missing. To
> prevent this, the source server must set <xref
> linkend="guc-max-slot-wal-keep-size"/> to <literal>-1</literal> to
> ensure that required WAL files are not prematurely removed.
>
> ~~~
>
> See the attached NITPICKS diff if you want to make this change.
>

I have used your suggestion and updated the 'pg_createsubscriber'
documentation accordingly.
The attached Patch contains the suggested change.

Thanks and regards,
Shubham Khanna.

On Mon, Jan 6, 2025 at 7:59 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> Thanks for creating a patch. I have one comment about it.
>
> check_publisher() assumed that the SQL function `pg_catalog.current_setting('max_slot_wal_keep_size')`
> will return the numeric, but it just return the text representation. I.e., if the parameter is
> set to 10MB, the function returns like below:
>
> ```
> postgres=# SELECT * FROM pg_catalog.current_setting('max_slot_wal_keep_size');
> current_setting
> -----------------
> 10MB
> (1 row)
> ```
>
> Your patch can work well because atoi() ignores the latter part of the string,
> e.g., "10MB" is converted to "10", but this is not clean. I suggest either of
> 1) accepting the value as the string, or 2) using an SQL function pg_size_bytes()
> to get max_slot_wal_keep_size.
>

I have fixed the given comment. The v7 version Patch attached at [1]
has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8Rj%2BSxmBgz-vw5f7meRzEh6pgp9YhAjL5BPzTpevu31rY7Q%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

Dear Shubham,

Thanks for updating the patch. Few comments.

```
+#include "common/int.h" /* include for strtoi64 */
```

I could build the source code without the inclusion. Can you remove?

```
+ max_slot_wal_keep_size = strtoi64(PQgetvalue(res, 0, 6), NULL, 0);
+
+ /* use strtoi64 to convert the string result to a 64-bit integer */
+ if (max_slot_wal_keep_size == 0 && errno != 0)
+ {
+ /* handle conversion error */
+ pg_log_error("Failed to convert max_slot_wal_keep_size to int64");
+ }
```

I'm not sure the error handling is really needed. pg_dump also uses the function and it does
not have such handlings.

```
+ pg_log_debug("publisher: max_slot_wal_keep_size: %ld",
+ max_slot_wal_keep_size);
```

IIUC, "%ld" does not always represent int64 format. Since it is a debug message, isn't it enough to
use INT64_FORMAT macro?

```
+# Configure 'max_slot_wal_keep_size = 1' on the publisher and
+# reload configuration
+$node_p->append_conf('postgresql.conf', 'max_slot_wal_keep_size = 1');
+$node_p->reload;
```

Can you use 'max_slot_wal_keep_size = 10MB' or something to test the handling is correct?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Jan 9, 2025 at 12:46 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch. Few comments.
>
> ```
> +#include "common/int.h" /* include for strtoi64 */
> ```
>
> I could build the source code without the inclusion. Can you remove?
>
> ```
> + max_slot_wal_keep_size = strtoi64(PQgetvalue(res, 0, 6), NULL, 0);
> +
> + /* use strtoi64 to convert the string result to a 64-bit integer */
> + if (max_slot_wal_keep_size == 0 && errno != 0)
> + {
> + /* handle conversion error */
> + pg_log_error("Failed to convert max_slot_wal_keep_size to int64");
> + }
> ```
>
> I'm not sure the error handling is really needed. pg_dump also uses the function and it does
> not have such handlings.
>
> ```
> + pg_log_debug("publisher: max_slot_wal_keep_size: %ld",
> + max_slot_wal_keep_size);
> ```
>
> IIUC, "%ld" does not always represent int64 format. Since it is a debug message, isn't it enough to
> use INT64_FORMAT macro?
>
> ```
> +# Configure 'max_slot_wal_keep_size = 1' on the publisher and
> +# reload configuration
> +$node_p->append_conf('postgresql.conf', 'max_slot_wal_keep_size = 1');
> +$node_p->reload;
> ```
>
> Can you use 'max_slot_wal_keep_size = 10MB' or something to test the handling is correct?
>

The attached Patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

Hi Shubham,

Some review comments for patch v8=0001.

======
Commit message.

1.
By ensuring 'max_slot_wal_keep_size' is -1, this patch prevents the potential
deletion of required WAL files on the publisher that could disrupt logical
replication. A test case has been added to validate correct warning reporting
when 'max_slot_wal_keep_size' is misconfigured.

~

AFAIK the patch only logs a warning. It is not *enforcing* anything at all.

So, saying "ensuring" and "preventing" is misleading.

======
src/bin/pg_basebackup/pg_createsubscriber.c

check_publisher:

2.
+ max_slot_wal_keep_size = strtoi64(PQgetvalue(res, 0, 6), NULL, 0);

Is passing base 0 here ok?

======
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl

3.
+# reload configuration
+$node_p->append_conf('postgresql.conf', 'max_slot_wal_keep_size = 10MB');
+$node_p->reload;
+
+# dry run mode on node S and verify the warning message for misconfigured
+# 'max_slot_wal_keep_size'
+command_checks_all(

Because of the --verbose I expected to see the pg_log_debug message
getting output showing the large (10MB) value for
max_slot_wal_keep_size.

But, I can only see a value of -1 in the log file
'tmp_check/log/regress_log_040_pg_createsubscriber', which may not
even be from the same test. Am I looking in the wrong place (???) e.g.
Where is the logging evidence of that publisher's bad GUC (10MB) value
in the logs before the warning is emitted?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Jan 6, 2025 at 1:29 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> Thanks for creating a patch. I have one comment about it.
>
> check_publisher() assumed that the SQL function `pg_catalog.current_setting('max_slot_wal_keep_size')`
> will return the numeric, but it just return the text representation. I.e., if the parameter is
> set to 10MB, the function returns like below:
>
> ```
> postgres=# SELECT * FROM pg_catalog.current_setting('max_slot_wal_keep_size');
> current_setting
> -----------------
> 10MB
> (1 row)
> ```
>
> Your patch can work well because atoi() ignores the latter part of the string,
> e.g., "10MB" is converted to "10", but this is not clean. I suggest either of
> 1) accepting the value as the string, or 2) using an SQL function pg_size_bytes()
> to get max_slot_wal_keep_size.
>

Hi Shubham.

Kuroda-san gave a couple of ideas above and you chose the option 2.

FWIW, recently I've been thinking that option 1 might have been a
better choice, because:
i) Using strtoi64 for a GUC value seems to be very rare. I didn't
find any other examples of what you've ended up doing in v8-0001.
ii) When you display the value in the pg_log_debug it would be better
to display the same human-readable format that the user configured
instead of some possibly huge int64 value
iii) AFAIK, we only need to check this against "-1". The actual value
is of no real importance to the patch, so going to extra trouble to
extract an int64 that we don't care about seems unnecessary

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Hi Shubham,

In a previous review [1, comment #3] I was complaining about the lack
of output emitted by the pg_log_debug() because I wanted to see some
LOG evidence of the bad value of max_slot_wal_keep_size.

FYI, I think that mystery has finally been solved.

AFAIK 2 separate problems were contributing to the lack of debug output.

1. Unlike the 'command_ok' subroutine, the 'command_checks_all'
subroutine apparently redirects the output before doing the requested
pattern matching on it. So there is no output to be seen.

2. After that, I did not realise the effect of '-verbose' is
cumulative on loglevel. Specifically, to see debug messages it is not
enough to just say '--verbose'. In fact, you need to specify it 2
times: '--verbose', '--verbose'

~

After addressing these (e.g. by executing the same pg_createsubscriber
using 'command_ok' and using a double '--verbose'), the expected LOGS
can be seen.

e.g. in the file tmp_check/log/regress_log_040_pg_createsubscriber
----------
...
pg_createsubscriber: checking settings on publisher
pg_createsubscriber: publisher: wal_level: logical
pg_createsubscriber: publisher: max_replication_slots: 10
pg_createsubscriber: publisher: current replication slots: 2
pg_createsubscriber: publisher: max_wal_senders: 10
pg_createsubscriber: publisher: current wal senders: 1
pg_createsubscriber: publisher: max_prepared_transactions: 0
pg_createsubscriber: publisher: max_slot_wal_keep_size: 10485760
pg_createsubscriber: warning: publisher requires WAL size must not be restricted
pg_createsubscriber: hint: Set the configuration parameter
"max_slot_wal_keep_size" to -1 to ensure that required WAL files are
not prematurely removed.
pg_createsubscriber: stopping the subscriber
...
----------

See the attached diff for the TAP changes I used to expose this
logging. Apply this atop v8.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPszk61QM%2BcEvq_1-A2y%2BJrAD0USB%2BNvtcidajYOfHDkyw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Jan 10, 2025 at 6:56 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Some review comments for patch v8=0001.
>
> ======
> Commit message.
>
> 1.
> By ensuring 'max_slot_wal_keep_size' is -1, this patch prevents the potential
> deletion of required WAL files on the publisher that could disrupt logical
> replication. A test case has been added to validate correct warning reporting
> when 'max_slot_wal_keep_size' is misconfigured.
>
> ~
>
> AFAIK the patch only logs a warning. It is not *enforcing* anything at all.
>
> So, saying "ensuring" and "preventing" is misleading.
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> check_publisher:
>
> 2.
> + max_slot_wal_keep_size = strtoi64(PQgetvalue(res, 0, 6), NULL, 0);
>
> Is passing base 0 here ok?
>
> ======
> src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
>
> 3.
> +# reload configuration
> +$node_p->append_conf('postgresql.conf', 'max_slot_wal_keep_size = 10MB');
> +$node_p->reload;
> +
> +# dry run mode on node S and verify the warning message for misconfigured
> +# 'max_slot_wal_keep_size'
> +command_checks_all(
>
> Because of the --verbose I expected to see the pg_log_debug message
> getting output showing the large (10MB) value for
> max_slot_wal_keep_size.
>
> But, I can only see a value of -1 in the log file
> 'tmp_check/log/regress_log_040_pg_createsubscriber', which may not
> even be from the same test. Am I looking in the wrong place (???) e.g.
> Where is the logging evidence of that publisher's bad GUC (10MB) value
> in the logs before the warning is emitted?
>

I have fixed the given comments using your suggestions. The attached
patch contains the suggested changes.

Thanks and Regards,
Shubham Khanna.

On Fri, Jan 10, 2025 at 8:21 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Mon, Jan 6, 2025 at 1:29 PM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Dear Shubham,
> >
> > Thanks for creating a patch. I have one comment about it.
> >
> > check_publisher() assumed that the SQL function `pg_catalog.current_setting('max_slot_wal_keep_size')`
> > will return the numeric, but it just return the text representation. I.e., if the parameter is
> > set to 10MB, the function returns like below:
> >
> > ```
> > postgres=# SELECT * FROM pg_catalog.current_setting('max_slot_wal_keep_size');
> > current_setting
> > -----------------
> > 10MB
> > (1 row)
> > ```
> >
> > Your patch can work well because atoi() ignores the latter part of the string,
> > e.g., "10MB" is converted to "10", but this is not clean. I suggest either of
> > 1) accepting the value as the string, or 2) using an SQL function pg_size_bytes()
> > to get max_slot_wal_keep_size.
> >
>
> Hi Shubham.
>
> Kuroda-san gave a couple of ideas above and you chose the option 2.
>
> FWIW, recently I've been thinking that option 1 might have been a
> better choice, because:
> i) Using strtoi64 for a GUC value seems to be very rare. I didn't
> find any other examples of what you've ended up doing in v8-0001.
> ii) When you display the value in the pg_log_debug it would be better
> to display the same human-readable format that the user configured
> instead of some possibly huge int64 value
> iii) AFAIK, we only need to check this against "-1". The actual value
> is of no real importance to the patch, so going to extra trouble to
> extract an int64 that we don't care about seems unnecessary
>

I have fixed the given comment and used the string to accept the
value. The v9 version Patch attached at [1] has the changes for the
same.
[1] - https://www.postgresql.org/message-id/CAHv8RjL4MFEDosz9%2BTFmufYCDoNCnyN6PFeSxG6MwsH1ufhZLA%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

On Tue, Jan 14, 2025 at 10:10 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> In a previous review [1, comment #3] I was complaining about the lack
> of output emitted by the pg_log_debug() because I wanted to see some
> LOG evidence of the bad value of max_slot_wal_keep_size.
>
> FYI, I think that mystery has finally been solved.
>
> AFAIK 2 separate problems were contributing to the lack of debug output.
>
> 1. Unlike the 'command_ok' subroutine, the 'command_checks_all'
> subroutine apparently redirects the output before doing the requested
> pattern matching on it. So there is no output to be seen.
>
> 2. After that, I did not realise the effect of '-verbose' is
> cumulative on loglevel. Specifically, to see debug messages it is not
> enough to just say '--verbose'. In fact, you need to specify it 2
> times: '--verbose', '--verbose'
>
> ~
>
> After addressing these (e.g. by executing the same pg_createsubscriber
> using 'command_ok' and using a double '--verbose'), the expected LOGS
> can be seen.
>
> e.g. in the file tmp_check/log/regress_log_040_pg_createsubscriber
> ----------
> ...
> pg_createsubscriber: checking settings on publisher
> pg_createsubscriber: publisher: wal_level: logical
> pg_createsubscriber: publisher: max_replication_slots: 10
> pg_createsubscriber: publisher: current replication slots: 2
> pg_createsubscriber: publisher: max_wal_senders: 10
> pg_createsubscriber: publisher: current wal senders: 1
> pg_createsubscriber: publisher: max_prepared_transactions: 0
> pg_createsubscriber: publisher: max_slot_wal_keep_size: 10485760
> pg_createsubscriber: warning: publisher requires WAL size must not be restricted
> pg_createsubscriber: hint: Set the configuration parameter
> "max_slot_wal_keep_size" to -1 to ensure that required WAL files are
> not prematurely removed.
> pg_createsubscriber: stopping the subscriber
> ...
> ----------
>
> See the attached diff for the TAP changes I used to expose this
> logging. Apply this atop v8.
>
> ======
> [1] https://www.postgresql.org/message-id/CAHut%2BPszk61QM%2BcEvq_1-A2y%2BJrAD0USB%2BNvtcidajYOfHDkyw%40mail.gmail.com
>

I have used your diff to update the TAP tests accordingly. The v9
version Patch attached at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8RjL4MFEDosz9%2BTFmufYCDoNCnyN6PFeSxG6MwsH1ufhZLA%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

Hi Shubham.

Patch v9-0001 LGTM.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Jan 15, 2025 at 3:28 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham.
>
> Patch v9-0001 LGTM.
>

Upon reviewing the v9-0001 patch, I noticed that running 'pg perltidy'
was necessary. To ensure the patch adheres to the required coding
standards, I have applied 'pg perltidy' and made the necessary
adjustments.
The attached patch includes these formatting fixes along with the
latest changes.

Thanks and regards,
Shubham Khanna.

On Thu, 16 Jan 2025 at 10:48, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Wed, Jan 15, 2025 at 3:28 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Shubham.
> >
> > Patch v9-0001 LGTM.
> >
>
> Upon reviewing the v9-0001 patch, I noticed that running 'pg perltidy'
> was necessary. To ensure the patch adheres to the required coding
> standards, I have applied 'pg perltidy' and made the necessary
> adjustments.
> The attached patch includes these formatting fixes along with the
> latest changes.
>
Hi Shubham,

I reviewed the v10 patch. I have following comments:

1. The documentation is added under the prerequisite section. And the
prerequisite section states that
' There are some prerequisites for
<application>pg_createsubscriber</application> to convert the target server
into a logical replica. If these are not met, an error will be reported.'

But for our case we are giving a warning only during the dry run. So,
I feel we should add the documentation under the 'Warnings' section.

2. Spacing is not consistent with the rest of document:

+ Replication failures can occur if required WAL files are missing. To prevent
+ this, the source server must set <xref
linkend="guc-max-slot-wal-keep-size"/>
+ to <literal>-1</literal> to ensure that required WAL files are not
+ prematurely removed.

For the above added line, we should add a space before each line.

Thanks and Regards,
Shlok Kyal

On Thu, Jan 16, 2025 at 12:12 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Thu, 16 Jan 2025 at 10:48, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Wed, Jan 15, 2025 at 3:28 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Hi Shubham.
> > >
> > > Patch v9-0001 LGTM.
> > >
> >
> > Upon reviewing the v9-0001 patch, I noticed that running 'pg perltidy'
> > was necessary. To ensure the patch adheres to the required coding
> > standards, I have applied 'pg perltidy' and made the necessary
> > adjustments.
> > The attached patch includes these formatting fixes along with the
> > latest changes.
> >
> Hi Shubham,
>
> I reviewed the v10 patch. I have following comments:
>
> 1. The documentation is added under the prerequisite section. And the
> prerequisite section states that
> ' There are some prerequisites for
> <application>pg_createsubscriber</application> to convert the target server
> into a logical replica. If these are not met, an error will be reported.'
>
> But for our case we are giving a warning only during the dry run. So,
> I feel we should add the documentation under the 'Warnings' section.
>
> 2. Spacing is not consistent with the rest of document:
>
> + Replication failures can occur if required WAL files are missing. To prevent
> + this, the source server must set <xref
> linkend="guc-max-slot-wal-keep-size"/>
> + to <literal>-1</literal> to ensure that required WAL files are not
> + prematurely removed.
>
> For the above added line, we should add a space before each line.
>

Fixed the given comments. The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Thu, 16 Jan 2025 at 12:34, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Thu, Jan 16, 2025 at 12:12 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Thu, 16 Jan 2025 at 10:48, Shubham Khanna
> > <khannashubham1197(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Jan 15, 2025 at 3:28 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > >
> > > > Hi Shubham.
> > > >
> > > > Patch v9-0001 LGTM.
> > > >
> > >
> > > Upon reviewing the v9-0001 patch, I noticed that running 'pg perltidy'
> > > was necessary. To ensure the patch adheres to the required coding
> > > standards, I have applied 'pg perltidy' and made the necessary
> > > adjustments.
> > > The attached patch includes these formatting fixes along with the
> > > latest changes.
> > >
> > Hi Shubham,
> >
> > I reviewed the v10 patch. I have following comments:
> >
> > 1. The documentation is added under the prerequisite section. And the
> > prerequisite section states that
> > ' There are some prerequisites for
> > <application>pg_createsubscriber</application> to convert the target server
> > into a logical replica. If these are not met, an error will be reported.'
> >
> > But for our case we are giving a warning only during the dry run. So,
> > I feel we should add the documentation under the 'Warnings' section.
> >
> > 2. Spacing is not consistent with the rest of document:
> >
> > + Replication failures can occur if required WAL files are missing. To prevent
> > + this, the source server must set <xref
> > linkend="guc-max-slot-wal-keep-size"/>
> > + to <literal>-1</literal> to ensure that required WAL files are not
> > + prematurely removed.
> >
> > For the above added line, we should add a space before each line.
> >
>
> Fixed the given comments. The attached patch contains the suggested changes.
>
Hi Shubham,

I reviewed the v11 patch and it looks good to me.

Thanks and Regards,
Shlok Kyal

Patch v11-0001 LGTM.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Jan 17, 2025 at 3:39 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Patch v11-0001 LGTM.
>
> ======

Following the recent comments on Patch v11-0001, the patch was not
applied to the HEAD. I have addressed the feedback and prepared a
rebased version to incorporate the necessary adjustments.
Please find the updated patch.

Thanks and regards,
Shubham Khanna.

Hi Shubham.

The v12-0001 works OK, but your added TAP tests of this patch do not
conform to the new style of the fat-comma parameter passing since the
recent commit [1], so you'll need to fix them so they do...

======
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl

1.
+command_checks_all(
+ [
+ 'pg_createsubscriber', '--verbose',
+ '--verbose', '--recovery-timeout',
+ "$PostgreSQL::Test::Utils::timeout_default", '--dry-run',
+ '--pgdata', $node_s->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_s->host,
+ '--subscriber-port', $node_s->port,
+ '--publication', 'pub1',
+ '--publication', 'pub2',
+ '--subscription', 'sub1',
+ '--subscription', 'sub2',
+ '--database', $db1,
+ '--database', $db2
+ ],

1a.
Fix the formatting. e.g. Look at master for example what new style
parameters look like and make yours look the same.

~

1b.
Here I think we don't really any --verbose at all because AFAIK
command_checks_all is not producing any output we can look at anyhow.

~~~

2.
+# And, same again to see the output...
command_ok(
[
- 'pg_createsubscriber',
- '--verbose',
- '--dry-run',
- '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
- '--pgdata' => $node_s->data_dir,
- '--publisher-server' => $node_p->connstr($db1),
- '--socketdir' => $node_s->host,
- '--subscriber-port' => $node_s->port,
- '--publication' => 'pub1',
- '--publication' => 'pub2',
- '--subscription' => 'sub1',
- '--subscription' => 'sub2',
- '--database' => $db1,
- '--database' => $db2,
+ 'pg_createsubscriber', '--verbose',
+ '--verbose', '--recovery-timeout',
+ "$PostgreSQL::Test::Utils::timeout_default", '--dry-run',
+ '--pgdata', $node_s->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_s->host,
+ '--subscriber-port', $node_s->port,
+ '--publication', 'pub1',
+ '--publication', 'pub2',
+ '--subscription', 'sub1',
+ '--subscription', 'sub2',
+ '--database', $db1,
+ '--database', $db2
],

2a.
+# And, same again to see the output...

Maybe a better comment is more like

# Execute the same command again but this time use 'command_ok' so
that the log files can be inspected.

~

2b.
Fix the formatting. Look at master for example what new style
parameters look like and make yours look the same.

~

2c.
Include some comment (like exists elsewhere in this file) to say
"--verbose is used twice to show more information.", otherwise readers
could be tempted to think the double --verbose is a mistake.

~~~

3.
Rerun perltidy (the correct version) to ensure the formatting is
stable and good.

======
[1] https://github.com/postgres/postgres/commit/ce1b0f9da03e1cebc293f60b378079b22bd7cc0f

Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Jan 23, 2025 at 12:30 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham.
>
> The v12-0001 works OK, but your added TAP tests of this patch do not
> conform to the new style of the fat-comma parameter passing since the
> recent commit [1], so you'll need to fix them so they do...
>
> ======
> src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
>
> 1.
> +command_checks_all(
> + [
> + 'pg_createsubscriber', '--verbose',
> + '--verbose', '--recovery-timeout',
> + "$PostgreSQL::Test::Utils::timeout_default", '--dry-run',
> + '--pgdata', $node_s->data_dir,
> + '--publisher-server', $node_p->connstr($db1),
> + '--socketdir', $node_s->host,
> + '--subscriber-port', $node_s->port,
> + '--publication', 'pub1',
> + '--publication', 'pub2',
> + '--subscription', 'sub1',
> + '--subscription', 'sub2',
> + '--database', $db1,
> + '--database', $db2
> + ],
>
>
> 1a.
> Fix the formatting. e.g. Look at master for example what new style
> parameters look like and make yours look the same.
>
> ~
>
> 1b.
> Here I think we don't really any --verbose at all because AFAIK
> command_checks_all is not producing any output we can look at anyhow.
>
> ~~~
>
> 2.
> +# And, same again to see the output...
> command_ok(
> [
> - 'pg_createsubscriber',
> - '--verbose',
> - '--dry-run',
> - '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
> - '--pgdata' => $node_s->data_dir,
> - '--publisher-server' => $node_p->connstr($db1),
> - '--socketdir' => $node_s->host,
> - '--subscriber-port' => $node_s->port,
> - '--publication' => 'pub1',
> - '--publication' => 'pub2',
> - '--subscription' => 'sub1',
> - '--subscription' => 'sub2',
> - '--database' => $db1,
> - '--database' => $db2,
> + 'pg_createsubscriber', '--verbose',
> + '--verbose', '--recovery-timeout',
> + "$PostgreSQL::Test::Utils::timeout_default", '--dry-run',
> + '--pgdata', $node_s->data_dir,
> + '--publisher-server', $node_p->connstr($db1),
> + '--socketdir', $node_s->host,
> + '--subscriber-port', $node_s->port,
> + '--publication', 'pub1',
> + '--publication', 'pub2',
> + '--subscription', 'sub1',
> + '--subscription', 'sub2',
> + '--database', $db1,
> + '--database', $db2
> ],
>
> 2a.
> +# And, same again to see the output...
>
> Maybe a better comment is more like
>
> # Execute the same command again but this time use 'command_ok' so
> that the log files can be inspected.
>
> ~
>
> 2b.
> Fix the formatting. Look at master for example what new style
> parameters look like and make yours look the same.
>
> ~
>
> 2c.
> Include some comment (like exists elsewhere in this file) to say
> "--verbose is used twice to show more information.", otherwise readers
> could be tempted to think the double --verbose is a mistake.
>
> ~~~
>
> 3.
> Rerun perltidy (the correct version) to ensure the formatting is
> stable and good.
>
> ======

Fixed the given comments. The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

Patch v13-0001 LGTM.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Jan 23, 2025 at 6:42 PM Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> Fixed the given comments. The attached patch contains the suggested changes.
>

I am fine with giving a WARNING for max_slot_wal_keep_size but I don't
think we need a test for each and every setting required for
pg_createsubscriber. This will add time to the tests without adding
much value. So, I don't think this patch deserves an additional test.

--
With Regards,
Amit Kapila.

On Mon, Feb 17, 2025 at 4:06 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jan 23, 2025 at 6:42 PM Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > Fixed the given comments. The attached patch contains the suggested changes.
> >
>
> I am fine with giving a WARNING for max_slot_wal_keep_size but I don't
> think we need a test for each and every setting required for
> pg_createsubscriber. This will add time to the tests without adding
> much value. So, I don't think this patch deserves an additional test.
>

I understand the concern that adding test cases for every required
setting in pg_createsubscriber may unnecessarily increase test
execution time without providing significant additional value.
Based on this, I have removed the additional test cases as they do not
offer substantial benefits to the overall test suite.

The attached Patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Mon, Feb 17, 2025 at 4:23 PM Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> The attached Patch contains the suggested changes.
>

Pushed after making a minor change in the WARNING message.

--
With Regards,
Amit Kapila.