On Fri, Jan 10, 2025 at 8:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Jan 6, 2025 at 1:29 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@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
>
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.
