Re: [PATCHES] datum passed to macro which expects a pointer
Gavin Sherry wrote: > Hi all, > > Attached are more fixes. Applied, thanks. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] datum passed to macro which expects a pointer
Hi all,
Attached are more fixes.
Thanks,
Gavin, with Feng Tian
Index: src/backend/access/common/heaptuple.c
===
RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/access/common/heaptuple.c,v
retrieving revision 1.120
diff -c -p -r1.120 heaptuple.c
*** src/backend/access/common/heaptuple.c 1 Jan 2008 19:45:45 - 1.120
--- src/backend/access/common/heaptuple.c 13 Apr 2008 13:06:53 -
*** heap_form_tuple(TupleDesc tupleDescripto
*** 890,896
else if (att[i]->attlen == -1 &&
att[i]->attalign == 'd' &&
att[i]->attndims == 0 &&
! !VARATT_IS_EXTENDED(values[i]))
{
values[i] = toast_flatten_tuple_attribute(values[i],
att[i]->atttypid,
--- 890,896
else if (att[i]->attlen == -1 &&
att[i]->attalign == 'd' &&
att[i]->attndims == 0 &&
! !VARATT_IS_EXTENDED(DatumGetPointer(values[i])))
{
values[i] = toast_flatten_tuple_attribute(values[i],
att[i]->atttypid,
*** heap_formtuple(TupleDesc tupleDescriptor
*** 1001,1007
else if (att[i]->attlen == -1 &&
att[i]->attalign == 'd' &&
att[i]->attndims == 0 &&
! !VARATT_IS_EXTENDED(values[i]))
{
values[i] = toast_flatten_tuple_attribute(values[i],
att[i]->atttypid,
--- 1001,1007
else if (att[i]->attlen == -1 &&
att[i]->attalign == 'd' &&
att[i]->attndims == 0 &&
! !VARATT_IS_EXTENDED(DatumGetPointer(values[i])))
{
values[i] = toast_flatten_tuple_attribute(values[i],
att[i]->atttypid,
Index: src/backend/access/common/indextuple.c
===
RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/access/common/indextuple.c,v
retrieving revision 1.85
diff -c -p -r1.85 indextuple.c
*** src/backend/access/common/indextuple.c 1 Jan 2008 19:45:45 - 1.85
--- src/backend/access/common/indextuple.c 13 Apr 2008 18:16:44 -
*** index_form_tuple(TupleDesc tupleDescript
*** 73,79
* If value is stored EXTERNAL, must fetch it so we are not depending
* on outside storage. This should be improved someday.
*/
! if (VARATT_IS_EXTERNAL(values[i]))
{
untoasted_values[i] =
PointerGetDatum(heap_tuple_fetch_attr((struct varlena *)
--- 73,79
* If value is stored EXTERNAL, must fetch it so we are not depending
* on outside storage. This should be improved someday.
*/
! if (VARATT_IS_EXTERNAL(DatumGetPointer(values[i])))
{
untoasted_values[i] =
PointerGetDatum(heap_tuple_fetch_attr((struct varlena *)
*** index_form_tuple(TupleDesc tupleDescript
*** 85,92
* If value is above size target, and is of a compressible datatype,
* try to compress it in-line.
*/
! if (!VARATT_IS_EXTENDED(untoasted_values[i]) &&
! VARSIZE(untoasted_values[i]) > TOAST_INDEX_TARGET &&
(att->attstorage == 'x' || att->attstorage == 'm'))
{
Datum cvalue = toast_compress_datum(untoasted_values[i]);
--- 85,92
* If value is above size target, and is of a compressible datatype,
* try to compress it in-line.
*/
! if (!VARATT_IS_EXTENDED(DatumGetPointer(untoasted_values[i])) &&
! VARSIZE(DatumGetPointer(untoasted_values[i])) > TOAST_INDEX_TARGET &&
(att->attstorage == 'x' || att->attstorage == 'm'))
{
Datum cvalue = toast_compress_datum(untoasted_values[i]);
Index: src/backend/access/common/printtup.c
===
RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/access/common/printtup.c,v
retrieving revision 1.101
diff -c -p -r1.101 printtup.c
*** src/backend/access/common/printtup.c 1 Jan 2008 19:45:45 - 1.101
--- src/backend/access/common/printtup.c 13 Apr 2008 13:14:52 -
*** printtup(TupleTableSlot *slot, DestRecei
*** 340,346
}
/* Clean up detoasted copy, if any */
! if (attr != origattr)
pfree(DatumGetPointer(attr));
}
--- 340,346
}
/* Clean up detoasted copy, if any */
! if (DatumGetPointer(attr) != DatumGetPointer(origattr))
pfree(DatumGetPointer(attr));
}
*** printtup_20(TupleTableSlot *slot, DestRe
*** 423,429
pfree(outputstr);
/* Clean up detoasted copy, if any */
! if (attr != origattr)
pfree(DatumGetPointer(attr));
}
--- 423,429
pfree(outputstr);
/* Clean up detoasted copy, if any */
! if (DatumGetPointer(attr) != DatumGetPointer(origattr))
pfree(DatumGetPointer(attr));
}
*** debugtup(TupleTableSlot *slot, DestRecei
*** 537,543
pfree(value);
/* Clean up detoasted copy, if any */
! if (attr != origattr)
pfree(DatumGetPointer(attr));
}
printf("\t\n");
--- 537,543
pfree(value);
/* Clean up detoasted copy, if any */
! if (DatumGetPointer(attr)
Re: [PATCHES] datum passed to macro which expects a pointer
On Sun, Apr 13, 2008 at 01:42:02AM +0100, Gregory Stark wrote: > "Gavin Sherry" <[EMAIL PROTECTED]> writes: > > > On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote: > >> Gavin Sherry <[EMAIL PROTECTED]> 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] datum passed to macro which expects a pointer
"Gavin Sherry" <[EMAIL PROTECTED]> writes: > On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote: >> Gavin Sherry <[EMAIL PROTECTED]> 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] datum passed to macro which expects a pointer
Gavin Sherry <[EMAIL PROTECTED]> writes: >> might as well just use PG_RETURN_DATUM instead of casting twice. > Oh of course. Updated patch attached. Applied, thanks. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] datum passed to macro which expects a pointer
On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote: > Gavin Sherry <[EMAIL PROTECTED]> 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] datum passed to macro which expects a pointer
Gavin Sherry <[EMAIL PROTECTED]> 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] datum passed to macro which expects a pointer
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 - 1.164 --- src/backend/utils/adt/varlena.c 12 Apr 2008 21:10:01 - *** 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 - 1.69 --- src/backend/utils/mb/mbutils.c 12 Apr 2008 22:55:49 - *** 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
Re: [PATCHES] datum passed to macro which expects a pointer
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. Was this just eyeball inspection or did you find a compiler that would complain about this? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
