Refactor parse analysis of EXECUTE command

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Refactor parse analysis of EXECUTE command

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Tom Lane-2
Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Peter Eisentraut-6
On 2019-11-02 16:00, Tom Lane wrote:
> Peter Eisentraut <[hidden email]> 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

0001-Refactor-parse-analysis-of-EXECUTE-command.patch (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Kyotaro Horiguchi-4
Hello.

At Mon, 4 Nov 2019 08:53:18 +0100, Peter Eisentraut <[hidden email]> wrote in
> On 2019-11-02 16:00, Tom Lane wrote:
> > Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Pavel Stehule


út 5. 11. 2019 v 11:28 odesílatel Kyotaro Horiguchi <[hidden email]> napsal:
Hello.

At Mon, 4 Nov 2019 08:53:18 +0100, Peter Eisentraut <[hidden email]> wrote in
> On 2019-11-02 16:00, Tom Lane wrote:
> > Peter Eisentraut <[hidden email]> 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?

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.

Regards

Pavel


regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Pavel Stehule
In reply to this post by Peter Eisentraut-6


po 4. 11. 2019 v 8:53 odesílatel Peter Eisentraut <[hidden email]> napsal:
On 2019-11-02 16:00, Tom Lane wrote:
> Peter Eisentraut <[hidden email]> 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.

I checked this patch, and I think so it's correct and wanted. It introduce transform stage for EXECUTE command, and move there the argument transformation.

This has sensible change - the code is much more correct now.

The patching, compilation was without any problems, make check-world too.

I was little bit confused about regress tests - the patch did some code refactoring and I expect so main target is same behave before and after patching. But the regress tests shows new feature that is just side effect (nice) of patch. More, the example is little bit strange - nobody will use prepared statements and execution in SQL function. It should be better commented.

I'll mark this patch as ready for commiters.

Regards

Pavel


--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Peter Eisentraut-6
In reply to this post by Pavel Stehule
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


Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Pavel Stehule


pá 8. 11. 2019 v 8:54 odesílatel Peter Eisentraut <[hidden email]> napsal:
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.

it should not be true - the subquery can be a expression.

Minimally on SQL level is not possible do prepare on execute. So execute should be evaluate as one step.



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
Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Pavel Stehule


pá 8. 11. 2019 v 13:34 odesílatel Peter Eisentraut <[hidden email]> napsal:
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.
ok

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

I say it not cleanly - I think so this change should be safe, because parsing, transforming, and execution must be in one statement.

Regards

Pavel

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: Refactor parse analysis of EXECUTE command

Tom Lane-2
In reply to this post by Peter Eisentraut-6
Peter Eisentraut <[hidden email]> 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@...