Thread: CREATE AGGREGATE disallows STYPE = internal
So I went off to convert contrib/intagg to a wrapper around the new core functions, along this line: CREATE OR REPLACE FUNCTION int_agg_state (internal, int4) RETURNS internal AS 'array_agg_transfn' LANGUAGE INTERNAL; CREATE OR REPLACE FUNCTION int_agg_final_array (internal) RETURNS int4[] AS 'array_agg_finalfn' LANGUAGE INTERNAL; CREATE AGGREGATE int_array_aggregate (BASETYPE = int4,SFUNC = int_agg_state,STYPE = internal,FINALFUNC = int_agg_final_array ); But it doesn't work: psql:int_aggregate.sql:27: ERROR: aggregate transition data type cannot be internal I thought about declaring the transition functions with some other datatype, but that seemed entirely unsafe. Now CREATE AGGREGATE has fairly good reason to reject internal as the transition type, since nodeAgg.c doesn't really know how to copy that type around --- but we're effectively *exploiting* that fact in the new array_agg stuff, as per the comment I added: /* * We cheat quite a lot here by assuming that a pointer datum will be * preserved intact when nodeAgg.c thinksit is a value of type "internal". * This will in fact work because internal is stated to be pass-by-value * inpg_type.h, and nodeAgg will never do anything with a pass-by-value * transvalue except pass it around in Datum form. But it's mighty * shaky seeing that internal is also stated to be 4 bytes wide in * pg_type.h. If nodeAgg didput the value into a tuple this would * crash and burn on 64-bit machines. */ So it seems like maybe we should open up the same technique to writers of add-on modules. You can certainly screw yourself up by connecting some incompatible internal-accepting functions together this way. So what I propose is that we allow STYPE = internal to be specified in CREATE AGGREGATE, but only by superusers, who are trusted not to create security holes anyway. Comments? regards, tom lane
> internal-accepting functions together this way. So what I propose is > that we allow STYPE = internal to be specified in CREATE AGGREGATE, > but only by superusers, who are trusted not to create security holes > anyway. Does that mean that it's possible to use as STYPE pointer to complex C-structure with internal pointers? If yes then performance of ts_stat() could be significantly increased. -- Teodor Sigaev E-mail: teodor@sigaev.ru WWW: http://www.sigaev.ru/
> You can certainly screw yourself up by connecting some incompatible > internal-accepting functions together this way. So what I propose is > that we allow STYPE = internal to be specified in CREATE AGGREGATE, > but only by superusers, who are trusted not to create security holes > anyway. > > Comments? To me, that sounds like a foot-gun you could take out a tank with. I would feel a lot better about it if we could invent some way of distinguishing between different types of "internal", based on what is actually being pointed to. ...Robert
Teodor Sigaev <teodor@sigaev.ru> writes: > Does that mean that it's possible to use as STYPE pointer to complex C-structure > with internal pointers? That's exactly what array_agg is doing. You have to be careful about which context you keep the data in ... regards, tom lane
"Robert Haas" <robertmhaas@gmail.com> writes: > I would feel a lot better about it if we could invent some way of > distinguishing between different types of "internal", based on what is > actually being pointed to. Yeah, we discussed that awhile ago, but nothing's been done about it. At this point I doubt it will happen for 8.4. regards, tom lane
On Fri, Nov 14, 2008 at 10:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Robert Haas" <robertmhaas@gmail.com> writes: >> I would feel a lot better about it if we could invent some way of >> distinguishing between different types of "internal", based on what is >> actually being pointed to. > > Yeah, we discussed that awhile ago, but nothing's been done about it. > At this point I doubt it will happen for 8.4. That's a shame. Do you happen to have a pointer to the relevant discussions in the archives? Without that, it seems that the change you're proposing is totally unsafe. It's not just a question of malicious activity: a clueless admin (a category we've all been in from time to time...) could relatively easily create a configuration that crashes the backend. ...Robert
"Robert Haas" <robertmhaas@gmail.com> writes: > On Fri, Nov 14, 2008 at 10:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> "Robert Haas" <robertmhaas@gmail.com> writes: >>> I would feel a lot better about it if we could invent some way of >>> distinguishing between different types of "internal", based on what is >>> actually being pointed to. >> >> Yeah, we discussed that awhile ago, but nothing's been done about it. >> At this point I doubt it will happen for 8.4. > That's a shame. Do you happen to have a pointer to the relevant > discussions in the archives? http://archives.postgresql.org/pgsql-hackers/2008-08/msg00644.php > Without that, it seems that the change you're proposing is totally > unsafe. It's not just a question of malicious activity: a clueless > admin (a category we've all been in from time to time...) could > relatively easily create a configuration that crashes the backend. It's no more dangerous than allowing the admin to create functions taking or returning internal to begin with. Basically, if you are superuser, there are no training wheels. regards, tom lane