Re: Add support for printing/reading MergeAction nodes

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.