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
