Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values
On Fri, Nov 6, 2015 at 12:44 PM, Adrian Vondendriesch wrote: > Am 06.11.2015 um 17:06 schrieb Robert Haas: >> On Thu, Nov 5, 2015 at 4:19 PM, Adrian.Vondendriesch >> wrote: >>> New patch attached and rebased on HEAD >>> (8c75ad436f75fc629b61f601ba884c8f9313c9af). >> >> I've committed this with some modifications: >> >> - I changed the comment for the half_rounded() macros because the one >> you had just restated the code. >> - I tightened up the coding in numeric_half_rounded() very slightly. >> - You didn't, as far as I can see, modify the regression test schedule >> to execute the files you added. I consolidated them into one file, >> added it to the schedule, and tightened up the SQL a bit. > > Looks much better now. > >> >> Thanks for the patch, and please let me know if I muffed anything. > > Thanks for reviewing and improving the changes. > > I changed the status to committed. Great, thank you for working on this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values
Am 06.11.2015 um 17:06 schrieb Robert Haas: > On Thu, Nov 5, 2015 at 4:19 PM, Adrian.Vondendriesch > wrote: >> New patch attached and rebased on HEAD >> (8c75ad436f75fc629b61f601ba884c8f9313c9af). > > I've committed this with some modifications: > > - I changed the comment for the half_rounded() macros because the one > you had just restated the code. > - I tightened up the coding in numeric_half_rounded() very slightly. > - You didn't, as far as I can see, modify the regression test schedule > to execute the files you added. I consolidated them into one file, > added it to the schedule, and tightened up the SQL a bit. Looks much better now. > > Thanks for the patch, and please let me know if I muffed anything. Thanks for reviewing and improving the changes. I changed the status to committed. Regards, - Adrian signature.asc Description: OpenPGP digital signature
Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values
On Thu, Nov 5, 2015 at 4:19 PM, Adrian.Vondendriesch wrote: > New patch attached and rebased on HEAD > (8c75ad436f75fc629b61f601ba884c8f9313c9af). I've committed this with some modifications: - I changed the comment for the half_rounded() macros because the one you had just restated the code. - I tightened up the coding in numeric_half_rounded() very slightly. - You didn't, as far as I can see, modify the regression test schedule to execute the files you added. I consolidated them into one file, added it to the schedule, and tightened up the SQL a bit. Thanks for the patch, and please let me know if I muffed anything. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values
New patch attached and rebased on HEAD (8c75ad436f75fc629b61f601ba884c8f9313c9af). Am 03.11.2015 um 04:06 schrieb Robert Haas: > On Sat, Oct 31, 2015 at 2:25 PM, Julien Rouhaud > wrote: >> I just reviewed your patch, everything looks fine for me. Maybe some >> minor cosmetic changes could be made to avoid declaring too many vars, >> but I think a committer would have a better idea on this, so I mark >> this patch as ready for committer. > > I don't think we should define Sign(x) as a macro in c.h. c.h is > really only supposed to contain stuff that's pretty generic and > universal, and the fact that we haven't needed it up until now > suggests that Sign(x) isn't. I'd suggest instead defining something > like: > > #define half_rounded(x) (((x) + (x) < 0 ? 0 : 1) / 2) I removed the Sign macro and introduced half_rounded. It's placed it in dbsize.c. Please let me know if that's the wrong place. > > Maybe rename numeric_divide_by_two to numeric_half_rounded. > Generally, let's try to make the numeric and int64 code paths look as > similar as possible. I renamed numeric_divide_by_two. > > Recomputing numeric_absolute(x) multiple times should be avoided. > Compute it once and save the answer. Because "size" is shifted multiple times within pg_size_pretty and pg_size_pretty_numeric the absolute values needs to be recomputed. Please let me know if I oversee something. I changed the status back to "needs review". Regards, - Adrian From c06d1463cf21957260327c47217050e334058422 Mon Sep 17 00:00:00 2001 From: Adrian Vondendriesch Date: Sat, 19 Sep 2015 15:54:13 +0200 Subject: [PATCH] Make pg_size_pretty handle negative values. Make pg_size_pretty(bigint) and pg_size_pretty(numeric) handle negative values the same way as positive values are. This commit introduces a new macro "Sign", which is used within pg_size_pretty(bigint). Also numeric_plus_one_over_two is renamed to numeric_divide_by_two. numeric_divide_by_two now handles negative values the same way as positive values are handled. To get the absolute value of a Numeric variable, a new static function called numeric_absolute is introduced. --- src/backend/utils/adt/dbsize.c | 67 --- .../regress/expected/pg_size_pretty_bigint.out | 39 +++ .../regress/expected/pg_size_pretty_numeric.out| 75 ++ src/test/regress/sql/pg_size_pretty_bigint.sql | 15 + src/test/regress/sql/pg_size_pretty_numeric.sql| 29 + 5 files changed, 203 insertions(+), 22 deletions(-) create mode 100644 src/test/regress/expected/pg_size_pretty_bigint.out create mode 100644 src/test/regress/expected/pg_size_pretty_numeric.out create mode 100644 src/test/regress/sql/pg_size_pretty_bigint.sql create mode 100644 src/test/regress/sql/pg_size_pretty_numeric.sql diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index 82311b4..84c67d3 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -31,6 +31,12 @@ #include "utils/relmapper.h" #include "utils/syscache.h" +/* + * half_rounded + * Return (x / 2) if x is smaller than 0 + * ((x + 1) / 2) otherwise. + */ +#define half_rounded(x) (((x) + ((x) < 0 ? 0 : 1)) / 2) /* Return physical size of directory contents, or 0 if dir doesn't exist */ static int64 @@ -534,31 +540,31 @@ pg_size_pretty(PG_FUNCTION_ARGS) int64 limit = 10 * 1024; int64 limit2 = limit * 2 - 1; - if (size < limit) + if (Abs(size) < limit) snprintf(buf, sizeof(buf), INT64_FORMAT " bytes", size); else { size >>= 9;/* keep one extra bit for rounding */ - if (size < limit2) + if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " kB", - (size + 1) / 2); + half_rounded(size)); else { size >>= 10; - if (size < limit2) + if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " MB", - (size + 1) / 2); + half_rounded(size)); else { size >>= 10; -if (size < limit2) +if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " GB", - (size + 1) / 2); + half_rounded(size)); else { size >>= 10; snprintf(buf, sizeof(buf), INT64_FORMAT " TB", - (size + 1) / 2); + half_rounded(size)); } } } @@ -593,16 +599,37 @@ numeric_is_less(Numeric a, Numeric b) } static Numeric -numeric_plus_one_over_two(Numeric n) +numeric_absolute(Numeric n) +{ + Datum d = NumericGetDatum(n); + Datum result; + + result = DirectFunctionCall1(numeric_abs, d); + return DatumGetNumeric(result); +} + +static Numeric +numeric_half_rounded(Numeric n) { Datum d = NumericGetDatum(n); + Datum zero; Datum one; Datum two; Datum result; + bool greater_or_equal_zero; + zero = DirectFunctionCall1(int8_numeric, Int64GetDatum(0)); one = DirectFunctionCall1(int8_numeric, Int64GetDatum(1)); two = DirectFunctionCall1(int8_numeric, Int64Ge
Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values
On 03/11/2015 04:06, Robert Haas wrote: > On Sat, Oct 31, 2015 at 2:25 PM, Julien Rouhaud > wrote: >> I just reviewed your patch, everything looks fine for me. Maybe some >> minor cosmetic changes could be made to avoid declaring too many vars, >> but I think a committer would have a better idea on this, so I mark >> this patch as ready for committer. > > I don't think we should define Sign(x) as a macro in c.h. c.h is > really only supposed to contain stuff that's pretty generic and > universal, and the fact that we haven't needed it up until now > suggests that Sign(x) isn't. I'd suggest instead defining something > like: > > #define half_rounded(x) (((x) + (x) < 0 ? 0 : 1) / 2) > > Maybe rename numeric_divide_by_two to numeric_half_rounded. > Generally, let's try to make the numeric and int64 code paths look as > similar as possible. > > Recomputing numeric_absolute(x) multiple times should be avoided. > Compute it once and save the answer. > Thanks for these comments. I therefore change the status to waiting on author. Regards. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values
On Sat, Oct 31, 2015 at 2:25 PM, Julien Rouhaud wrote: > I just reviewed your patch, everything looks fine for me. Maybe some > minor cosmetic changes could be made to avoid declaring too many vars, > but I think a committer would have a better idea on this, so I mark > this patch as ready for committer. I don't think we should define Sign(x) as a macro in c.h. c.h is really only supposed to contain stuff that's pretty generic and universal, and the fact that we haven't needed it up until now suggests that Sign(x) isn't. I'd suggest instead defining something like: #define half_rounded(x) (((x) + (x) < 0 ? 0 : 1) / 2) Maybe rename numeric_divide_by_two to numeric_half_rounded. Generally, let's try to make the numeric and int64 code paths look as similar as possible. Recomputing numeric_absolute(x) multiple times should be avoided. Compute it once and save the answer. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values
On 20/09/2015 14:16, Adrian.Vondendriesch wrote: > Hi all, > Hello, > Am 06.04.2015 um 20:52 schrieb Tom Lane: >> "David G. Johnston" writes: >>> I'll let a hacker determine whether this is a bug or a feature >>> request though it is a POLA violation in either case. >> >> I'd say it's a feature request --- a perfectly reasonable one, >> but I doubt we'd alter the behavior of the function in the back >> branches. > > I was also wondering about the described behaviour. IMO > pg_size_pretty should handle negative values the same way as > positive values are handled. > +1 for me, thanks for the patch! > I've attached a patch which implements the requested behaviour. > The patch applies clean to HEAD (currently 85eda7e92). > I just reviewed your patch, everything looks fine for me. Maybe some minor cosmetic changes could be made to avoid declaring too many vars, but I think a committer would have a better idea on this, so I mark this patch as ready for committer. > AFAICS the documentation doesn't say anything about pg_size_pretty > and negative values. So, I didn't touch the documentation. If this > is a oversight by me or should be documented after all, I will > provide a additional documentation patch. > Yes, the documentation didn't say anything about the lack of formattting for negative value. I also don't think a patch is needed here. Regards. > Before the patch: > >> SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM >> (SELECT 100::bigint) foo(size); pg_size_pretty | >> pg_size_pretty + 91 TB >> | -100 bytes (1 row) > >> SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM >> (SELECT 100::numeric) foo(size); pg_size_pretty | >> pg_size_pretty + 91 TB >> | -100 bytes (1 row) > > After the patch: > >> SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM >> (SELECT 100::bigint) foo(size); pg_size_pretty | >> pg_size_pretty + 91 TB | >> -91 TB (1 row) > >> SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM >> (SELECT 100::numeric) foo(size); pg_size_pretty | >> pg_size_pretty + 91 TB | >> -91 TB (1 row) > > > The patch contains two tests (pg_size_pretty_bigint and > pg_size_pretty_numeric), to verify that positive and negative > values return the same result (except sign). > > Greetings, > > - Adrian > -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values
Hi all, Am 06.04.2015 um 20:52 schrieb Tom Lane: > "David G. Johnston" writes: >> I'll let a hacker determine whether this is a bug or a feature request >> though it is a POLA violation in either case. > > I'd say it's a feature request --- a perfectly reasonable one, but I doubt > we'd alter the behavior of the function in the back branches. I was also wondering about the described behaviour. IMO pg_size_pretty should handle negative values the same way as positive values are handled. I've attached a patch which implements the requested behaviour. The patch applies clean to HEAD (currently 85eda7e92). AFAICS the documentation doesn't say anything about pg_size_pretty and negative values. So, I didn't touch the documentation. If this is a oversight by me or should be documented after all, I will provide a additional documentation patch. Before the patch: > SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT > 100::bigint) foo(size); > pg_size_pretty | pg_size_pretty > + > 91 TB | -100 bytes > (1 row) > SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT > 100::numeric) foo(size); > pg_size_pretty | pg_size_pretty > + > 91 TB | -100 bytes > (1 row) After the patch: > SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT > 100::bigint) foo(size); > pg_size_pretty | pg_size_pretty > + > 91 TB | -91 TB > (1 row) > SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT > 100::numeric) foo(size); > pg_size_pretty | pg_size_pretty > + > 91 TB | -91 TB > (1 row) The patch contains two tests (pg_size_pretty_bigint and pg_size_pretty_numeric), to verify that positive and negative values return the same result (except sign). Greetings, - Adrian From 08ae6cdeaf261e156ce5f7622f0d7426c1249485 Mon Sep 17 00:00:00 2001 From: Adrian Vondendriesch Date: Sat, 19 Sep 2015 15:54:13 +0200 Subject: [PATCH] Make pg_size_pretty handle negative values. Make pg_size_pretty(bigint) and pg_size_pretty(numeric) handle negative values the same way as positive values are. This commit introduces a new macro "Sign", which is used within pg_size_pretty(bigint). Also numeric_plus_one_over_two is renamed to numeric_divide_by_two. numeric_divide_by_two now handles negative values the same way as positive values are handled. To get the absolute value of a Numeric variable, a new static function called numeric_absolute is introduced. --- src/backend/utils/adt/dbsize.c | 61 +++--- src/include/c.h| 6 ++ .../regress/expected/pg_size_pretty_bigint.out | 39 +++ .../regress/expected/pg_size_pretty_numeric.out| 75 ++ src/test/regress/sql/pg_size_pretty_bigint.sql | 15 + src/test/regress/sql/pg_size_pretty_numeric.sql| 29 + 6 files changed, 203 insertions(+), 22 deletions(-) create mode 100644 src/test/regress/expected/pg_size_pretty_bigint.out create mode 100644 src/test/regress/expected/pg_size_pretty_numeric.out create mode 100644 src/test/regress/sql/pg_size_pretty_bigint.sql create mode 100644 src/test/regress/sql/pg_size_pretty_numeric.sql diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index 82311b4..97095bb 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -534,31 +534,31 @@ pg_size_pretty(PG_FUNCTION_ARGS) int64 limit = 10 * 1024; int64 limit2 = limit * 2 - 1; - if (size < limit) + if (Abs(size) < limit) snprintf(buf, sizeof(buf), INT64_FORMAT " bytes", size); else { size >>= 9;/* keep one extra bit for rounding */ - if (size < limit2) + if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " kB", - (size + 1) / 2); + (size + Sign(size)) / 2); else { size >>= 10; - if (size < limit2) + if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " MB", - (size + 1) / 2); + (size + Sign(size)) / 2); else { size >>= 10; -if (size < limit2) +if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " GB", - (size + 1) / 2); + (size + Sign(size)) / 2); else { size >>= 10; snprintf(buf, sizeof(buf), INT64_FORMAT " TB", - (size + 1) / 2); + (size + Sign(size)) / 2); } } } @@ -593,16 +593,37 @@ numeric_is_less(Numeric a, Numeric b) } static Numeric -numeric_plus_one_over_two(Numeric n) +numeric_absolute(Numeric n) { Datum d = NumericGetDatum(n); + Datum result; + + result = DirectFunctionCall1(numeric_abs, d); + return DatumGetNumeric(result); +} + +static Numeric +numeric_divide_by_two(Numeric n) +{ + Datum d =