Thread: datum passed to macro which expects a pointer
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). Thanks, Gavin
Attachment
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
Attachment
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
Attachment
Gavin Sherry wrote: > Hi all, > > Attached are more fixes. Applied, thanks. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support