Hello,Pavel! On 1/26/16, Pavel Stehule <pavel.steh...@gmail.com> wrote: > I am grateful for review - now this patch is better, and I hope near final > stage. > > Regards > Pavel
I'm pretty sure we are close to it. Now besides a code design I mentioned[1] before (and no one has answered me about it), there are a few small notes. Now you have all messages in one style ("invalid size: \"%s\"") except size units ("invalid unit \"%s\", there are two places). I guess according to the Error Style Guide[2] it should be like "errmsg("invalid size: \"%s\"", str), errdetail("Invalid size unit \"%s\"", unitstr),". If it is not hard for you, it'll look better if "select" is uppercased in tests and a comment about sharing "memory_unit_conversion_table" is close to the "SELECT pg_size_bytes('1PB');". Moreover your comment has a flaw: "pg_size_pretty" doesn't use that array at all, so the phrase "These functions shares" is wrong. Also I've just found that numeric format supports values with exponent like "1e5" which is not allowed in pg_size_bytes. I guess the phrase "The format is numeric with an optional size unit" should be replaced to "The format is a fixed point number with an optional size unit" in the documentation. Have a look for a formatting: in the rows "num = DatumGetNumeric", "mul_num = DatumGetNumeric", "num = DatumGetNumeric" and in error reporting close to the "/* only alphabetic character are allowed */" parameters in the next lines are not under the first parameters. I insist there is no reason to call "pfree" for "str" and "buffer". But if you do so, clean up also buffers from numeric_in, numeric_mul and int8_numeric. If you insist it should be left as is, I leave that decision to a committer. P.S.: Have you thought to wrap the call "numeric_in" by a PG_TRY/PG_CATCH instead of checking for correctness by yourself? [1]http://www.postgresql.org/message-id/cakoswnm+ssggkysv09n_6hhfwzmrr+csjpgfhbpff5nnpfj...@mail.gmail.com [2]http://www.postgresql.org/docs/devel/static/error-style-guide.html#AEN111165 -- Best regards, Vitaly Burovoy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers