Re: pgsql: Fix pg_size_pretty() to avoid overflow for inputs close to INT64

Fix pg_size_pretty() to avoid overflow for inputs close to INT64_MAX.

The expression that tried to round the value to the nearest TB could
overflow, leading to bogus output as reported in bug #5993 from Nicola
Cossu. This isn't likely to ever happen in the intended usage of the
function (if it could, we'd be needing to use a wider datatype instead);
but it's not hard to give the expected output, so let's do so.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/af0f20092c8662bf7610fab07b8a1e354abba67f

Modified Files
--------------
src/backend/utils/adt/dbsize.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)

Excerpts from Tom Lane's message of lun abr 25 17:22:41 -0300 2011:
> Fix pg_size_pretty() to avoid overflow for inputs close to INT64_MAX.
>
> The expression that tried to round the value to the nearest TB could
> overflow, leading to bogus output as reported in bug #5993 from Nicola
> Cossu. This isn't likely to ever happen in the intended usage of the
> function (if it could, we'd be needing to use a wider datatype instead);
> but it's not hard to give the expected output, so let's do so.

Apparently this change is causing Moa's SunStudio compiler to fail an
assertion.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Tom Lane's message of lun abr 25 17:22:41 -0300 2011:
>> Fix pg_size_pretty() to avoid overflow for inputs close to INT64_MAX.

> Apparently this change is causing Moa's SunStudio compiler to fail an
> assertion.

[ scratches head... ] Hard to see why, there's nothing at all
interesting in that code.

regards, tom lane

Excerpts from Tom Lane's message of mié abr 27 17:10:37 -0300 2011:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Excerpts from Tom Lane's message of lun abr 25 17:22:41 -0300 2011:
> >> Fix pg_size_pretty() to avoid overflow for inputs close to INT64_MAX.
>
> > Apparently this change is causing Moa's SunStudio compiler to fail an
> > assertion.
>
> [ scratches head... ] Hard to see why, there's nothing at all
> interesting in that code.

I agree, but it fails exactly in that code, and started to fail
immediately after that patch.

Maybe casting the 2 to int64 would fix it?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Tom Lane's message of mi abr 27 17:10:37 -0300 2011:
>> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>>> Apparently this change is causing Moa's SunStudio compiler to fail an
>>> assertion.

>> [ scratches head... ] Hard to see why, there's nothing at all
>> interesting in that code.

> I agree, but it fails exactly in that code, and started to fail
> immediately after that patch.

> Maybe casting the 2 to int64 would fix it?

I'm not excited about trying random code changes to dodge a compiler bug
with a 24-hour turnaround time. Dave, can you poke at this?

regards, tom lane

On Thu, Apr 28, 2011 at 6:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Excerpts from Tom Lane's message of mié abr 27 17:10:37 -0300 2011:
>>> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>>>> Apparently this change is causing Moa's SunStudio compiler to fail an
>>>> assertion.
>
>>> [ scratches head... ]  Hard to see why, there's nothing at all
>>> interesting in that code.
>
>> I agree, but it fails exactly in that code, and started to fail
>> immediately after that patch.
>
>> Maybe casting the 2 to int64 would fix it?
>
> I'm not excited about trying random code changes to dodge a compiler bug
> with a 24-hour turnaround time.  Dave, can you poke at this?

I think we may have to award Sun (or whats left of them) the "Bizarre
compiler bug of the week" award here. It's actually the val++; that's
causing the assertion, but I'm darned if I can get it to work. I've
tried spelling out the addition, casting, changing val to an int64*,
renaming val, and probably a dozen or so things that are broken, all
with no success.

Any other ideas?

--
Dave Page
PostgreSQL Core Team
http://www.postgresql.org/

Dave Page <dpage(at)postgresql(dot)org> writes:
> On Thu, Apr 28, 2011 at 6:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Dave, can you poke at this?

> I think we may have to award Sun (or whats left of them) the "Bizarre
> compiler bug of the week" award here.

Yeah. Has anyone filed a report with them?

> It's actually the val++; that's
> causing the assertion, but I'm darned if I can get it to work. I've
> tried spelling out the addition, casting, changing val to an int64*,
> renaming val, and probably a dozen or so things that are broken, all
> with no success.
> Any other ideas?

I was suspicious that it had something to do with the compiler trying to
optimize the size / mult and size % mult subexpressions to get both
results with one division instruction. Can you check if removing either
of those makes the crash go away?

If it does have something to do with that, my inclination is to fix it
by rewriting the function to depend on shifting rather than division.
I was considering doing that anyway but decided it was pointless
micro-optimization. However, if it dodges a compiler bug ...

regards, tom lane

On Thu, Apr 28, 2011 at 8:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dave Page <dpage(at)postgresql(dot)org> writes:
>> On Thu, Apr 28, 2011 at 6:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Dave, can you poke at this?
>
>> I think we may have to award Sun (or whats left of them) the "Bizarre
>> compiler bug of the week" award here.
>
> Yeah.  Has anyone filed a report with them?

I haven't. Oracle dont seem to make it easy to do such things (at
least for those of us without support contracts).

>> It's actually the val++; that's
>> causing the assertion, but I'm darned if I can get it to work. I've
>> tried spelling out the addition, casting, changing val to an int64*,
>> renaming val, and probably a dozen or so things that are broken, all
>> with no success.
>> Any other ideas?
>
> I was suspicious that it had something to do with the compiler trying to
> optimize the size / mult and size % mult subexpressions to get both
> results with one division instruction.  Can you check if removing either
> of those makes the crash go away?

Already did (that was my first assumption). Removing them doesn't
help, nor does rewriting them in various strange ways. Removing val++;
(and replacing with { } ) allows compilation to succeed.

I guess the best bet is a compiler update, if I can persuade the
oracle website to let me download it...

--
Dave Page
PostgreSQL Core Team
http://www.postgresql.org/

Excerpts from Dave Page's message of jue abr 28 16:33:44 -0300 2011:

> I think we may have to award Sun (or whats left of them) the "Bizarre
> compiler bug of the week" award here. It's actually the val++; that's
> causing the assertion, but I'm darned if I can get it to work. I've
> tried spelling out the addition, casting, changing val to an int64*,
> renaming val, and probably a dozen or so things that are broken, all
> with no success.

Err, val = val + 1?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Dave Page <dpage(at)postgresql(dot)org> writes:
> On Thu, Apr 28, 2011 at 8:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I was suspicious that it had something to do with the compiler trying to
>> optimize the size / mult and size % mult subexpressions

> Already did (that was my first assumption). Removing them doesn't
> help, nor does rewriting them in various strange ways. Removing val++;
> (and replacing with { } ) allows compilation to succeed.

Huh. Well, it might still be the case that switching to a shift-based
implementation would work around it, since we could avoid having any ++
operation in that. Let me give it a shot.

regards, tom lane

On Thu, Apr 28, 2011 at 9:05 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Dave Page's message of jue abr 28 16:33:44 -0300 2011:
>
>> I think we may have to award Sun (or whats left of them) the "Bizarre
>> compiler bug of the week" award here. It's actually the val++; that's
>> causing the assertion, but I'm darned if I can get it to work. I've
>> tried spelling out the addition, casting, changing val to an int64*,
>> renaming val, and probably a dozen or so things that are broken, all
>> with no success.
>
> Err, val = val + 1?

That's what I meant by "spelling out the addition".

--
Dave Page
PostgreSQL Core Team
http://www.postgresql.org/

Please see if the attached version works.

regards, tom lane

Datum
pg_size_pretty(PG_FUNCTION_ARGS)
{
int64 size = PG_GETARG_INT64(0);
char buf[64];
int64 limit = 10 * 1024;
int64 limit2 = limit * 2 - 1;

if (size < limit)
snprintf(buf, sizeof(buf), INT64_FORMAT " bytes", size);
else
{
size >>= 9; /* keep one extra bit for rounding */
if (size < limit2)
snprintf(buf, sizeof(buf), INT64_FORMAT " kB",
(size + 1) / 2);
else
{
size >>= 10;
if (size < limit2)
snprintf(buf, sizeof(buf), INT64_FORMAT " MB",
(size + 1) / 2);
else
{
size >>= 10;
if (size < limit2)
snprintf(buf, sizeof(buf), INT64_FORMAT " GB",
(size + 1) / 2);
else
{
size >>= 10;
snprintf(buf, sizeof(buf), INT64_FORMAT " TB",
(size + 1) / 2);
}
}
}
}

PG_RETURN_TEXT_P(cstring_to_text(buf));
}

Looks like you committed this before I got to it - moa is green, so I
guess it works.

Thanks.

On Thursday, April 28, 2011, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Please see if the attached version works.
>
>                        regards, tom lane
>
>
> Datum
> pg_size_pretty(PG_FUNCTION_ARGS)
> {
>        int64           size = PG_GETARG_INT64(0);
>        char            buf[64];
>        int64           limit = 10 * 1024;
>        int64           limit2 = limit * 2 - 1;
>
>        if (size < limit)
>                snprintf(buf, sizeof(buf), INT64_FORMAT " bytes", size);
>        else
>        {
>                size >>= 9;                             /* keep one extra bit for rounding */
>                if (size < limit2)
>                        snprintf(buf, sizeof(buf), INT64_FORMAT " kB",
>                                         (size + 1) / 2);
>                else
>                {
>                        size >>= 10;
>                        if (size < limit2)
>                                snprintf(buf, sizeof(buf), INT64_FORMAT " MB",
>                                                 (size + 1) / 2);
>                        else
>                        {
>                                size >>= 10;
>                                if (size < limit2)
>                                        snprintf(buf, sizeof(buf), INT64_FORMAT " GB",
>                                                         (size + 1) / 2);
>                                else
>                                {
>                                        size >>= 10;
>                                        snprintf(buf, sizeof(buf), INT64_FORMAT " TB",
>                                                         (size + 1) / 2);
>                                }
>                        }
>                }
>        }
>
>        PG_RETURN_TEXT_P(cstring_to_text(buf));
> }
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company