Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values

[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.

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

[Adrian Vondendriesch]

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

[Robert Haas]

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

[Adrian.Vondendriesch]

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, 

Re: [HACKERS] [BUGS] BUG #12989: pg_size_pretty with negative values

[Julien Rouhaud]

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

[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)

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

[Julien Rouhaud]

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

[Adrian.Vondendriesch]

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);
+}
+