Обсуждение: 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