On Wed, Dec 30, 2015 at 8:28 PM, Pavel Stehule <pavel.steh...@gmail.com> wrote:
> > > 2015-12-30 17:33 GMT+01:00 Robert Haas <robertmh...@gmail.com>: > >> On Mon, Dec 28, 2015 at 8:45 AM, Shulgin, Oleksandr >> <oleksandr.shul...@zalando.de> wrote: >> > I didn't check out earlier versions of this patch, but the latest one >> still >> > changes pg_size_pretty() to emit PB suffix. >> > >> > I don't think it is worth it to throw a number of changes together like >> > that. We should focus on adding pg_size_bytes() first and make it >> > compatible with both pg_size_pretty() and existing GUC units: that is >> > support suffixes up to TB and make sure they have the meaning of powers >> of >> > 2^10, not 10^3. Re-using the table present in guc.c would be a plus. >> > >> > Next, we could think about adding handling of PB suffix on input and >> output, >> > but I don't see a big problem if that is emitted as 1024TB or the user >> has >> > to specify it as 1024TB in a GUC or argument to pg_size_bytes(): an >> minor >> > inconvenience only. >> >> +1 to everything in this email. >> > > so I removed support for PB and SI units. Now the > memory_unit_conversion_table is shared. > Looks better, thanks. I'm not sure why the need to touch the regression test for pg_size_pretty(): ! 10.5 | 10.5 bytes | -10.5 bytes ! 1000.5 | 1000.5 bytes | -1000.5 bytes ! 1000000.5 | 977 kB | -977 kB ! 1000000000.5 | 954 MB | -954 MB ! 1000000000000.5 | 931 GB | -931 GB ! 1000000000000000.5 | 909 TB | -909 TB A nitpick, this loop: + while (*cp) + { + if ((isdigit(*cp) || *cp == '.') && ndigits < MAX_DIGITS) + digits[ndigits++] = *cp++; + else + break; + } would be a bit easier to parse if spelled as: + while (*cp && (isdigit(*cp) || *cp == '.') && ndigits < MAX_DIGITS) + digits[ndigits++] = *cp++; On the other hand, this seems to truncate the digits silently: + digits[ndigits] = '\0'; I don't think we want that, e.g: postgres=# select pg_size_bytes('9223372036854775807.9'); ERROR: invalid unit "9" HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB". I think making a mutable copy of the input string and truncating it before passing to numeric_in() would make more sense--no need to hard-code MAX_DIGITS. The same goes for hard-coding MAX_UNIT_LEN, e.g. compare the following two outputs: postgres=# select pg_size_bytes('1 KiB'); ERROR: invalid unit "KiB" HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB". postgres=# select pg_size_bytes('1024 bytes'); ERROR: invalid format I believe we should see a similar error message and a hint in the latter case. (No, I don't think we should add support for 'bytes' as a unit, not even for "compatibility" with pg_size_pretty()--for one, I don't think it would be wise to expect pg_size_bytes() to be able to deparse *every* possible output produced by pg_size_pretty() as it's purpose is human-readable display; also, pg_size_pretty() can easily produce output that doesn't fit into bigint type, or is just negative) Code comments and doc change need proof-reading by a native English speaker, which I am not. -- Alex