Thread: Refactor parse analysis of EXECUTE command

This patch moves the parse analysis component of ExecuteQuery() and
EvaluateParams() into a new transformExecuteStmt() that is called from
transformStmt().  This makes EXECUTE behave more like other utility
commands.

Effects are that error messages can have position information (see 
regression test case), and it allows using external parameters in the 
arguments of the EXECUTE command.

I had previously inquired about this in [0] and some vague concerns were 
raised.  I haven't dug very deep on this, but I figure with an actual 
patch it might be easier to review and figure out if there are any problems.


[0]: 
https://www.postgresql.org/message-id/flat/ed2767e5-c506-048d-8ddf-280ecbc9e1b7%402ndquadrant.com

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> This patch moves the parse analysis component of ExecuteQuery() and
> EvaluateParams() into a new transformExecuteStmt() that is called from
> transformStmt().

Uhmm ... no actual patch attached?

            regards, tom lane

On 2019-11-02 16:00, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> This patch moves the parse analysis component of ExecuteQuery() and
>> EvaluateParams() into a new transformExecuteStmt() that is called from
>> transformStmt().
> 
> Uhmm ... no actual patch attached?

Oops, here it is.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello.

At Mon, 4 Nov 2019 08:53:18 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in 
> On 2019-11-02 16:00, Tom Lane wrote:
> > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> >> This patch moves the parse analysis component of ExecuteQuery() and
> >> EvaluateParams() into a new transformExecuteStmt() that is called from
> >> transformStmt().
> > Uhmm ... no actual patch attached?
> 
> Oops, here it is.

The patch just moves the first half of EvaluateParams that is
irrelevant to executor state to before portal parameters are set. I
looked with a suspect that extended protocol or SPI are affected but
AFAICS it doesn't seem to.

I dug into repository and found that transformExecuteStmt existed at
the time of implementing PREPARE-EXECUTE statements(28e82066a1) and
removed by the commit b9527e9840 which is related to
plan-invalidation.

git show -s --format=%B b9527e984092e838790b543b014c0c2720ea4f11
> In service of this, rearrange utility-statement processing so that parse
> analysis does not assume table schemas can't change before execution for
> utility statements (necessary because we don't attempt to re-acquire locks
> for utility statements when reusing a stored plan).  This requires some

Isn't this related to the current structure?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2019-11-08 08:13, Pavel Stehule wrote:
>     I dug into repository and found that transformExecuteStmt existed at
>     the time of implementing PREPARE-EXECUTE statements(28e82066a1) and
>     removed by the commit b9527e9840 which is related to
>     plan-invalidation.
> 
>     git show -s --format=%B b9527e984092e838790b543b014c0c2720ea4f11
>      > In service of this, rearrange utility-statement processing so
>     that parse
>      > analysis does not assume table schemas can't change before
>     execution for
>      > utility statements (necessary because we don't attempt to
>     re-acquire locks
>      > for utility statements when reusing a stored plan).  This
>     requires some
> 
>     Isn't this related to the current structure?
> 
> I think so it should be ok, because the transformation is still in same 
> statement - if I understand well.
> 
> So visibility of system catalogue or access to plan cache should not be 
> changed.

I think what that patch was addressing is, if you use a protocol-level 
prepare+execute with commands like CREATE INDEX, CREATE VIEW, or COPY 
and you change the table schema between the prepare and execute, things 
would break, for the reasons explained in the commit message.  So any 
parse analysis in utility statements that accesses table schemas needs 
to be done in the execute phase, not in the prepare phase, as one might 
think.

Parse analysis of EXECUTE does not access any tables, so if I understood 
this correctly, this concern doesn't apply here.

Interestingly, the above commit also removed the prepare-time 
transformation of ExplainStmt, but it was later put back and now has the 
comment "We used to postpone that until execution, but it's really 
necessary to do it during the normal parse analysis phase to ensure that 
side effects of parser hooks happen at the expected time."  So there 
appears to be a generally uneasy situation still about how to do this 
correctly.

Perhaps something could be done about the issue "because we don't 
attempt to re-acquire locks for utility statements when reusing a stored 
plan"?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-11-08 09:03, Pavel Stehule wrote:
>     Parse analysis of EXECUTE does not access any tables, so if I
>     understood
>     this correctly, this concern doesn't apply here.
> 
> 
> it should not be true - the subquery can be a expression.

Arguments of EXECUTE cannot be subqueries.

> 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.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

After digesting the discussion, let's reshuffle this a bit.

I have committed the change that adds the error location in one place. 
That worked independently.

Attached is a new patch that refactors things a bit to pass the 
ParseState into functions such as PrepareQuery() and ExecuteQuery() 
instead of passing the query string and query environment as a separate 
arguments.  We had already done that for most utility commands; there 
were just some left that happened to be involved in the current thread's 
discussion anyway.

That's a nice cosmetic improvement in any case, but I think that it 
would also help with the issue of passing parameters into some utility 
commands later on.  I will look into that some other time.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

This patch replaced query string by parse state on few places. It increase code consistency.

The new status of this patch is: Ready for Committer

On 2020-01-02 14:26, Pavel Stehule wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       not tested
> Spec compliant:           not tested
> Documentation:            not tested
> 
> This patch replaced query string by parse state on few places. It increase code consistency.
> 
> The new status of this patch is: Ready for Committer

committed, thanks

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services