support for MERGE

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
<tomas.vondra@enterprisedb.com> 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:

On Sun, Jan 10, 2021 at 1:44 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> 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.

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.

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.

Makes sense.

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.

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
<tomas.vondra@enterprisedb.com> 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 <alvherre@alvh.no-ip.org> 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 <alvherre@alvh.no-ip.org> 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:

/messages/by-id/CABOikdOeraX0RKojBs0jZxY5SmbQF3GJBP0+Rat=2g+VQsA+9g@mail.gmail.com
/messages/by-id/7168.1547584387@sss.pgh.pa.us

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

--
�lvaro Herrera Valdivia, Chile

On 1/18/21 11:48 AM, Alvaro Herrera wrote:

On 2021-Jan-18, Robert Haas wrote:

On Thu, Dec 31, 2020 at 8:48 AM Alvaro Herrera <alvherre@alvh.no-ip.org> 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:

/messages/by-id/CABOikdOeraX0RKojBs0jZxY5SmbQF3GJBP0+Rat=2g+VQsA+9g@mail.gmail.com
/messages/by-id/7168.1547584387@sss.pgh.pa.us

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

Since it does not appear this is being worked on for PG14 perhaps we
should close it in this CF and then reopen it when a patch is available?

Regards,
--
-David
david@pgmasters.net

On 2021-Mar-19, David Steele wrote:

Since it does not appear this is being worked on for PG14 perhaps we should
close it in this CF and then reopen it when a patch is available?

No, please move it to the next CF. Thanks

--
�lvaro Herrera Valdivia, Chile

On 3/19/21 10:44 AM, Alvaro Herrera wrote:

On 2021-Mar-19, David Steele wrote:

Since it does not appear this is being worked on for PG14 perhaps we should
close it in this CF and then reopen it when a patch is available?

No, please move it to the next CF. Thanks

Ugh. I clicked wrong and moved it to the 2021-09 CF. I'm not sure if
there's a way to fix it without creating a new entry.

Regards,
--
-David
david@pgmasters.net

On Fri, Mar 19, 2021 at 10:53:53AM -0400, David Steele wrote:

On 3/19/21 10:44 AM, Alvaro Herrera wrote:

On 2021-Mar-19, David Steele wrote:

Since it does not appear this is being worked on for PG14 perhaps we should
close it in this CF and then reopen it when a patch is available?

No, please move it to the next CF. Thanks

Ugh. I clicked wrong and moved it to the 2021-09 CF. I'm not sure if there's
a way to fix it without creating a new entry.

Let's get someone to go into the "database" and adjust it. ;-)

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Fri, Mar 19, 2021 at 6:03 PM Bruce Momjian <bruce@momjian.us> wrote:

On Fri, Mar 19, 2021 at 10:53:53AM -0400, David Steele wrote:

On 3/19/21 10:44 AM, Alvaro Herrera wrote:

On 2021-Mar-19, David Steele wrote:

Since it does not appear this is being worked on for PG14 perhaps we should
close it in this CF and then reopen it when a patch is available?

No, please move it to the next CF. Thanks

Ugh. I clicked wrong and moved it to the 2021-09 CF. I'm not sure if there's
a way to fix it without creating a new entry.

Let's get someone to go into the "database" and adjust it. ;-)

Yeah, we probably can clean that up on the database side :)

But what is it we *want* it to do? That is, what should be the target?
Is it 2021-07 with status WoA?

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On 2021-Mar-21, Magnus Hagander wrote:

On Fri, Mar 19, 2021 at 6:03 PM Bruce Momjian <bruce@momjian.us> wrote:

Let's get someone to go into the "database" and adjust it. ;-)

Yeah, we probably can clean that up on the database side :)

Thank you!

But what is it we *want* it to do? That is, what should be the target?
Is it 2021-07 with status WoA?

Yeah, that sounds like it, thanks.

--
�lvaro Herrera Valdivia, Chile

On Sun, Mar 21, 2021 at 2:57 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2021-Mar-21, Magnus Hagander wrote:

On Fri, Mar 19, 2021 at 6:03 PM Bruce Momjian <bruce@momjian.us> wrote:

Let's get someone to go into the "database" and adjust it. ;-)

Yeah, we probably can clean that up on the database side :)

Thank you!

But what is it we *want* it to do? That is, what should be the target?
Is it 2021-07 with status WoA?

Yeah, that sounds like it, thanks.

Done.

The log entry is still there, to show what has been done :)

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On 21 Mar 2021, at 14:57, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2021-Mar-21, Magnus Hagander wrote:

But what is it we *want* it to do? That is, what should be the target?
Is it 2021-07 with status WoA?

Yeah, that sounds like it, thanks.

This patch fails to apply, will there be a new version for this CF?

--
Daniel Gustafsson https://vmware.com/

On 2021-Sep-01, Daniel Gustafsson wrote:

On 21 Mar 2021, at 14:57, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2021-Mar-21, Magnus Hagander wrote:

But what is it we *want* it to do? That is, what should be the target?
Is it 2021-07 with status WoA?

Yeah, that sounds like it, thanks.

This patch fails to apply, will there be a new version for this CF?

No, sorry, there won't. I think it's better to close it as RfW at this
point, I'll create a new one when I have it.

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/

On 1 Sep 2021, at 14:25, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2021-Sep-01, Daniel Gustafsson wrote:

On 21 Mar 2021, at 14:57, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2021-Mar-21, Magnus Hagander wrote:

But what is it we *want* it to do? That is, what should be the target?
Is it 2021-07 with status WoA?

Yeah, that sounds like it, thanks.

This patch fails to apply, will there be a new version for this CF?

No, sorry, there won't. I think it's better to close it as RfW at this
point, I'll create a new one when I have it.

No worries, I did that now.

--
Daniel Gustafsson https://vmware.com/