MERGE SQL statement for PG12

On 06/19/2018 07:06 AM, Pavan Deolasee wrote:

Hello,

I would like to resubmit the MERGE patch for PG12. The past
discussions about the patch can be found here [1] [2].

The patch is rebased on the current master. But otherwise I haven't
done any further work on it since it was punted from PG11. Couple of
hackers had expressed desire to review the patch much more carefully
and possibly also help in reworking some bits of it. So the idea is to
get those reviews going. Once that happens, I can address the review
comments in a more meaningful way.

Thanks,
Pavan

[1]
/messages/by-id/CANP8+jKitBSrB7oTgT9CY2i1ObfOt36z0XMraQc+Xrz8QB0nXA@mail.gmail.com

[2]
/messages/by-id/CAH2-WzkJdBuxj9PO=2QaO9-3h3xGbQPZ34kJH=HukRekwM-GZg@mail.gmail.com

--
\302\240Pavan Deolasee http://www.2ndQuadrant.com/
\302\240PostgreSQL Development, 24x7 Support, Training & Services

It's already in the commitfest, although I think it's almost certain to
be pushed out to the September CF. I'll add this email to the existing item.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jun 19, 2018 at 4:41 PM, Andrew Dunstan <
andrew.dunstan@2ndquadrant.com> wrote:

It's already in the commitfest, although I think it's almost certain to be
pushed out to the September CF. I'll add this email to the existing item.

Thanks Andrew; I was gonna do that once the email gets archives but thanks
for taking care of it. I changed the status to Needs Review, though I
understand this patch might be pushed to Sep CF. There were suggestions in
the last release cycle that this feature should get early into PG12, if at
all, given the complexity of the patch. So I thought it makes sense to
submit it early.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Jun 19, 2018 at 4:06 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

I would like to resubmit the MERGE patch for PG12. The past discussions
about the patch can be found here [1] [2].

FWIW, I'm really glad that you're going to work on this for v12.

--
Peter Geoghegan

On 2018-Jun-19, Pavan Deolasee wrote:

Hello,

I would like to resubmit the MERGE patch for PG12. The past discussions
about the patch can be found here [1] [2].

Hello. A very minor thing, please see commit 15a8f8caad14 and the
discussion that led to it.

Thanks

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I've rebased the patch against the current master. The patch hasn't changed
much since the last time.

I hope that the patch gets some review this CF so that we're left with
enough time to address those reviews and get the patch committed early in
the cycle. Copying Tom and Andres since they had expressed willingness to
review the patch in detail when the new development cycle opens. But fresh
reviews from Peter G, others is welcome too.

On Mon, Jun 25, 2018 at 11:13 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Hello. A very minor thing, please see commit 15a8f8caad14 and the
discussion that led to it.

Alvaro, thanks for the reminder. I've fixed that in the rebased patch.
While working on it, I thought we should just consolidate these different
counters in an array. Something like this:

+typedef enum TupleCounterType
+{
+ TUPLE_COUNTER_PROCESSED = 0,
+ TUPLE_COUNTER_CONFLICTING,
+ TUPLE_COUNTER_IOS_HEAP_FETCHES,
+ TUPLE_COUNTER_MERGE_INSERTED,
+ TUPLE_COUNTER_MERGE_UPDATED,
+ TUPLE_COUNTER_MERGE_DELETED,
+ TUPLE_COUNTER_FILTERED_BY_JOIN_QUALS,
+ TUPLE_COUNTER_FILTERED_BY_OTHER_QUALS,
+ TUPLE_COUNTER_MAX_COUNT /* should be last */
+} TupleCounterType;
+
 typedef struct Instrumentation
 {
  /* Parameters set at node creation: */
@@ -56,14 +69,9 @@ typedef struct Instrumentation
  /* Accumulated statistics across all completed cycles: */
  double startup; /* Total startup time (in seconds) */
  double total; /* Total total time (in seconds) */
- double ntuples; /* Total tuples produced */
- /* Additional node-specific tuple counters */
- double node_ntuples1;
- double node_ntuples2;
- double node_ntuples3;
+ /* Tuple counters */
+ double ntuples[TUPLE_COUNTER_MAX_COUNT];
  double nloops; /* # of run cycles for this node */
- double nfiltered1; /* # tuples removed by scanqual or joinqual */
- double nfiltered2; /* # tuples removed by "other" quals */
  BufferUsage bufusage; /* Total buffer usage */
 } Instrumentation;

And then have matching macros to get/set/manage those counters per type. Do
you see a value in doing so?

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, 4 Sep 2018 at 00:01, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

I've rebased the patch against the current master. The patch hasn't changed much since the last time.

Hi Pavan,

I had this crash when running sqlsmith against the previous version of
this patch and just confirmed it still crash with this version (which
makes sense because you said patch hasn't changed much)

To reproduce run this query against regression database:

"""
MERGE INTO public.pagg_tab_ml_p3 as target_0
USING public.prt2_l_p3_p2 as ref_0 ON target_0.a = ref_0.a
WHEN MATCHED AND cast(null as tid) <= cast(null as tid) THEN DELETE;
"""

attached is a backtrace

--
Jaime Casanova www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Sep 4, 2018 at 11:51 AM Jaime Casanova <
jaime.casanova@2ndquadrant.com> wrote:

On Tue, 4 Sep 2018 at 00:01, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

I've rebased the patch against the current master. The patch hasn't

changed much since the last time.

Hi Pavan,

I had this crash when running sqlsmith against the previous version of
this patch and just confirmed it still crash with this version (which
makes sense because you said patch hasn't changed much)

Hi Jaime,

Thanks for taking efforts to run sqlsmith. I've fixed the problem in the
attached patch. Please confirm and let me know if sqlsmith throws more
errors with the revised version.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

A new version rebased against the current master is attached.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, 24 Sep 2018 at 05:15, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

A new version rebased against the current master is attached.

Hi Pavan,

A day after you posted this patch commit
29c94e03c7d05d2b29afa1de32795ce178531246 removed ExecStoreTuple.
I'm right in believe that the change in
src/backend/executor/execMerge.c should be for ExecStoreHeapTuples?

-       ExecStoreTuple(&tuple, mtstate->mt_existing, buffer, false);
+       ExecStoreHeapTuple(&tuple, mtstate->mt_existing, false);

--
Jaime Casanova www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sun, Sep 30, 2018 at 2:55 AM Jaime Casanova <
jaime.casanova@2ndquadrant.com> wrote:

On Mon, 24 Sep 2018 at 05:15, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

A new version rebased against the current master is attached.

Hi Pavan,

A day after you posted this patch commit
29c94e03c7d05d2b29afa1de32795ce178531246 removed ExecStoreTuple.
I'm right in believe that the change in
src/backend/executor/execMerge.c should be for ExecStoreHeapTuples?

-       ExecStoreTuple(&tuple, mtstate->mt_existing, buffer, false);
+       ExecStoreHeapTuple(&tuple, mtstate->mt_existing, false);

Hi Jaime,

Thanks for keeping an eye on the patch. I've rebased the patch against the
current master. A new version is attached.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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."

+    * 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

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&gt; 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

Hi Tomas,

On Thu, Nov 22, 2018 at 10:03 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:

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.

Ok. I will try that approach again. In the meanwhile, I am posting a
rebased version. There had been quite a lot changes on partitioning side
and that caused non-trivial conflicts. I noticed a couple of problems
during the rebase, but I haven't attempted to address them fully yet, since
I think a detailed review at some point might require us to change that
anyways.

- Noticed that partColsUpdated is not set correctly in case of MERGE since
we never get to call expand_partitioned_rtentry() for the partition table
in case of MERGE. This right now does not cause significant problem, since
we initialise the required states by other means. But we should fix this.

- I am not entirely sure if the tuple-conversion business is bug-free for
MERGE, post this rebase. Since this code has changed quite a bit in the
master, I will have another look and check. The tests do not show any
issues, but that could also be because of lack of tests in this area with
respect to MERGE.

- I am not sure if I adopted the slot related changes introduced
by 1a0586de3657cd35581f0639c87d5050c6197bb7.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi Pavan,

Thanks for continuing to work on this.

On 2018/11/27 20:18, Pavan Deolasee wrote:

Ok. I will try that approach again. In the meanwhile, I am posting a
rebased version. There had been quite a lot changes on partitioning side
and that caused non-trivial conflicts. I noticed a couple of problems
during the rebase, but I haven't attempted to address them fully yet, since
I think a detailed review at some point might require us to change that
anyways.

- Noticed that partColsUpdated is not set correctly in case of MERGE since
we never get to call expand_partitioned_rtentry() for the partition table
in case of MERGE. This right now does not cause significant problem, since
we initialise the required states by other means. But we should fix this.

Regarding this, you may want to take a look at the following patch that
refactors things in this area.

https://commitfest.postgresql.org/20/1778/

Among other changes (such as completely redesigned inheritance_planner),
expand_partitioned_rtentry is now called after entering make_one_rel()
with the patch, which is much later than currently. That allows us to
initialize RTEs and RelOptInfos for only the partitions that are left
after pruning. As of now, it's called at a point in subquery_planner
where it's too soon to prune, so expand_partitioned_rtentry ends up
building RTEs for *all* partitions. I think that is something we're going
to have change in the long run anyway, so perhaps something you should
consider when designing related parts of MERGE. I will try to look at
that portion of your patch maybe next month.

Thanks,
Amit

On Tue, Nov 27, 2018 at 4:48 PM Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

In the meanwhile, I am posting a rebased version.

Another rebase on the current master.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi!

Looking at the commitfest as a novice contributor I was searching for patches to review without any reviewers set. And because I just spend some time and made a patch improving how REFRESH MATERIALIZED VIEW CONCURRENTLY works (does INSERTs/UPDATEs/DELETEs instead of just DELETEs/INSERTs) when I saw this patch I said to myself, great, MERGE is exactly what would be needed there. Because we already have a merge implementation there (requiring unique columns). I didn't know that I will discover such a long and beautiful thread.

So I will just add my 2c based on experience from REFRESH MATERIALIZED VIEW CONCURRENTLY work. I think that we would need an additional statement-level trigger for MERGE, instead of it being exposed as INSERT, UPDATE, and DELETE triggers. Because it is really tricky to make triggers work if you want to know how exactly the table as changed through MERGE if this is split into three separate triggers and transient relations. If we do not have a new statement-level trigger for MERGE, then this is really just a syntactic sugar on top of INSERTs, UPDATEs, and DELETEs.

Mitar

On Thu, Jan 3, 2019 at 2:11 AM Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

On Tue, Nov 27, 2018 at 4:48 PM Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

In the meanwhile, I am posting a rebased version.

Another rebase on the current master.

I feel like there has been some other thread where this was discussed,
but I can't find it right now. I think that the "query construction"
logic in transformMergeStmt is fundamentally the wrong way to do this.
I think several others have said the same. And I don't think this
should be considered for commit until that is rewritten.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jan 10, 2019 at 1:15 PM Robert Haas <robertmhaas@gmail.com> wrote:

I feel like there has been some other thread where this was discussed,
but I can't find it right now. I think that the "query construction"
logic in transformMergeStmt is fundamentally the wrong way to do this.
I think several others have said the same. And I don't think this
should be considered for commit until that is rewritten.

I agree with that.

I think that it's worth acknowledging that Pavan is in a difficult
position with this patch. As things stand, Pavan really needs input
from a couple of people to put the query construction stuff on the
right path, and that input has not been forthcoming. I'm not
suggesting that anybody has failed to meet an obligation to Pavan to
put time in here, or that anybody has suggested that this is down to a
failure on Pavan's part. I'm merely pointing out that Pavan is in an
unenviable position with this patch, through no fault of his own, and
despite a worthy effort.

I hope that he sticks it out, because that seems to be what it takes
to see something like this through. I don't know how to do better at
that.

--
Peter Geoghegan