datum passed to macro which expects a pointer

Gavin Sherry <swm@alcove.com.au> writes:

This may seem a little pedantic but I noticed a few places where we pass
a datum to a macro which treats the datum as a pointer. This works now
but might not in the future (if, say, Datum were to be 8 bytes).

Yeah, definitely something to fix. I think though that the cases
like this:

! PG_RETURN_TEXT_P(DatumGetPointer(result));

might as well just use PG_RETURN_DATUM instead of casting twice.

Was this just eyeball inspection or did you find a compiler that would
complain about this?

regards, tom lane

On Sat, Apr 12, 2008 at 06:02:39PM -0400, Tom Lane wrote:

Gavin Sherry <swm@alcove.com.au> writes:

This may seem a little pedantic but I noticed a few places where we pass
a datum to a macro which treats the datum as a pointer. This works now
but might not in the future (if, say, Datum were to be 8 bytes).

Yeah, definitely something to fix. I think though that the cases
like this:

! PG_RETURN_TEXT_P(DatumGetPointer(result));

might as well just use PG_RETURN_DATUM instead of casting twice.

Oh of course. Updated patch attached.

Was this just eyeball inspection or did you find a compiler that would
complain about this?

I wish. It was actually thrown up when we (Greenplum) changed the macros
to be inline functions as part of changing Datum to be 8 bytes. By using
inline functions we get proper type checking from the compiler and since
we have only a small number of target platforms and architectures,
inlining isn't an issue.

Thanks,

Gavin

Gavin Sherry <swm@alcove.com.au> writes:

I wish. It was actually thrown up when we (Greenplum) changed the macros
to be inline functions as part of changing Datum to be 8 bytes.

Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines.
What is it you're trying to accomplish by making it wider on 32-bitters?

regards, tom lane

On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote:

Gavin Sherry <swm@alcove.com.au> writes:

I wish. It was actually thrown up when we (Greenplum) changed the macros
to be inline functions as part of changing Datum to be 8 bytes.

Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines.
What is it you're trying to accomplish by making it wider on 32-bitters?

I miss stated there. This was actually about making key 64 bit types
pass by value instead of pass by reference.

Thanks,

Gavin

Gavin Sherry <swm@alcove.com.au> writes:

might as well just use PG_RETURN_DATUM instead of casting twice.

Oh of course. Updated patch attached.

Applied, thanks.

regards, tom lane

"Gavin Sherry" <swm@alcove.com.au> writes:

On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote:

Gavin Sherry <swm@alcove.com.au> writes:

I wish. It was actually thrown up when we (Greenplum) changed the macros
to be inline functions as part of changing Datum to be 8 bytes.

Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines.
What is it you're trying to accomplish by making it wider on 32-bitters?

I miss stated there. This was actually about making key 64 bit types
pass by value instead of pass by reference.

There was a patch to do this posted recently here as well.

http://archives.postgresql.org/pgsql-patches/2008-03/msg00335.php

Hm. I suppose it's true that you could make Datum 64-bit even on 32-bit
machines and make int8 and float8 pass-by-value. Seems unlikely to be a net
win though.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning

On Sun, Apr 13, 2008 at 01:42:02AM +0100, Gregory Stark wrote:

"Gavin Sherry" <swm@alcove.com.au> writes:

On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote:

Gavin Sherry <swm@alcove.com.au> writes:

I wish. It was actually thrown up when we (Greenplum) changed the macros
to be inline functions as part of changing Datum to be 8 bytes.

Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines.
What is it you're trying to accomplish by making it wider on 32-bitters?

I miss stated there. This was actually about making key 64 bit types
pass by value instead of pass by reference.

There was a patch to do this posted recently here as well.

http://archives.postgresql.org/pgsql-patches/2008-03/msg00335.php

Hm. I suppose it's true that you could make Datum 64-bit even on 32-bit
machines and make int8 and float8 pass-by-value. Seems unlikely to be a net
win though.

A very quick scan showed me that one bet is missed in this patch which
we learned about the hard way: write_auth_file() assumes timestamptz is
pass by reference.

I'm also not sure if endianness is completely covered in the patch but
it looks fairly accurate. I think PointerGetDatum() may need the union
trick (it's late where I am).

There were other places in the code which were assuming Datums were
equivalent to pointers. I'll dig them up.

Also, it means we can clean up parts of numeric.c which special case
calls from aggregates.

Seems like a pretty clean patch though.

Thanks,

Gavin

Hi all,

Attached are more fixes.

Thanks,

Gavin, with Feng Tian

Gavin Sherry wrote:

Hi all,

Attached are more fixes.

Applied, thanks.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support