On Mon, Jan 6, 2025 at 1:29 PM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.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
