Refactor parse analysis of EXECUTE command

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

út 5. 11. 2019 v 11:28 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com>
napsal:

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?

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

po 4. 11. 2019 v 8:53 odesílatel Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> napsal:

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.

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

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

pá 8. 11. 2019 v 8:54 odesílatel Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> 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.

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

pá 8. 11. 2019 v 13:34 odesílatel Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> 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 <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]/messages/by-id/10365.1558909428@sss.pgh.pa.us, 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]: /messages/by-id/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