BUG #12989: pg_size_pretty with negative values
The following bug has been logged on the website:
Bug reference: 12989
Logged by: Christian Almeida
Email address: cbalmeida@gmail.com
PostgreSQL version: 9.3.6
Operating system: Ubuntu 14.04 LTS
Description:
The function "pg_size_pretty" is not formatting negative numbers.
Example:
select pg_size_pretty(+123456789::bigint),
pg_size_pretty(-123456789::bigint)
Output:
----------------------------
118 MB -123456789 bytes
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, Apr 6, 2015 at 10:30 AM, <cbalmeida@gmail.com> wrote:
The following bug has been logged on the website:
Bug reference: 12989
Logged by: Christian Almeida
Email address: cbalmeida@gmail.com
PostgreSQL version: 9.3.6
Operating system: Ubuntu 14.04 LTS
Description:The function "pg_size_pretty" is not formatting negative numbers.
Example:
select pg_size_pretty(+123456789::bigint),
pg_size_pretty(-123456789::bigint)Output:
----------------------------
118 MB -123456789 bytes
Do you want the result to be "-118 MB" or "ERROR: invalid input - size
cannot be negative (-123456789)"?
David J.
Of course a file size will never be negative, but a size "delta" can be and
considering the function purpose, I think it should output "-118 MB".
Christian Almeida
2015-04-06 14:45 GMT-03:00 David G. Johnston <david.g.johnston@gmail.com>:
Show quoted text
On Mon, Apr 6, 2015 at 10:30 AM, <cbalmeida@gmail.com> wrote:
The following bug has been logged on the website:
Bug reference: 12989
Logged by: Christian Almeida
Email address: cbalmeida@gmail.com
PostgreSQL version: 9.3.6
Operating system: Ubuntu 14.04 LTS
Description:The function "pg_size_pretty" is not formatting negative numbers.
Example:
select pg_size_pretty(+123456789::bigint),
pg_size_pretty(-123456789::bigint)Output:
----------------------------
118 MB -123456789 bytesDo you want the result to be "-118 MB" or "ERROR: invalid input - size
cannot be negative (-123456789)"?David J.
That's what I get for being quippy...my bad.
I'll let a hacker determine whether this is a bug or a feature request
though it is a POLA violation in either case.
David J.
On Mon, Apr 6, 2015 at 10:54 AM, Christian Almeida <cbalmeida@gmail.com>
wrote:
Show quoted text
Of course a file size will never be negative, but a size "delta" can be
and considering the function purpose, I think it should output "-118 MB".Christian Almeida
2015-04-06 14:45 GMT-03:00 David G. Johnston <david.g.johnston@gmail.com>:
On Mon, Apr 6, 2015 at 10:30 AM, <cbalmeida@gmail.com> wrote:
The following bug has been logged on the website:
Bug reference: 12989
Logged by: Christian Almeida
Email address: cbalmeida@gmail.com
PostgreSQL version: 9.3.6
Operating system: Ubuntu 14.04 LTS
Description:The function "pg_size_pretty" is not formatting negative numbers.
Example:
select pg_size_pretty(+123456789::bigint),
pg_size_pretty(-123456789::bigint)Output:
----------------------------
118 MB -123456789 bytesDo you want the result to be "-118 MB" or "ERROR: invalid input - size
cannot be negative (-123456789)"?David J.
"David G. Johnston" <david.g.johnston@gmail.com> 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.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Hi all,
Am 06.04.2015 um 20:52 schrieb Tom Lane:
"David G. Johnston" <david.g.johnston@gmail.com> 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 100000000000000::bigint) foo(size);
pg_size_pretty | pg_size_pretty
----------------+------------------------
91 TB | -100000000000000 bytes
(1 row)
SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 100000000000000::numeric) foo(size);
pg_size_pretty | pg_size_pretty
----------------+------------------------
91 TB | -100000000000000 bytes
(1 row)
After the patch:
SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 100000000000000::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 100000000000000::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
Attachments:
0001-Make-pg_size_pretty-handle-negative-values-v1.patchtext/x-patch; name=0001-Make-pg_size_pretty-handle-negative-values-v1.patchDownload
From 08ae6cdeaf261e156ce5f7622f0d7426c1249485 Mon Sep 17 00:00:00 2001
From: Adrian Vondendriesch <adrian.vondendriesch@credativ.de>
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 = 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, Int64GetDatum(2));
- result = DirectFunctionCall2(numeric_add, d, one);
+
+ result = DirectFunctionCall2(numeric_ge, d, zero);
+ greater_or_equal_zero = DatumGetBool(result);
+
+ if (greater_or_equal_zero)
+ result = DirectFunctionCall2(numeric_add, d, one);
+ else
+ result = DirectFunctionCall2(numeric_sub, d, one);
+
result = DirectFunctionCall2(numeric_div_trunc, result, two);
return DatumGetNumeric(result);
}
@@ -632,7 +653,7 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
limit = int64_to_numeric(10 * 1024);
limit2 = int64_to_numeric(10 * 1024 * 2 - 1);
- if (numeric_is_less(size, limit))
+ if (numeric_is_less(numeric_absolute(size), limit))
{
result = psprintf("%s bytes", numeric_to_cstring(size));
}
@@ -642,20 +663,18 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
/* size >>= 9 */
size = numeric_shift_right(size, 9);
- if (numeric_is_less(size, limit2))
+ if (numeric_is_less(numeric_absolute(size), limit2))
{
- /* size = (size + 1) / 2 */
- size = numeric_plus_one_over_two(size);
+ size = numeric_divide_by_two(size);
result = psprintf("%s kB", numeric_to_cstring(size));
}
else
{
/* size >>= 10 */
size = numeric_shift_right(size, 10);
- if (numeric_is_less(size, limit2))
+ if (numeric_is_less(numeric_absolute(size), limit2))
{
- /* size = (size + 1) / 2 */
- size = numeric_plus_one_over_two(size);
+ size = numeric_divide_by_two(size);
result = psprintf("%s MB", numeric_to_cstring(size));
}
else
@@ -663,18 +682,16 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
/* size >>= 10 */
size = numeric_shift_right(size, 10);
- if (numeric_is_less(size, limit2))
+ if (numeric_is_less(numeric_absolute(size), limit2))
{
- /* size = (size + 1) / 2 */
- size = numeric_plus_one_over_two(size);
+ size = numeric_divide_by_two(size);
result = psprintf("%s GB", numeric_to_cstring(size));
}
else
{
/* size >>= 10 */
size = numeric_shift_right(size, 10);
- /* size = (size + 1) / 2 */
- size = numeric_plus_one_over_two(size);
+ size = numeric_divide_by_two(size);
result = psprintf("%s TB", numeric_to_cstring(size));
}
}
diff --git a/src/include/c.h b/src/include/c.h
index 8163b00..1fb16f1 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -793,6 +793,12 @@ typedef NameData *Name;
#define Abs(x) ((x) >= 0 ? (x) : -(x))
/*
+ * Sign
+ * Return one if a number is positive 0 otherwise.
+ */
+#define Sign(x) ((x) >= 0 ? 1 : 0)
+
+/*
* StrNCpy
* Like standard library function strncpy(), except that result string
* is guaranteed to be null-terminated --- that is, at most N-1 bytes
diff --git a/src/test/regress/expected/pg_size_pretty_bigint.out b/src/test/regress/expected/pg_size_pretty_bigint.out
new file mode 100644
index 0000000..f669e35
--- /dev/null
+++ b/src/test/regress/expected/pg_size_pretty_bigint.out
@@ -0,0 +1,39 @@
+--
+-- pg_size_pretty_bigint
+--
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 10 bytes | -10 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 1000 bytes | -1000 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 977 kB | -977 kB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 954 MB | -954 MB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 931 GB | -931 GB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 909 TB | -909 TB
+(1 row)
+
diff --git a/src/test/regress/expected/pg_size_pretty_numeric.out b/src/test/regress/expected/pg_size_pretty_numeric.out
new file mode 100644
index 0000000..ce34952
--- /dev/null
+++ b/src/test/regress/expected/pg_size_pretty_numeric.out
@@ -0,0 +1,75 @@
+--
+-- pg_size_pretty_numeric
+--
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 10 bytes | -10 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 1000 bytes | -1000 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 977 kB | -977 kB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 954 MB | -954 MB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 931 GB | -931 GB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 909 TB | -909 TB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 10.5 bytes | -10.5 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 1000.5 bytes | -1000.5 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 977 kB | -977 kB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 954 MB | -954 MB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 931 GB | -931 GB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 909 TB | -909 TB
+(1 row)
+
diff --git a/src/test/regress/sql/pg_size_pretty_bigint.sql b/src/test/regress/sql/pg_size_pretty_bigint.sql
new file mode 100644
index 0000000..a232203
--- /dev/null
+++ b/src/test/regress/sql/pg_size_pretty_bigint.sql
@@ -0,0 +1,15 @@
+--
+-- pg_size_pretty_bigint
+--
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000::bigint) pg_size_pretty(size);
diff --git a/src/test/regress/sql/pg_size_pretty_numeric.sql b/src/test/regress/sql/pg_size_pretty_numeric.sql
new file mode 100644
index 0000000..c1b626e
--- /dev/null
+++ b/src/test/regress/sql/pg_size_pretty_numeric.sql
@@ -0,0 +1,29 @@
+
+--
+-- pg_size_pretty_numeric
+--
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000::numeric) pg_size_pretty(size);
+
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000.5::numeric) pg_size_pretty(size);
--
2.5.1
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" <david.g.johnston@gmail.com> 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 100000000000000::bigint) foo(size); pg_size_pretty |
pg_size_pretty ----------------+------------------------ 91 TB
| -100000000000000 bytes (1 row)SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(SELECT 100000000000000::numeric) foo(size); pg_size_pretty |
pg_size_pretty ----------------+------------------------ 91 TB
| -100000000000000 bytes (1 row)After the patch:
SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(SELECT 100000000000000::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 100000000000000::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
On Sat, Oct 31, 2015 at 2:25 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> 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
On 03/11/2015 04:06, Robert Haas wrote:
On Sat, Oct 31, 2015 at 2:25 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> 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
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
<julien.rouhaud@dalibo.com> 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
Attachments:
0001-Make-pg_size_pretty-handle-negative-values-v2.patchtext/x-diff; name=0001-Make-pg_size_pretty-handle-negative-values-v2.patchDownload
From c06d1463cf21957260327c47217050e334058422 Mon Sep 17 00:00:00 2001
From: Adrian Vondendriesch <adrian.vondendriesch@credativ.de>
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, Int64GetDatum(2));
- result = DirectFunctionCall2(numeric_add, d, one);
+
+ result = DirectFunctionCall2(numeric_ge, d, zero);
+ greater_or_equal_zero = DatumGetBool(result);
+
+ if (greater_or_equal_zero)
+ result = DirectFunctionCall2(numeric_add, d, one);
+ else
+ result = DirectFunctionCall2(numeric_sub, d, one);
+
result = DirectFunctionCall2(numeric_div_trunc, result, two);
return DatumGetNumeric(result);
}
@@ -632,7 +659,7 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
limit = int64_to_numeric(10 * 1024);
limit2 = int64_to_numeric(10 * 1024 * 2 - 1);
- if (numeric_is_less(size, limit))
+ if (numeric_is_less(numeric_absolute(size), limit))
{
result = psprintf("%s bytes", numeric_to_cstring(size));
}
@@ -642,20 +669,18 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
/* size >>= 9 */
size = numeric_shift_right(size, 9);
- if (numeric_is_less(size, limit2))
+ if (numeric_is_less(numeric_absolute(size), limit2))
{
- /* size = (size + 1) / 2 */
- size = numeric_plus_one_over_two(size);
+ size = numeric_half_rounded(size);
result = psprintf("%s kB", numeric_to_cstring(size));
}
else
{
/* size >>= 10 */
size = numeric_shift_right(size, 10);
- if (numeric_is_less(size, limit2))
+ if (numeric_is_less(numeric_absolute(size), limit2))
{
- /* size = (size + 1) / 2 */
- size = numeric_plus_one_over_two(size);
+ size = numeric_half_rounded(size);
result = psprintf("%s MB", numeric_to_cstring(size));
}
else
@@ -663,18 +688,16 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
/* size >>= 10 */
size = numeric_shift_right(size, 10);
- if (numeric_is_less(size, limit2))
+ if (numeric_is_less(numeric_absolute(size), limit2))
{
- /* size = (size + 1) / 2 */
- size = numeric_plus_one_over_two(size);
+ size = numeric_half_rounded(size);
result = psprintf("%s GB", numeric_to_cstring(size));
}
else
{
/* size >>= 10 */
size = numeric_shift_right(size, 10);
- /* size = (size + 1) / 2 */
- size = numeric_plus_one_over_two(size);
+ size = numeric_half_rounded(size);
result = psprintf("%s TB", numeric_to_cstring(size));
}
}
diff --git a/src/test/regress/expected/pg_size_pretty_bigint.out b/src/test/regress/expected/pg_size_pretty_bigint.out
new file mode 100644
index 0000000..f669e35
--- /dev/null
+++ b/src/test/regress/expected/pg_size_pretty_bigint.out
@@ -0,0 +1,39 @@
+--
+-- pg_size_pretty_bigint
+--
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 10 bytes | -10 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 1000 bytes | -1000 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 977 kB | -977 kB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 954 MB | -954 MB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 931 GB | -931 GB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000::bigint) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 909 TB | -909 TB
+(1 row)
+
diff --git a/src/test/regress/expected/pg_size_pretty_numeric.out b/src/test/regress/expected/pg_size_pretty_numeric.out
new file mode 100644
index 0000000..ce34952
--- /dev/null
+++ b/src/test/regress/expected/pg_size_pretty_numeric.out
@@ -0,0 +1,75 @@
+--
+-- pg_size_pretty_numeric
+--
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 10 bytes | -10 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 1000 bytes | -1000 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 977 kB | -977 kB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 954 MB | -954 MB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 931 GB | -931 GB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 909 TB | -909 TB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 10.5 bytes | -10.5 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 1000.5 bytes | -1000.5 bytes
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 977 kB | -977 kB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 954 MB | -954 MB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 931 GB | -931 GB
+(1 row)
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000.5::numeric) pg_size_pretty(size);
+ pg_size_pretty | pg_size_pretty
+----------------+----------------
+ 909 TB | -909 TB
+(1 row)
+
diff --git a/src/test/regress/sql/pg_size_pretty_bigint.sql b/src/test/regress/sql/pg_size_pretty_bigint.sql
new file mode 100644
index 0000000..a232203
--- /dev/null
+++ b/src/test/regress/sql/pg_size_pretty_bigint.sql
@@ -0,0 +1,15 @@
+--
+-- pg_size_pretty_bigint
+--
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000::bigint) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000::bigint) pg_size_pretty(size);
diff --git a/src/test/regress/sql/pg_size_pretty_numeric.sql b/src/test/regress/sql/pg_size_pretty_numeric.sql
new file mode 100644
index 0000000..c1b626e
--- /dev/null
+++ b/src/test/regress/sql/pg_size_pretty_numeric.sql
@@ -0,0 +1,29 @@
+
+--
+-- pg_size_pretty_numeric
+--
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000::numeric) pg_size_pretty(size);
+
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 10.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000.5::numeric) pg_size_pretty(size);
+
+SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 1000000000000000.5::numeric) pg_size_pretty(size);
--
2.6.2
On Thu, Nov 5, 2015 at 4:19 PM, Adrian.Vondendriesch
<Adrian.Vondendriesch@credativ.de> 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-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Am 06.11.2015 um 17:06 schrieb Robert Haas:
On Thu, Nov 5, 2015 at 4:19 PM, Adrian.Vondendriesch
<Adrian.Vondendriesch@credativ.de> 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
On Fri, Nov 6, 2015 at 12:44 PM, Adrian Vondendriesch
<adrian.vondendriesch@credativ.de> wrote:
Am 06.11.2015 um 17:06 schrieb Robert Haas:
On Thu, Nov 5, 2015 at 4:19 PM, Adrian.Vondendriesch
<Adrian.Vondendriesch@credativ.de> 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