Re: MERGE SQL statement for PG12
| От | Pavan Deolasee |
|---|---|
| Тема | Re: MERGE SQL statement for PG12 |
| Дата | |
| Msg-id | CABOikdOeraX0RKojBs0jZxY5SmbQF3GJBP0+Rat=2g+VQsA+9g@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: MERGE SQL statement for PG12 (Tomas Vondra <tomas.vondra@2ndquadrant.com>) |
| Ответы |
Re: MERGE SQL statement for PG12
(Tomas Vondra <tomas.vondra@2ndquadrant.com>)
|
| Список | pgsql-hackers |
Hi Tomas,
Sorry for a delayed response.
On Mon, Oct 29, 2018 at 4:59 PM Tomas Vondra <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 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."
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 <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 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
PostgreSQL Development, 24x7 Support, Training & Services
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Justin PryzbyДата:
Сообщение: Re: performance statistics monitoring without spamming logs