Re: Add support for printing/reading MergeAction nodes

On 4 April 2018 at 18:51, 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.

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