On Fri, Jan 10, 2025 at 8:21 AM Peter Smith <smithpb2...@gmail.com> wrote: > > 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 >
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.