Re: MERGE SQL statement for PG12

On 11/22/18 7:44 AM, Pavan Deolasee wrote:
>
> Hi Tomas,
>
> Sorry for a delayed response.
>
> On Mon, Oct 29, 2018 at 4:59 PM Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>> wrote:
>
> Hi Pavan,
>
> On 10/29/2018 10:23 AM, Pavan Deolasee wrote:
> >
> > ...
> >
> > Thanks for keeping an eye on the patch. I've rebased the patch
> > against the current master. A new version is attached.
> >
> > Thanks,
> > Pavan
> >
>
> It's good to see the patch moving forward. What are your thoughts
> regarding things that need to be improved/fixed to make it committable?
>
> I briefly discussed the patch with a couple of people at pgconf.eu
> <http://pgconf.eu> last
> week, and IIRC the main concern was how the merge is represented in
> parser/executor.
>
>
> Yes, Andres and to some extent Peter G had expressed concerns about
> that. Andres suggested that we should rework the parser and executor
> side. Here are some excerpts from his review comments.
>
> <review>
> "I don't think the parser / executor implementation
> ofMERGEis architecturally sound.  I think creating hidden joins during
> parse-analysis to implementMERGEis a seriously bad idea and it needs
> to be replaced by a different executor structure."
>
> +    * As top-level statements INSERT, UPDATE and DELETE have a Query,
> whereas
> +    * with MERGE the individual actions do not require separate planning,
> +    * only different handling in the executor. See nodeModifyTable handling
> +    * of commandType CMD_MERGE.
> +    *
> +    * A sub-query can include the Target, but otherwise the sub-query
> cannot
> +    * reference the outermost Target table at all.
> +    */
> +   qry->querySource = QSRC_PARSER;
>
> Why is this, and not building a proper executor node for merge that
> knows how to get the tuples, the right approach?  We did a rough
> equivalent for matview updates, and I think it turned out to be a pretty
> bad plan.
> </review>
>
> <review>
> On Fri, Apr 6, 2018 at 1:30 AM, Andres Freund<andres(at)anarazel(dot)de
> <mailto:andres(at)anarazel(dot)de>>wrote:
>
>
>
> My impression is that this simply shouldn't be going through
> nodeModifyTuple, but be it's own nodeMerge node. The trigger handling
> would need to be abstraced into execTrigger.c or such to avoid
> duplication.  We're now going from nodeModifyTable.c:ExecModifyTable()
> into execMerge.c:ExecMerge(), back to
> nodeModifyTable.c:ExecUpdate/Insert(). To avoid ExecInsert() doing
> things that aren't appropriate formerge, we then pass an actionState,
> which neuters part of ExecUpdate/Insert(). This is just bad.
>
>
> </review>
>
> To be honest, I need more directions on how to address these major
> architectural concerns. I'd tried to rework the executor part and had
> commented on that. But I know that was probably done in a hurry since we
> were trying to save a revert. Having said that, I am still not very sure
> how exactly the executor side should look like without causing too much
> duplication of handling nodeModifyTable() and proposed nodeMerge(). If
> Andres can give me further inputs, I will be more than happy to figure
> out the details and improve the patch.
>

I think not going through nodeModifyTable and having a separate
nodeMerge.c makes sense. It might result in some code being duplicated,
but I suppose that code can be shared (wrapped as a function, moved to
some file shared by the two nodes). I wouldn't expect the result to be
particularly ugly, at least not compared to how nodeModifyTable is
twisted with the current patch.

> As far as the parser side goes, do I understand correctly that instead
> of building a special join in transformMergeStmt() as the patch does
> today, we should follow what transformUpdateStmt() does for a potential
> join? i.e. just have a single RTE for the source relation and present it
> as a left hand side for the implicit join? If I get a little more
> direction on this, I can try to address the issues.
>

Not sure.

> It looks very likely that the patch won't get much review in the current
> state. But if I get inputs, I can work out the details and try to get
> the patch in committable state.
>
> BTW I am aware that the patch is bit-rotten because of the partitioning
> related changes. I will address those and post a revised patch soon.
>

thanks

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