On 2/17/16, Dean Rasheed <dean.a.rash...@gmail.com> wrote: > On 17 February 2016 at 00:39, Vitaly Burovoy <vitaly.buro...@gmail.com> > wrote: >> Now parse_memory_unit returns -1024 for bytes as divider, constant >> "bytes" has moved there. >> Add new memory_units_bytes_hint which differs from an original >> memory_units_int by "bytes" size unit. >> Allow hintmsg be NULL and if so, skip setting dereferenced variable to >> memory_units_bytes_hint. >> > > I think that approach is getting more and more unwieldy, and it simply > isn't worth the effort just to share a few values from the unit > conversion table, especially given that the set of supported units > differs between the two places. > >> On 2/16/16, Dean Rasheed <dean.a.rash...@gmail.com> wrote: >>> ISTM that it would be far less code, and much simpler and more >>> readable to just parse the supported units directly in >>> pg_size_bytes(), rather than trying to share code with guc.c, when the >>> supported units are actually different and may well diverge further in >>> the future. > > I've gone with this approach and it is indeed far less code, and much > simpler and easier to read. This will also make it easier to > maintain/extend in the future. > > I've made a few minor tweaks to the docs, and added a note to make it > clear that the units in these functions work in powers of 2 not 10. > > I also took the opportunity to tidy up the number scanning code > somewhat (I was tempted to rip it out entirely, since it feels awfully > close to duplicating the numeric code, but it's probably worth it for > the better error message).
Yes, the better error message was the only reason to leave that scan. > Additionally, note that I replaced strcasecmp() with pg_strcasecmp(), > since AIUI the former is not available on all supported platforms. Thank you. I didn't know that function exists. > Barring objections, and subject to some more testing, I intend to > commit this version. > > Regards, > Dean Than you for your work. Your version seems even better that refactored by me. But I have several questions (below). > + bool have_digits; Agree, "main_digits" and "error" together was redundant, "have_digits" is enough. > + else > + have_digits = false; Does it worth to move it to the declaration and remove that "else" block? + bool have_digits = false; Is it important to use: > + ObjectIdGetDatum(InvalidOid), > + Int32GetDatum(-1))); instead of "0, -1"? Previously I left immediate constants because I found the same thing in jsonb.c (rows 363, 774)... > + if (pg_strcasecmp(strptr, "bytes") == 0) > + else if > + else if > + else if It seems little ugly for me. In that case (not using values from GUC) I'd create a static array for it and do a small cycle via it. Would it better? > + errmsg("invalid size: \"%s\"", text_to_cstring(arg)), Agree. Usage of "text_to_cstring" again is a good idea for improving readability. > + NumericGetDatum(mul_num), Two more space for indentation. Also I've found that with allowing exponent there is a weird result (we tried to avoid "type numeric" in the error message): SELECT pg_size_bytes('123e100000000000000000000000000000000000000000000000000000000000000000000000 kB'); ERROR: invalid input syntax for type numeric: "123e100000000000000000000000000000000000000000000000000000000000000000000000" http://www.postgresql.org/docs/9.5/static/error-style-guide.html#AEN110702 -- Best regards, Vitaly Burovoy -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers