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
