Hi Tomas, Sorry for a delayed response.
On Mon, Oct 29, 2018 at 4:59 PM Tomas Vondra <tomas.von...@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 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 of MERGE is architecturally sound. I think creating hidden joins during parse-analysis to implement MERGE is 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 <and...@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 for merge, 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. 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. 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, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
