Re: Refactor parse analysis of EXECUTE command - Mailing list pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-11-08 09:03, Pavel Stehule wrote:
>> Minimally on SQL level is not possible do prepare on execute. So execute 
>> should be evaluate as one step.

> Well, that's kind of the question that is being discussed in this thread.

Yeah.  Having now taken a quick look at this patch, it makes me pretty
queasy.  In particular, it doesn't appear to add any support for
invalidation of cached EXECUTE commands when their parameter expressions
change.  You dismissed that as irrelevant because no table schemas would
be involved, but there's also the possibility of replacements of user
defined functions.  I'm not sure how easy it is to create a situation
where an EXECUTE statement is in plancache, but it's probably possible
(maybe using some other PL than plpgsql).  In that case, we really would
need the EXECUTE's transformed expressions to get invalidated if the
user drops or replaces a function they use.

In view of the ALTER TABLE bugs I'm struggling with over in [1], I feel
like this patch is probably going in the wrong direction.  We should
generally be striving to do all transformation of utility commands as
late as possible.  As long as a plancached utility statement contains
nothing beyond raw-parser output, it never needs invalidation.

You pointed to an old comment of mine about EXPLAIN that seems to argue
in the other direction, but digging in the commit log, I see that it
came from commit 08f8d478, whose log entry is perhaps more informative
than the comment:

    Do parse analysis of an EXPLAIN's contained statement during the normal
    parse analysis phase, rather than at execution time.  This makes parameter
    handling work the same as it does in ordinary plannable queries, and in
    particular fixes the incompatibility that Pavel pointed out with plpgsql's
    new handling of variable references.  plancache.c gets a little bit
    grottier, but the alternatives seem worse.

So what this really is all about is still the same old issue of how we
handle external parameter references in utility statements.  Maybe we
ought to focus on a redesign addressing that specific problem, rather
than nibbling around the edges.  It seems like the core of the issue
is that we have mechanisms for PLs to capture parameter references
during parse analysis, and those hooks aren't managed in a way that
lets them be invoked if we do parse analysis during utility statement
execution.  But we *need* to be able to do that.  ALTER TABLE already
does do that, yet we need to postpone its analysis to even later than
it's doing it now.

Another issue in all this is that for many utility statements, you
don't actually want injections of PL parameter references, for instance
it'd make little sense to allow "alter table ... add check (f1 > p1)"
if p1 is a local variable in the function doing the ALTER.  It's
probably time to have some explicit recognition and management of such
cases, rather than just dodging them by not invoking the hooks.

tl;dr: I think that we need to embrace parse analysis during utility
statement execution as a fully supported thing, not a stepchild.
Trying to make it go away is the wrong approach.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/10365.1558909428@sss.pgh.pa.us