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 thinks it is a value of type "internal".
* This will in fact work because internal is stated to be pass-by-value
* in pg_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 did put 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