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.von...@2ndquadrant.com <mailto: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
<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<and...@anarazel.de
<mailto: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 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
