Re: SQL-standard function body

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Thu, Apr 08, 2021 at 04:54:56PM +0900, Michael Paquier wrote:
>> Indeed, I agree that enforcing the availability of querystring
>> everywhere sounds like a sensible thing to do in terms of consistency,
>> and that's my impression when I scanned the parallel execution code,
>> and I don't really get why SQL function bodies should not bind by this
>> rule.  Would people object if I add an open item to track that?

> It makes sense, +1 for an open item.

So here's what I propose to do about this.

0001 attached reverts the patch's change to remove the NOT NULL
constraint on pg_proc.prosrc.  I think that was an extremely poor
decision; it risks breaking non-core PLs, and for that matter I'm not
sure the core PLs wouldn't crash on null prosrc.  It is not any harder
for the SQL-language-related code to condition its checks on not-null
prosqlbody instead of null prosrc.  Of course that then requires us to
put something into prosrc for these newfangled functions, but in 0001
I just used an empty string.  (This patch also adds an Assert to
standard_ExecutorStart checking that some source text was provided,
responding to Andres' point that we should be checking that upstream
of parallel query.  We should then revert b3ee4c503, but for simplicity
I didn't include that here.)

0002 addresses a different missing-source-text problem, which is that
the patch didn't bother to provide source text while running parse
analysis on the SQL function body.  That means no error cursors for
problems; which might seem cosmetic on the toy example I added to the
regression tests, but it won't be for people writing functions that
are dozens or hundreds of lines long.

Finally, 0003 might be a bit controversial: it changes the stored
prosrc for new-style SQL functions to be the query text of the CREATE
FUNCTION command.  The main argument I can see being made against this
is that it'll bloat the pg_proc entry.  But I think that that's
not a terribly reasonable concern, because the source text is going
to be a good deal smaller than the nodeToString representation in
just about every case.

The real value of 0003 of course would be to get an error cursor at
runtime, but I failed to create an example where that would happen
today.  Right now there are only three calls of executor_errposition,
and all of them are for cases that are already rejected by the parser,
so they're effectively unreachable.  A scenario that seems more likely
to be reachable is a failure reported during function inlining, but
most of the reasons I can think of for that also seem unreachable given
the already-parse-analyzed nature of the function body in these cases.
Maybe I'm just under-caffeinated today.

Another point here is that for any error cursor to appear, we need
not only source text at hand but also token locations in the query
tree nodes.  Right now, since readfuncs.c intentionally discards
those locations, we won't have that.  There is not-normally-compiled
logic to reload those location fields, though, and I think before too
long we'll want to enable it in some mainstream cases --- notably
parallel query's shipping of querytrees to workers.  However, until
it gets easier to reach cases where an error-with-location can be
thrown from the executor, I don't feel a need to do that.

I do have ambitions to make execution-time errors produce cursors
in more cases, so I think this will come to fruition before long,
but not in v14.

One could make an argument, therefore, for holding off 0003 until
there's more support for execution-time error cursors.  I don't
think we should though, for two reasons:
1. It'd be better to keep the pg_proc representation of new-style
SQL functions stable across versions.
2. Storing the CREATE text means we'll capture comments associated
with the function text, which is something that at least some
people will complain about the loss of.  Admittedly we have no way
to re-integrate the comments into the de-parsed body, but some
folks might be satisfied with grabbing the prosrc text.

Thoughts?

			regards, tom lane