Re: RFC: Restructuring pg_aggregate

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Tom Lane writes:
>> I was slightly bemused to notice that your implementation of it for
>> regular functions tests the privilege at plan startup but doesn't
>> actually throw the error until the function is called. What's the
>> point of that? Seems like we might as well throw the error in
>> init_fcache and not bother with storing a boolean.

> Yeah, it's a bit funny. I wanted to keep the fcache code from doing
> anything not to do with caching, and I wanted to keep the permission check
> in the executor, like it is for tables.

Well, init_fcache is part of executor startup, so that seems reasonably
parallel to the table case. I do not buy the idea that we shouldn't
throw an error if the function happens not to be called --- that makes
the behavior dependent on both planner choices and data conditions,
which seems like a bad idea. For comparison, we *will* throw a
permission error on tables even if the actual execution of the plan
turns out never to read a single row from that table.

(After a bit of code reading --- actually, at present init_fcache
doesn't get called until first use of the function, so it's a
distinction without a difference. But I have been thinking about
revising this so that function caches are set up during plan
initialization, which is why I'm questioning the point now.)

> There were a couple of cases, which I have not fully explored yet, for
> which this seemed like a good idea, such as some functions being in the
> plan but not being executed, or the permission check being avoided for
> some functions (e.g., cast functions).

Cast functions? Not sure that I see the point of excluding them.

What I'm inclined to do for aggregates is to check and throw the error
(if any) during ExecInitAgg, ie, plan startup for the Agg plan node.
I was just a tad startled to notice that the implementation wasn't
parallel for plain functions ...

regards, tom lane