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.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.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@anarazel.de 
> <mailto:andres@anarazel.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