Re: Add support for printing/reading MergeAction nodes

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

> Simon Riggs <simon(at)2ndquadrant(dot)com> writes:
> > On 4 April 2018 at 17:19, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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