MERGE SQL statement for PG12

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

MERGE SQL statement for PG12

Pavan Deolasee
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


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

0001-MERGE-SQL-Command-following-SQL-2016.patch (531K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Andrew Dunstan-8


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]
> https://www.postgresql.org/message-id/CANP8%2BjKitBSrB7oTgT9CY2i1ObfOt36z0XMraQc%2BXrz8QB0nXA%40mail.gmail.com
>
> [2]
> https://www.postgresql.org/message-id/CAH2-WzkJdBuxj9PO%3D2QaO9-3h3xGbQPZ34kJH%3DHukRekwM-GZg%40mail.gmail.com
>
> --
>  Pavan Deolasee http://www.2ndQuadrant.com/
>  PostgreSQL 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


Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Pavan Deolasee


On Tue, Jun 19, 2018 at 4:41 PM, Andrew Dunstan <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Peter Geoghegan-4
In reply to this post by Pavan Deolasee
On Tue, Jun 19, 2018 at 4:06 AM, Pavan Deolasee
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Alvaro Herrera-9
In reply to this post by Pavan Deolasee
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

Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Pavan Deolasee

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 <[hidden email]> 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

0001-MERGE-SQL-Command-following-SQL-2016_v2.patch (535K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Jaime Casanova-4
On Tue, 4 Sep 2018 at 00:01, Pavan Deolasee <[hidden email]> 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

crash.txt (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Pavan Deolasee


On Tue, Sep 4, 2018 at 11:51 AM Jaime Casanova <[hidden email]> wrote:
On Tue, 4 Sep 2018 at 00:01, Pavan Deolasee <[hidden email]> 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

0001-MERGE-SQL-Command-following-SQL-2016_v3.patch (535K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Pavan Deolasee

A new version rebased against the current master is attached.

Thanks,
Pavan

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

0001-MERGE-SQL-Command-following-SQL-2016_v4.patch (535K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Jaime Casanova-4
On Mon, 24 Sep 2018 at 05:15, Pavan Deolasee <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Pavan Deolasee


On Sun, Sep 30, 2018 at 2:55 AM Jaime Casanova <[hidden email]> wrote:
On Mon, 24 Sep 2018 at 05:15, Pavan Deolasee <[hidden email]> 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

0001-MERGE-SQL-Command-following-SQL-2016_v5.patch (535K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Tomas Vondra-4
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

Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Pavan Deolasee

Hi Tomas,

Sorry for a delayed response. 

On Mon, Oct 29, 2018 at 4:59 PM Tomas Vondra <[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Tomas Vondra-4
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
> <[hidden email] <mailto:[hidden email]>> 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<[hidden email]
> <mailto:[hidden email]>>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

Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Pavan Deolasee

Hi Tomas,

On Thu, Nov 22, 2018 at 10:03 PM Tomas Vondra <[hidden email]> 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

0001-MERGE-SQL-Command-following-SQL-2016_v6.patch (540K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Amit Langote-2
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


Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Pavan Deolasee
In reply to this post by Pavan Deolasee


On Tue, Nov 27, 2018 at 4:48 PM Pavan Deolasee <[hidden email]> 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

0001-MERGE-SQL-Command-following-SQL-2016_v7.patch (540K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Mitar
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
Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Robert Haas
In reply to this post by Pavan Deolasee
On Thu, Jan 3, 2019 at 2:11 AM Pavan Deolasee <[hidden email]> wrote:
> On Tue, Nov 27, 2018 at 4:48 PM Pavan Deolasee <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: MERGE SQL statement for PG12

Peter Geoghegan-4
On Thu, Jan 10, 2019 at 1:15 PM Robert Haas <[hidden email]> 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

12