On Thu, Jan 9, 2025 at 12:46 PM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.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.
v8-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
