Thread: pgsql: Fix pg_size_bytes() to be more portable.
Fix pg_size_bytes() to be more portable. Commit 53874c5228fe16589a4d01b3e1fab3678e0fd8e3 broke various 32-bit buildfarm machines because it incorrectly used an 'L' suffix for what needed to be a 64-bit literal. Thanks to Michael Paquier for helping to diagnose this. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/ad7cc1c554980145b226a066afe56d9c777ce7ae Modified Files -------------- src/backend/utils/adt/dbsize.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Fix pg_size_bytes() to be more portable.
> Commit 53874c5228fe16589a4d01b3e1fab3678e0fd8e3 broke various 32-bit
> buildfarm machines because it incorrectly used an 'L' suffix for what
> needed to be a 64-bit literal. Thanks to Michael Paquier for helping
> to diagnose this.
That's still not right: not all compilers support "long long", and the
ones that don't won't have "LL" notation either.
Project style is to use something like "(uint64) 1024" instead.
regards, tom lane
On 20 February 2016 at 14:54, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dean Rasheed <dean.a.rasheed@gmail.com> writes: >> Fix pg_size_bytes() to be more portable. >> Commit 53874c5228fe16589a4d01b3e1fab3678e0fd8e3 broke various 32-bit >> buildfarm machines because it incorrectly used an 'L' suffix for what >> needed to be a 64-bit literal. Thanks to Michael Paquier for helping >> to diagnose this. > > That's still not right: not all compilers support "long long", and the > ones that don't won't have "LL" notation either. > > Project style is to use something like "(uint64) 1024" instead. > OK, will fix. BTW, I found a couple of instances of 'LL' in ecpglib/prepare.c, which is why I thought it was OK to use it. Regards, Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 20 February 2016 at 14:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Project style is to use something like "(uint64) 1024" instead.
> BTW, I found a couple of instances of 'LL' in ecpglib/prepare.c, which
> is why I thought it was OK to use it.
[ squint... ] Don't know how long that code has been there, but it's
gratuitously unportable. Even if you grant that "long long" exists
in the compiler, there's no promises whatever as to what width it is.
This code looks to be assuming that it's int64, but just using int64
would be a better answer.
BTW, I now remember that we have an INT64CONST macro in c.h, which
would be another solution if you don't like the cast for some reason.
regards, tom lane
After further thought about the portability implications of this ---
1. We probably gave up support for long-long-less compilers when we agreed
to start requiring a working int64 type. On a 32-bit machine that's
highly likely to be "long long", and 64-bit machines are mostly new enough
that they'd have C99-compliant compilers.
2. Nonetheless, LL is not the same as int64; on modern 64-bit machines it
probably means int128. So the right thing to do when writing a constant
you mean to be int64 is to use a cast or [U]INT64CONST(). (You need that
macro if the constant value might be too wide for plain int, since pickier
compilers may reject an unsuffixed constant wider than int.)
Your updated code looks fine from here. I looked into changing that code
in ecpg, but it would be more invasive than I thought because ecpg doesn't
use c.h. Some rearrangement of the ecpg headers would be required, and
in view of point #1, it's unlikely to be worth it; it might buy a bit of
micro-efficiency but not much.
regards, tom lane
On 20 February 2016 at 16:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> After further thought about the portability implications of this ---
>
> 1. We probably gave up support for long-long-less compilers when we agreed
> to start requiring a working int64 type. On a 32-bit machine that's
> highly likely to be "long long", and 64-bit machines are mostly new enough
> that they'd have C99-compliant compilers.
>
> 2. Nonetheless, LL is not the same as int64; on modern 64-bit machines it
> probably means int128. So the right thing to do when writing a constant
> you mean to be int64 is to use a cast or [U]INT64CONST(). (You need that
> macro if the constant value might be too wide for plain int, since pickier
> compilers may reject an unsuffixed constant wider than int.)
>
OK, thanks that's all good to know.
> Your updated code looks fine from here. I looked into changing that code
> in ecpg, but it would be more invasive than I thought because ecpg doesn't
> use c.h. Some rearrangement of the ecpg headers would be required, and
> in view of point #1, it's unlikely to be worth it; it might buy a bit of
> micro-efficiency but not much.
>
It looks to me as though it doesn't need long long anyway, since the
rotation it's doing can be done just as easily with ints, for example:
int hashVal = 0;
for (...)
{
hashVal = hashVal + (int) ecpgQuery[stmtIx];
hashVal = (((unsigned int) hashVal) >> 19) | (hashVal << 13);
}
but it's probably not worth changing, for risk of breaking it.
Regards,
Dean
On 20 February 2016 at 17:24, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > It looks to me as though it doesn't need long long anyway, since the > rotation it's doing can be done just as easily with ints, for example: Oh, scratch that -- I was assuming int would be 32-bit, which it might not be. Regards, Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 20 February 2016 at 17:24, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> It looks to me as though it doesn't need long long anyway, since the
>> rotation it's doing can be done just as easily with ints, for example:
> Oh, scratch that -- I was assuming int would be 32-bit, which it might not be.
There's quite a lot of our code that does assume "int" is 32 bits.
(It would be better to write int32 where it matters, but I really doubt
we've been completely consistent about that.) It probably isn't worth
worrying about; I think the platforms where "int" means int16 are all dead
and buried, or at least far too underpowered to run modern Postgres.
At some point we might have to contend with "int" meaning int64, but
I haven't heard of any such platforms yet. The real issue is with
"long" and "long long", which definitely do vary in width across supported
platforms.
regards, tom lane