Thread: Add support for printing/reading MergeAction nodes
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
Attachment
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
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
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
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
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
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
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.
Thanks for the review comments.
Attached patch refactors the grammar/parser side per your comments. We no longer use InsertStmt/UpdateStmt/
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.
Pavan
--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
PostgreSQL Development, 24x7 Support, Training & Services
Attachment
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
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
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
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