Thread: 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
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>: > 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. >
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: > 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 bytes >>> >>> >> Do 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
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
Attachment
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
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
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
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
Attachment
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
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