support for MERGE

Here's a rebase of Simon/Pavan's MERGE patch to current sources.  I
cleaned up some minor things in it, but aside from rebasing, it's pretty
much their work (even the commit message is Simon's).

Adding to commitfest.

--
Álvaro Herrera

On 2020-Dec-31, Alvaro Herrera wrote:

> Here's a rebase of Simon/Pavan's MERGE patch to current sources.  I
> cleaned up some minor things in it, but aside from rebasing, it's pretty
> much their work (even the commit message is Simon's).

Rebased, no changes.


--
Álvaro Herrera                            39°49'30"S 73°17'W

On 1/8/21 8:22 PM, Alvaro Herrera wrote:
> On 2020-Dec-31, Alvaro Herrera wrote:
>
>> Here's a rebase of Simon/Pavan's MERGE patch to current sources.  I
>> cleaned up some minor things in it, but aside from rebasing, it's pretty
>> much their work (even the commit message is Simon's).
>
> Rebased, no changes.
>

I took a look at this today. Some initial comments (perhaps nitpicking,
in some cases).

1) sgml docs

This probably needs more work. Some of the sentences (in mvcc.sgml) are
so long I can't quite parse them. Maybe that's because I'm not a native
speaker, but still ... :-/

Also, there are tags missing - UPDATE/INSERT/... should be <command> or
maybe <literal>, depending on the context. I think the new docs are a
bit confused which of those it should be, but I'd say <command> should
be used for SQL commands and <literal> for MERGE actions?

It'd be a mess to list all the places, so the attached patch (0001)
tweaks this. Feel free to reject changes that you disagree with.

The patch also adds a bunch of XXX comments, suggesting some changes
(clarifications, removing unnecessarily duplicate text, etc.)


2) explain.c

I'm a bit confused about this

    case CMD_MERGE:
        operation = "Merge";
        foperation = "Foreign Merge";
        break;

because the commit message says foreign tables are not supported. So why
do we need this?


3) nodeModifyTable.c

ExecModifyTable does this:

    if (operation == CMD_MERGE)
    {
        ExecMerge(node, resultRelInfo, estate, slot, junkfilter);
        continue;
    }

i.e. it handles the MERGE far from the other DML operations. That seems
somewhat strange, especially without any comment - can't we refactor
this and handle it in the switch with the other DML?


4) preptlist.c

I propose to add a brief comment in preprocess_targetlist, explaining
what we need to do for CMD_MERGE (see 0001). And I think it'd be good to
explain why MERGE uses the same code as UPDATE (it's not obvious to me).


5) WHEN AND

I admit the "WHEN AND" conditions sounds a bit cryptic - it took me a
while to realize what this refers to. Is that a term established by SQL
Standard, or something we invented?


6) walsender.c

Huh, why does this patch touch this at all?


7) rewriteHandler.c

I see MERGE "doesn't support" rewrite rules in the sense that it simply
ignores them. Shouldn't it error-out instead? Seems like a foot-gun to
me, because people won't realize this limitation and may not notice
their rules don't fire.


8) varlena.c

Again, why are these changes to length checks in a MERGE patch?


9) parsenodes.h

Should we rename mergeTarget_relation to mergeTargetRelation? The
current name seems like a mix between two naming schemes.


10) per valgrind, there's a bug in ExecDelete

The table_tuple_delete may not set tmfd (which is no stack), leaving it
not initialized. But the code later branches depending on this. The 0002
patch fixes that by simply setting it to OK before the call, which makes
the valgrind error go away. But maybe it should be fixed in a different
way (e.g. by setting it at the beginning of table_tuple_delete).


regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 1/10/21 2:44 AM, Tomas Vondra wrote:
> 5) WHEN AND
>
> I admit the "WHEN AND" conditions sounds a bit cryptic - it took me a
> while to realize what this refers to. Is that a term established by SQL
> Standard, or something we invented?

The grammar gets it right but the error messages are nonsensical to me.
 I would like to see all user-facing instances of "WHEN AND" be replaced
by the admittedly more verbose "WHEN [NOT] MATCHED AND".
--
Vik Fearing

On Sun, Jan 10, 2021 at 1:44 AM Tomas Vondra
<[hidden email]> wrote:

> 5) WHEN AND
>
> I admit the "WHEN AND" conditions sounds a bit cryptic - it took me a
> while to realize what this refers to. Is that a term established by SQL
> Standard, or something we invented?

As Vik notes, this refers to the WHEN [NOT] MATCHED AND when-and-clause
so in that case I was referring to the "when-and_clause" portion.
Yes, that is part of the standard.

> 6) walsender.c
>
> Huh, why does this patch touch this at all?

Nothing I added, IIRC, nor am I aware of why that would exist.

> 7) rewriteHandler.c
>
> I see MERGE "doesn't support" rewrite rules in the sense that it simply
> ignores them. Shouldn't it error-out instead? Seems like a foot-gun to
> me, because people won't realize this limitation and may not notice
> their rules don't fire.

Simply ignoring rules is consistent with COPY, that was the only
reason for that choice. It could certainly throw an error instead.

> 8) varlena.c
>
> Again, why are these changes to length checks in a MERGE patch?

Nothing I added, IIRC, nor am I aware of why that would exist.

> 9) parsenodes.h
>
> Should we rename mergeTarget_relation to mergeTargetRelation? The
> current name seems like a mix between two naming schemes.

+1

We've had code from 4-5 people in the patch now, so I will re-review
myself to see if I can shed light on anything.

--
Simon Riggs                http://www.EnterpriseDB.com/

On 1/13/21 11:20 AM, Simon Riggs wrote:
Yes, I know what it was referring to, and I know that the feature is per
SQL standard. My point is that the "WHEN AND" term may be somewhat
unclear, especially when used in a error message (which typically has
very little context). I don't think SQL standard uses "WHEN AND" at all,
it simply talks about <search conditions> and that's it.

Makes sense.

OK, thanks.


regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Jan-13, Simon Riggs wrote:

> On Sun, Jan 10, 2021 at 1:44 AM Tomas Vondra
> <[hidden email]> wrote:

> > 8) varlena.c
> >
> > Again, why are these changes to length checks in a MERGE patch?
>
> Nothing I added, IIRC, nor am I aware of why that would exist.

Sorry, this was a borked merge.

--
Álvaro Herrera       Valdivia, Chile
"No hay ausente sin culpa ni presente sin disculpa" (Prov. francés)

Jaime Casanova just reported that this patch causes a crash on the
regression database with this query:

 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;

The reason is down to adjust_partition_tlist() not being willing to deal
with empty tlists.  So this is the most direct fix:

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 1fa4d84c42..d6b478ec33 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -976,7 +976,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
  conv_tl = map_partition_varattnos((List *) action->targetList,
   firstVarno,
   partrel, firstResultRel);
- conv_tl = adjust_partition_tlist(conv_tl, map);
+ if (conv_tl != NIL)
+ conv_tl = adjust_partition_tlist(conv_tl, map);
  tupdesc = ExecTypeFromTL(conv_tl);
  /* XXX gotta pfree conv_tl and tupdesc? */
 
 

But I wonder if it wouldn't be better to patch adjust_partition_tlist()
to return NIL on NIL input, instead, like this:

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 1fa4d84c42..6a170eea03 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -1589,6 +1589,9 @@ adjust_partition_tlist(List *tlist, TupleConversionMap *map)
  AttrMap    *attrMap = map->attrMap;
  AttrNumber attrno;
 
+ if (tlist == NIL)
+ return NIL;
+
  Assert(tupdesc->natts == attrMap->maplen);
  for (attrno = 1; attrno <= tupdesc->natts; attrno++)
  {

I lean towards the latter myself.

--
Álvaro Herrera       Valdivia, Chile

On Thu, Dec 31, 2020 at 8:48 AM Alvaro Herrera <[hidden email]> wrote:
> Here's a rebase of Simon/Pavan's MERGE patch to current sources.  I
> cleaned up some minor things in it, but aside from rebasing, it's pretty
> much their work (even the commit message is Simon's).

It's my impression that the previous discussion concluded that their
version needed pretty substantial design changes. Is there a plan to
work on that? Or was some of that stuff done already?

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2021-Jan-18, Robert Haas wrote:

> On Thu, Dec 31, 2020 at 8:48 AM Alvaro Herrera <[hidden email]> wrote:
> > Here's a rebase of Simon/Pavan's MERGE patch to current sources.  I
> > cleaned up some minor things in it, but aside from rebasing, it's pretty
> > much their work (even the commit message is Simon's).
>
> It's my impression that the previous discussion concluded that their
> version needed pretty substantial design changes. Is there a plan to
> work on that? Or was some of that stuff done already?

I think some of the issues were handled as I adapted the patch to
current sources.  However, the extensive refactoring that had been
recommended in the old threads has not been done.  I have these two
comments mainly:

https://postgr.es/m/CABOikdOeraX0RKojBs0jZxY5SmbQF3GJBP0+Rat=2g+VQsA+9g@...
https://postgr.es/m/7168.1547584387@...

I'll try to get to those, but I have some other patches that I need to
handle first.

--
Álvaro Herrera       Valdivia, Chile