On Fri, Jan 10, 2025 at 6:56 AM Peter Smith <smithpb2...@gmail.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.
v9-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
