Re: MERGE SQL statement for PG12
От | Tomas Vondra |
---|---|
Тема | Re: MERGE SQL statement for PG12 |
Дата | |
Msg-id | 359a3698-23f0-c97a-ff23-f4b783119fe4@2ndquadrant.com обсуждение исходный текст |
Ответ на | Re: MERGE SQL statement for PG12 (Pavan Deolasee <pavan.deolasee@gmail.com>) |
Ответы |
Re: MERGE SQL statement for PG12
|
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления: