Horton

Thread

Commits

  1. Improve parse representation for MERGE

  1. Add support for printing/reading MergeAction nodes

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2018-04-04T15:13:23Z

    Hello, hackers!

When debugging is enabled for server logging, isolation tests fail 
because there're no corresponding output functions for InsertStmt / 
DeleteStmt / UpdateStmt that are used in the output of the MergeAction 
nodes (see the attached regressions diffs and output). I also attached a 
try that makes the tests pass. Sorry if I missed that it was already 
discussed somewhere.

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  2. Re: Add support for printing/reading MergeAction nodes

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-04T16:19:34Z

    Marina Polyakova <m.polyakova@postgrespro.ru> writes:
> When debugging is enabled for server logging, isolation tests fail 
> because there're no corresponding output functions for InsertStmt / 
> DeleteStmt / UpdateStmt that are used in the output of the MergeAction 
> nodes (see the attached regressions diffs and output). I also attached a 
> try that makes the tests pass. Sorry if I missed that it was already 
> discussed somewhere.

Uh ... what?

Those node types are supposed to appear in raw grammar output only;
they should never survive past parse analysis.

If the MERGE patch has broken this, I'm going to push back on that
and push back on it hard, because it probably means there are a
whole bunch of other raw-grammar-output-only node types that can
now get past the parser stage as well, as children of these nodes.
The answer to that is not to add a ton of new infrastructure, it's
"you did MERGE wrong".

BTW, poking around in the grammar, I notice that MergeStmt did not
get added to RuleActionStmt.  That seems like a rather serious
omission.

			regards, tom lane

  3. Re: Add support for printing/reading MergeAction nodes

    Simon Riggs <simon@2ndquadrant.com> — 2018-04-04T16:57:39Z

    On 4 April 2018 at 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> BTW, poking around in the grammar, I notice that MergeStmt did not
> get added to RuleActionStmt.  That seems like a rather serious
> omission.

MERGE isn't a privilege, a trigger action or a policy action. Why
would it be in RuleActionStmt?

Could you explain what command you think should be supported?

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

  4. Re: Add support for printing/reading MergeAction nodes

    Simon Riggs <simon@2ndquadrant.com> — 2018-04-04T17:07:08Z

    On 4 April 2018 at 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marina Polyakova <m.polyakova@postgrespro.ru> writes:
>> When debugging is enabled for server logging, isolation tests fail
>> because there're no corresponding output functions for InsertStmt /
>> DeleteStmt / UpdateStmt that are used in the output of the MergeAction
>> nodes (see the attached regressions diffs and output). I also attached a
>> try that makes the tests pass. Sorry if I missed that it was already
>> discussed somewhere.
>
> Uh ... what?
>
> Those node types are supposed to appear in raw grammar output only;
> they should never survive past parse analysis.

So if I understand this correctly, it has nothing to do with the
isolation tester, that's just the place where the report was from.

Which debug mode are we talking about, please?

> If the MERGE patch has broken this, I'm going to push back on that
> and push back on it hard, because it probably means there are a
> whole bunch of other raw-grammar-output-only node types that can
> now get past the parser stage as well, as children of these nodes.
> The answer to that is not to add a ton of new infrastructure, it's
> "you did MERGE wrong".

MERGE hasn't broken anything. Marina is saying that the debug output
for MERGE isn't generated correctly.

I accept it shouldn't give spurious messages and I'm sure we can fix.

MERGE contains multiple actions of Insert, Update and Delete and these
could be output in various debug modes. I'm not clear what meaning we
might attach to them if we looked since that differs from normal
INSERTs, UPDATEs, DELETEs, but lets see.

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

  5. Re: Add support for printing/reading MergeAction nodes

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-04T17:08:31Z

    Simon Riggs <simon@2ndquadrant.com> writes:
> On 4 April 2018 at 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, poking around in the grammar, I notice that MergeStmt did not
>> get added to RuleActionStmt.  That seems like a rather serious
>> omission.

> MERGE isn't a privilege, a trigger action or a policy action. Why
> would it be in RuleActionStmt?

Because it seems likely that somebody would want to write a rule along
the lines of "ON UPDATE TO mytable DO INSTEAD MERGE ...".

Looking a little further ahead, one can easily envision somebody
wanting to do "ON MERGE TO mytable DO INSTEAD something".  I'd be
prepared to give a pass on that for the present, partly because
it's not very clear what stuff from the original MERGE needs to be
available to the rule.  But the other case seems pretty fundamental.
MERGE is not supposed to be a utility command IMO, it's supposed to
be DML, and that means it needs to work anywhere that you could
write e.g. UPDATE.

			regards, tom lane

  6. Re: Add support for printing/reading MergeAction nodes

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-04T17:51:56Z

    Simon Riggs <simon@2ndquadrant.com> writes:
> On 4 April 2018 at 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If the MERGE patch has broken this, I'm going to push back on that
>> and push back on it hard, because it probably means there are a
>> whole bunch of other raw-grammar-output-only node types that can
>> now get past the parser stage as well, as children of these nodes.
>> The answer to that is not to add a ton of new infrastructure, it's
>> "you did MERGE wrong".

> MERGE contains multiple actions of Insert, Update and Delete and these
> could be output in various debug modes. I'm not clear what meaning we
> might attach to them if we looked since that differs from normal
> INSERTs, UPDATEs, DELETEs, but lets see.

What I'm complaining about is that that's a very poorly designed parsetree
representation.  It may not be the worst one I've ever seen, but it's
got claims in that direction.  You're repurposing InsertStmt et al to
do something that's *not* an INSERT statement, but by chance happens
to share some (not all) of the same fields.  This is bad because it
invites confusion, and then bugs of commission or omission (eg, assuming
that some particular processing has happened or not happened to subtrees
of that parse node).  The most egregious way in which it's a bad idea
is that, as I said, InsertStmt doesn't even exist in post-parse-analysis
trees so far as the normal type of INSERT is concerned.  This just opens
a whole batch of ways to screw up.  We have some types of raw parse nodes
that are replaced entirely during parse analysis, and we have some others
where it's convenient to use the same node type before and after parse
analysis, but we don't have any that are one way in one context and the
other way in other contexts.  And this is not the place to start.

I think you'd have been better off to fold all of those fields into
MergeAction, or perhaps make several variants of MergeAction.

			regards, tom lane

  7. Re: Add support for printing/reading MergeAction nodes

    Simon Riggs <simon@2ndquadrant.com> — 2018-04-04T18:08:20Z

    On 4 April 2018 at 18:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
>> On 4 April 2018 at 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> If the MERGE patch has broken this, I'm going to push back on that
>>> and push back on it hard, because it probably means there are a
>>> whole bunch of other raw-grammar-output-only node types that can
>>> now get past the parser stage as well, as children of these nodes.
>>> The answer to that is not to add a ton of new infrastructure, it's
>>> "you did MERGE wrong".
>
>> MERGE contains multiple actions of Insert, Update and Delete and these
>> could be output in various debug modes. I'm not clear what meaning we
>> might attach to them if we looked since that differs from normal
>> INSERTs, UPDATEs, DELETEs, but lets see.
>
> What I'm complaining about is that that's a very poorly designed parsetree
> representation.  It may not be the worst one I've ever seen, but it's
> got claims in that direction.  You're repurposing InsertStmt et al to
> do something that's *not* an INSERT statement, but by chance happens
> to share some (not all) of the same fields.  This is bad because it
> invites confusion, and then bugs of commission or omission (eg, assuming
> that some particular processing has happened or not happened to subtrees
> of that parse node).  The most egregious way in which it's a bad idea
> is that, as I said, InsertStmt doesn't even exist in post-parse-analysis
> trees so far as the normal type of INSERT is concerned.  This just opens
> a whole batch of ways to screw up.  We have some types of raw parse nodes
> that are replaced entirely during parse analysis, and we have some others
> where it's convenient to use the same node type before and after parse
> analysis, but we don't have any that are one way in one context and the
> other way in other contexts.  And this is not the place to start.
>
> I think you'd have been better off to fold all of those fields into
> MergeAction, or perhaps make several variants of MergeAction.

OK, that can be changed, will check and report back tomorrow.

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

  8. Re: Add support for printing/reading MergeAction nodes

    Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-04-05T10:31:34Z

    On Wed, Apr 4, 2018 at 11:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Simon Riggs <simon@2ndquadrant.com> writes:
> > On 4 April 2018 at 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> If the MERGE patch has broken this, I'm going to push back on that
> >> and push back on it hard, because it probably means there are a
> >> whole bunch of other raw-grammar-output-only node types that can
> >> now get past the parser stage as well, as children of these nodes.
> >> The answer to that is not to add a ton of new infrastructure, it's
> >> "you did MERGE wrong".
>
> > MERGE contains multiple actions of Insert, Update and Delete and these
> > could be output in various debug modes. I'm not clear what meaning we
> > might attach to them if we looked since that differs from normal
> > INSERTs, UPDATEs, DELETEs, but lets see.
>
> What I'm complaining about is that that's a very poorly designed parsetree
> representation.  It may not be the worst one I've ever seen, but it's
> got claims in that direction.  You're repurposing InsertStmt et al to
> do something that's *not* an INSERT statement, but by chance happens
> to share some (not all) of the same fields.  This is bad because it
> invites confusion, and then bugs of commission or omission (eg, assuming
> that some particular processing has happened or not happened to subtrees
> of that parse node).  The most egregious way in which it's a bad idea
> is that, as I said, InsertStmt doesn't even exist in post-parse-analysis
> trees so far as the normal type of INSERT is concerned.  This just opens
> a whole batch of ways to screw up.  We have some types of raw parse nodes
> that are replaced entirely during parse analysis, and we have some others
> where it's convenient to use the same node type before and after parse
> analysis, but we don't have any that are one way in one context and the
> other way in other contexts.  And this is not the place to start.
>
> I think you'd have been better off to fold all of those fields into
> MergeAction, or perhaps make several variants of MergeAction.
>
>
Hi Tom,

Thanks for the review comments.

Attached patch refactors the grammar/parser side per your comments. We no
longer use InsertStmt/UpdateStmt/DeleteStmt/SelectStmt as part of
MergeAction. Instead we only collect the necessary information for running
the INSERT/UPDATE/DELETE actions. Speaking of MergeAction itself, I decided
to use a new parser-only node named MergeWhenClause and removed unnecessary
members from the MergeAction node which now gets to planner/executor.

Regarding the original report by Marina I suspect she may have turned
debug_print_parse=on while running regression. I could reproduce the
failures in the isolation tests by doing same. The attached patch however
passes all tests with the following additional GUCs. So I am more confident
that we should have got the outfuncs.c support ok.

debug_print_parse=on
debug_print_rewritten=on
debug_print_plan=on

Also, I am now running tests with -DCOPY_PARSE_PLAN_TREES
-DRAW_EXPRESSION_COVERAGE_TEST since the buildfarm had earlier uncovered
some issues with those flags. No problem there too.

This now also enforces single VALUES clause in the grammar itself instead
of doing that check at parse-analyse time. So that's a net improvement too.

Thanks,
Pavan

-- 
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  9. Re: Add support for printing/reading MergeAction nodes

    Simon Riggs <simon@2ndquadrant.com> — 2018-04-05T11:12:06Z

    On 5 April 2018 at 11:31, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

> Attached patch refactors the grammar/parser side per your comments. We no
> longer use InsertStmt/UpdateStmt/DeleteStmt/SelectStmt as part of
> MergeAction. Instead we only collect the necessary information for running
> the INSERT/UPDATE/DELETE actions. Speaking of MergeAction itself, I decided
> to use a new parser-only node named MergeWhenClause and removed unnecessary
> members from the MergeAction node which now gets to planner/executor.

That looks good to me. Simply separation of duty.

> Regarding the original report by Marina I suspect she may have turned
> debug_print_parse=on while running regression. I could reproduce the
> failures in the isolation tests by doing same. The attached patch however
> passes all tests with the following additional GUCs. So I am more confident
> that we should have got the outfuncs.c support ok.
>
> debug_print_parse=on
> debug_print_rewritten=on
> debug_print_plan=on
>
> Also, I am now running tests with -DCOPY_PARSE_PLAN_TREES
> -DRAW_EXPRESSION_COVERAGE_TEST since the buildfarm had earlier uncovered
> some issues with those flags. No problem there too.

OK, so $OP fixed.

> This now also enforces single VALUES clause in the grammar itself instead of
> doing that check at parse-analyse time. So that's a net improvement too.

OK, that's good. I've updated the docs to show this restriction correctly.

I'll commit this tomorrow morning unless further comments or edits.

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

  10. Re: Add support for printing/reading MergeAction nodes

    Simon Riggs <simon@2ndquadrant.com> — 2018-04-06T09:39:19Z

    On 4 April 2018 at 18:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
>> On 4 April 2018 at 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> BTW, poking around in the grammar, I notice that MergeStmt did not
>>> get added to RuleActionStmt.  That seems like a rather serious
>>> omission.
>
>> MERGE isn't a privilege, a trigger action or a policy action. Why
>> would it be in RuleActionStmt?
>
> Because it seems likely that somebody would want to write a rule along
> the lines of "ON UPDATE TO mytable DO INSTEAD MERGE ...".
>
> Looking a little further ahead, one can easily envision somebody
> wanting to do "ON MERGE TO mytable DO INSTEAD something".  I'd be
> prepared to give a pass on that for the present, partly because
> it's not very clear what stuff from the original MERGE needs to be
> available to the rule.  But the other case seems pretty fundamental.
> MERGE is not supposed to be a utility command IMO, it's supposed to
> be DML, and that means it needs to work anywhere that you could
> write e.g. UPDATE.

MERGE is important because it is implemented by other databases,
making it an application portability issue and an important feature
for PostgreSQL adoption with real users.

Enhancing Rules to allow interoperation with MERGE doesn't fall into
that same category, so I don't see it needs to work anywhere you can
write UPDATE - that certainly isn't the case with triggers, row level
security policies or privileges.

With that said, we can still discuss it to see if it's possible.

... ON UPDATE TO foo DO INSTEAD MERGE...
would look like this
MERGE INTO foo USING (what?) ON (join condition) WHEN MATCHED THEN UPDATE
which if we managed to achieve that is simply a much poorer version of
UPDATE, since MERGE with a single WHEN clause is semantically similar
but higher overhead than a single DML operation. So if we implemented
it you wouldn't ever use it.

Achieving the marriage between rules and merge is made complex in the
source and join condition. MERGE is different from other DML in that
it always has two tables, so is hard to see how that work with rules
on just one table.

So that leaves the thought of could we do a more complex version with
some kind of exploitation of multi-DML features? Well possibly, but I
think its months of work. Adding this requested feature doesn't
enhance the original goal of application portability and standards
compliance, so there's no way to know when you've got it right.
Without a scope or a spec it would be difficult to see where that
would go.

I would be most glad to be proved wrong to allow us to implement
something here and I have no objection to others adding that, though
it seems clear that can't happen in this release.

For this release, in discussion with Stephen and others at the Dev
Meeting in Brussels the addition of features for RLS and partitioning
were decided as the priorities for MERGE ahead of other items. This
item wasn't mentioned by anyone before and not at all by any users
I've spoken to.

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

  11. Re: Add support for printing/reading MergeAction nodes

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2018-04-06T19:06:54Z

    I'm sorry I was very busy with the patch for pgbench..

On 04-04-2018 19:19, Tom Lane wrote:
> ...
> 
> BTW, poking around in the grammar, I notice that MergeStmt did not
> get added to RuleActionStmt.  That seems like a rather serious
> omission.

Thank you very much! I will try to do this, if you do not mind, of 
course..

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  12. Re: Add support for printing/reading MergeAction nodes

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2018-04-06T19:26:47Z

    Sorry for this late reply, I was very busy with the patch for pgbench..

On 04-04-2018 20:07, Simon Riggs wrote:
> ...
> Which debug mode are we talking about, please?

-d 5

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company