On Sat, Apr 12, 2008 at 06:02:39PM -0400, Tom Lane wrote: > Gavin Sherry <[EMAIL PROTECTED]> 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
Index: src/backend/utils/adt/varlena.c =================================================================== RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/utils/adt/varlena.c,v retrieving revision 1.164 diff -c -p -c -r1.164 varlena.c *** src/backend/utils/adt/varlena.c 25 Mar 2008 22:42:44 -0000 1.164 --- src/backend/utils/adt/varlena.c 12 Apr 2008 21:10:01 -0000 *************** text_substring(Datum str, int32 start, i *** 754,760 **** * If we're working with an untoasted source, no need to do an extra * copying step. */ ! if (VARATT_IS_COMPRESSED(str) || VARATT_IS_EXTERNAL(str)) slice = DatumGetTextPSlice(str, slice_start, slice_size); else slice = (text *) DatumGetPointer(str); --- 754,761 ---- * If we're working with an untoasted source, no need to do an extra * copying step. */ ! if (VARATT_IS_COMPRESSED(DatumGetPointer(str)) || ! VARATT_IS_EXTERNAL(DatumGetPointer(str))) slice = DatumGetTextPSlice(str, slice_start, slice_size); else slice = (text *) DatumGetPointer(str); Index: src/backend/utils/mb/mbutils.c =================================================================== RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/utils/mb/mbutils.c,v retrieving revision 1.69 diff -c -p -c -r1.69 mbutils.c *** src/backend/utils/mb/mbutils.c 9 Jan 2008 23:43:54 -0000 1.69 --- src/backend/utils/mb/mbutils.c 12 Apr 2008 22:55:49 -0000 *************** pg_convert_to(PG_FUNCTION_ARGS) *** 313,319 **** result = DirectFunctionCall3(pg_convert, string, src_encoding_name, dest_encoding_name); ! PG_RETURN_BYTEA_P(result); } /* --- 313,319 ---- result = DirectFunctionCall3(pg_convert, string, src_encoding_name, dest_encoding_name); ! PG_RETURN_DATUM(result); } /* *************** pg_convert_from(PG_FUNCTION_ARGS) *** 340,346 **** * in this case it will be because we've told pg_convert to return one * that is valid as text in the current database encoding. */ ! PG_RETURN_TEXT_P(result); } /* --- 340,346 ---- * in this case it will be because we've told pg_convert to return one * that is valid as text in the current database encoding. */ ! PG_RETURN_DATUM(result); } /*
-- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
