Обсуждение: Re: [PATCHES] array_accum aggregate

Stephen Frost <sfrost@snowman.net> writes:
> * Neil Conway (neilc@samurai.com) wrote:
>> There is no guarantee why SQL NULL and PG_RETURN_XYZ(NULL) refer to the
>> same thing -- use PG_RETURN_NULL() to return a SQL NULL value, or just
>> make the function strict.

> Huh, alright.  I'll probably just change it to PG_RETURN_NULL().

Unless the function actually *needs* to be non-strict, you should mark
it strict and omit the runtime test for null input altogether.  This
is the general way that it's done in existing backend C functions.
Doing it the other way is needlessly inconsistent (thus distracting
readers) and clutters the code.

(However, now that we support nulls in arrays, meseems a more consistent
definition would be that it allows null inputs and just includes them in
the output.  So probably you do need it non-strict.)

Personally though I'm much more concerned about the state datatype.
As-is I think it's not only ugly but probably a security hole.  If you
are declaring the state type as something other than what it really is
then you have to defend against two sorts of problems: someone being
able to crash the database by calling your function and passing it
something it didn't expect, or crashing the database by using your
function to pass some other function an input it didn't expect.  For
example, since you've got aaccum_sfunc declared to return anyarray when
it returns no such thing, something like array_out(aaccum_sfunc(...))
would trivially crash the backend.  It's possible that the error check
to insist on being called with an AggState context is a sufficient
defense against that, but I feel nervous about it, and would much rather
have a solution that isn't playing fast and loose with the type system.
Particularly if it's going to go into core rather than contrib.

I'm inclined to think that this example demonstrates a deficiency in the
aggregate-function design: there should be a way to declare what we're
really doing.  But I don't know exactly what that should look like.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> (However, now that we support nulls in arrays, meseems a more consistent
> definition would be that it allows null inputs and just includes them in
> the output.  So probably you do need it non-strict.)

This was my intention.

> I'm inclined to think that this example demonstrates a deficiency in the
> aggregate-function design: there should be a way to declare what we're
> really doing.  But I don't know exactly what that should look like.

I agree and would much rather have a clean solution which works with the
design than one which has to work outside it.  When I first was trying
to decide on the state-type I was looking through the PG catalogs for
essentially a "complex C type" which translated to a void*.  Perhaps
such a type could be added.  Unless that's considered along the lines of
an 'any' type it'd cause problems for the polymorphism aspect.

Another alternative would be to provide a seperate area for each
aggregate to put any other information it needs.  This would almost
certainly only be available to C functions but would essentially be a
void* which is provided through the AggState structure but tracked by
the aggregator routines and reset for each aggregate function being
run.  If that's acceptable, I don't think it'd be all that difficult to
implement.  With that, aaccum_sfunc and aaccum_ffunc would ignore the
state variable passed to them in favor of their custom structure
available through fcinfo->AggState (I expect they'd just keep the
state variable NULL and be marked non-strict, or set it to some constant
if necessary).  The pointer would have to be tracked somewhere and then
copied in/out on each call, but that doesn't seem too difficult to me.
After all, the state variable is already being tracked somewhere, this
would just sit next to it, in my head anyway.

I've got some time this weekend and would be happy to take a shot at
the second proposal if that's generally acceptable.

    Thanks,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Another alternative would be to provide a seperate area for each
> aggregate to put any other information it needs.

I'm not convinced that that's necessary --- the cases we have at hand
suggest that the transition function is perfectly capable of doing the
storage management it wants.  The problem is how to declare to CREATE
AGGREGATE that we're using a transition function of this kind rather
than the "stupid" functions it expects.  When the function is doing its
own storage management, we'd really rather that nodeAgg.c stayed out of
the way and didn't try to do any datum copying at all; having it copy a
placeholder bytea or anyarray or whatever is really a waste of cycles,
not to mention obscuring what is going on.  If nodeAgg just provided a
pass-by-value Datum, which the transition function could use to store a
pointer to storage it's handling, things would be a lot cleaner.

After a little bit of thought I'm tempted to propose that we handle this
by inventing a new pseudotype called something like "aggregate_state",
which'd be declared in the catalogs as pass-by-value, thereby
suppressing useless copying activity in nodeAgg.c.  You'd declare the
aggregate as having stype = aggregate_state, and the transition function
would have signature
    sfunc(aggregate_state, ... aggregate-input-type(s) ...)
        returns aggregate_state
and the final function of course
    ffunc(aggregate_state) returns aggregate-result-type

aggregate_state would have no other uses in the system, and its input
and output functions would raise an error, so type safety is assured
--- there would be no way to call either the sfunc or ffunc "manually",
except by passing a NULL value, which should be safe because that's what
they'd expect as the aggregate initial condition.

One advantage of doing it this way is that the planner could be taught
to recognize aggregates with stype = aggregate_state specially, and make
allowance for the fact that they'll use more workspace than meets the
eye.  If we don't have something like this then the planner is likely to
try to use hash aggregation in scenarios where it'd be absolutely fatal
to do so.  I'm not sure whether we'd want to completely forbid hash
aggregation when any stype = aggregate_state is present, but for sure we
want to assume that there's some pretty large amount of per-aggregate
state we don't know about.

            regards, tom lane

I wrote:
> aggregate_state would have no other uses in the system, and its input
> and output functions would raise an error, so type safety is assured
> --- there would be no way to call either the sfunc or ffunc "manually",
> except by passing a NULL value, which should be safe because that's what
> they'd expect as the aggregate initial condition.

Um, no, I take that back, unless you want to invent a separate
pseudotype for each such aggregate.  Otherwise you can crash it with

    my_ffunc(your_sfunc(null, whatever))

because my_ffunc will be expecting a datastructure different from what
it gets.

Maybe having a check for AggState call context is enough of a defense for
that, but I'm not really satisfied.  Back to the drawing board ...

            regards, tom lane

On Thu, Oct 12, 2006 at 06:58:52PM -0400, Tom Lane wrote:
> I wrote:
> > aggregate_state would have no other uses in the system, and its input
> > and output functions would raise an error, so type safety is assured
> > --- there would be no way to call either the sfunc or ffunc "manually",
> > except by passing a NULL value, which should be safe because that's what
> > they'd expect as the aggregate initial condition.
>
> Um, no, I take that back, unless you want to invent a separate
> pseudotype for each such aggregate.  Otherwise you can crash it with

<snip>

What this really calls for is a type that users are forbidden to
interact with directly. Basically, the type may only be used by C
functions and such C functions may not appear in an SQL query.

Seems the only way to safely deal with datums you don't know the
representation of.

The name "internal" would be nice, but it's taken :(

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog@svana.org> writes:
> What this really calls for is a type that users are forbidden to
> interact with directly. Basically, the type may only be used by C
> functions and such C functions may not appear in an SQL query.

That's not really the flavor of solution I'd like to have.  Ideally,
it'd actually *work* to write

    my_ffunc(my_sfunc(my_sfunc(null, 1), 2))

and get the same result as aggregating over the values 1 and 2.  The
trick is to make sure that my_sfunc and my_ffunc can only be used
together.  Maybe we do need a type for each such aggregate ...

In any case, "internal" isn't quite the right model, because with that,
the type system is basically disclaiming all knowledge of the data's
properties.  With an "aggregate_state" datatype, the type system would
be asserting that it's OK to use this type (or more accurately, these
functions) in an aggregate.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> That's not really the flavor of solution I'd like to have.  Ideally,
> it'd actually *work* to write
>
>     my_ffunc(my_sfunc(my_sfunc(null, 1), 2))
>
> and get the same result as aggregating over the values 1 and 2.  The
> trick is to make sure that my_sfunc and my_ffunc can only be used
> together.  Maybe we do need a type for each such aggregate ...

In general I like this idea but there are some complications, the main
one being where would the memory be allocated?  I guess we could just
always use the QueryContext.  The other issue is, in the above scenario
is it acceptable to modify the result of my_sfunc(null, 1) in the ,2
call?  Normally it's unacceptable to modify your input, aggregates get a
special exception when called as an aggregate, but in this case you'd
have to copy the entire custom structure underneath, I'd think.

As for a type for each such aggregate, that seems reasonable to me,
honestly.  I'd like for the type creation to be reasonably simple
though, if possible.  ie: could we just provide NULLs for the
input/output functions instead of having to implement functions that
just return an error?  Or perhaps have a polymorphic function which can
be reused that just returns an error.  Additionally, we'd have to be
able to mark the types as being polymorhpic along the same lines as
anyelement/anyarray.  Hopefully that's already easy, but if not it'd be
nice if it could be made easy...

    Thanks,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
>> That's not really the flavor of solution I'd like to have.  Ideally,
>> it'd actually *work* to write
>> my_ffunc(my_sfunc(my_sfunc(null, 1), 2))

> In general I like this idea but there are some complications, the main
> one being where would the memory be allocated?

In the agg context if called with that context, else
CurrentMemoryContext will do fine.

> The other issue is, in the above scenario
> is it acceptable to modify the result of my_sfunc(null, 1) in the ,2
> call?

Yes, because the only place a nonnull value of the type could have come
from is a my_sfunc call; since it's a pseudotype, we don't allow it on
disk.  (We might also need a hack to prevent the type from being used as
a record-type component ... not sure if that comes for free with being a
pseudotype currently.)

> As for a type for each such aggregate, that seems reasonable to me,
> honestly.

The ugly part is that we'd still need a way for the planner to recognize
this class of types.

> Additionally, we'd have to be
> able to mark the types as being polymorhpic along the same lines as
> anyelement/anyarray.

What for?

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > The other issue is, in the above scenario
> > is it acceptable to modify the result of my_sfunc(null, 1) in the ,2
> > call?
>
> Yes, because the only place a nonnull value of the type could have come
> from is a my_sfunc call; since it's a pseudotype, we don't allow it on
> disk.  (We might also need a hack to prevent the type from being used as
> a record-type component ... not sure if that comes for free with being a
> pseudotype currently.)

Ah, ok.

> > As for a type for each such aggregate, that seems reasonable to me,
> > honestly.
>
> The ugly part is that we'd still need a way for the planner to recognize
> this class of types.

Hrm, true.  I would agree that they would need more memory than most
aggregates.  It seems likely to me that worst offenders are going to be
of the array_accum type- store all tuples coming in.  Therefore, my
suggestion would be that the memory usage be estimated along those lines
and allow for hashagg when there's enough memory to keep all the tuples
(or rather the portion of the tuple going into the aggregate) in memory
(plus some amount of overhead, maybe 10%).  That doesn't help with how
to get the planner to recognize this class of types though.  I don't
suppose we already have a class framework which the planner uses to
group types which have similar characteristics?

> > Additionally, we'd have to be
> > able to mark the types as being polymorhpic along the same lines as
> > anyelement/anyarray.
>
> What for?

So that the finalfunc can be polymorphic along the lines of my suggested
aaccum_sfunc(anyarray,anyelement) returns anyarray and
aaccum_ffunc(anyarray) returns anyarray.  If the state type isn't
polymorphic then PG won't let me create a function from it which returns
a polymorphic, aiui.

    Thanks,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
>>> Additionally, we'd have to be
>>> able to mark the types as being polymorhpic along the same lines as
>>> anyelement/anyarray.
>>
>> What for?

> So that the finalfunc can be polymorphic along the lines of my suggested
> aaccum_sfunc(anyarray,anyelement) returns anyarray and
> aaccum_ffunc(anyarray) returns anyarray.

Hmm ... there's not any real need for this at the level of the
aggregate, because we resolve a polymorphic aggregate's output type
directly from its input type(s), and we've already established that the
general-purpose agg code doesn't need to be able to infer anything about
the transition data type.  However the function code is going to
complain if you try to declare "my_sfunc(aggregate_state) returns
anyarray" because that looks ill-formed ... and indeed that kinda kills
the idea of being able to call my_sfunc standalone anyway.

Maybe we need a more radical solution in which the sfunc/ffunc don't
exist as separately callable entities at all.  That would sidestep the
whole need to have a type-system representation for the state data,
as well as eliminate worries about whether we've sufficiently plugged
the security risks of being able to call one of them in an unexpected
context.  Not sure what this should look like in the catalogs though.

            regards, tom lane

What is the status of this feature addition?

---------------------------------------------------------------------------

Stephen Frost wrote:
-- Start of PGP signed section.
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > (However, now that we support nulls in arrays, meseems a more consistent
> > definition would be that it allows null inputs and just includes them in
> > the output.  So probably you do need it non-strict.)
>
> This was my intention.
>
> > I'm inclined to think that this example demonstrates a deficiency in the
> > aggregate-function design: there should be a way to declare what we're
> > really doing.  But I don't know exactly what that should look like.
>
> I agree and would much rather have a clean solution which works with the
> design than one which has to work outside it.  When I first was trying
> to decide on the state-type I was looking through the PG catalogs for
> essentially a "complex C type" which translated to a void*.  Perhaps
> such a type could be added.  Unless that's considered along the lines of
> an 'any' type it'd cause problems for the polymorphism aspect.
>
> Another alternative would be to provide a seperate area for each
> aggregate to put any other information it needs.  This would almost
> certainly only be available to C functions but would essentially be a
> void* which is provided through the AggState structure but tracked by
> the aggregator routines and reset for each aggregate function being
> run.  If that's acceptable, I don't think it'd be all that difficult to
> implement.  With that, aaccum_sfunc and aaccum_ffunc would ignore the
> state variable passed to them in favor of their custom structure
> available through fcinfo->AggState (I expect they'd just keep the
> state variable NULL and be marked non-strict, or set it to some constant
> if necessary).  The pointer would have to be tracked somewhere and then
> copied in/out on each call, but that doesn't seem too difficult to me.
> After all, the state variable is already being tracked somewhere, this
> would just sit next to it, in my head anyway.
>
> I've got some time this weekend and would be happy to take a shot at
> the second proposal if that's generally acceptable.
>
>     Thanks,
>
>         Stephen
-- End of PGP section, PGP failed!

--
  Bruce Momjian  <bruce@momjian.us>          http://momjian.us
  EnterpriseDB                               http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +