Обсуждение: 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
<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:

https://postgr.es/m/CABOikdOeraX0RKojBs0jZxY5SmbQF3GJBP0+Rat=2g+VQsA+9g@mail.gmail.com
https://postgr.es/m/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:
> 
> https://postgr.es/m/CABOikdOeraX0RKojBs0jZxY5SmbQF3GJBP0+Rat=2g+VQsA+9g@mail.gmail.com
> https://postgr.es/m/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/

Here's a new version.  Many of the old complaints have been fixed;
particularly, the handling of partitioned tables is now much cleaner and
straightforward.  Amit Langote helped considerably in getting this part
to shape -- thanks for that.  Amit also helped correct the EvalPlanQual
behavior, which wasn't quite up to snuff.

There are a few things that can still be improved here.  For one, I need
to clean up the interactions with table AM (and thus heapam.c etc).
Secondarily, and I'm now not sure that I really want to do it, is change
the representation for executor: instead of creating a fake join between
target and source, perhaps we should have just source, and give
optimizer a separate query to fetch tuples from target.

What I did do is change how the target table is represented from parse
analysis to executor.  For example, in the original code, there were two
RTEs that represented the target table; that is gone.  Now the target
table is always just the query's resultRelation.  This removes a good
number of ugly hacks that had been objected to.

I'll park this in the January commitfest now.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)

On 11/12/21 18:57, Alvaro Herrera wrote:
> Here's a new version.  Many of the old complaints have been fixed;
> particularly, the handling of partitioned tables is now much cleaner and
> straightforward.  Amit Langote helped considerably in getting this part
> to shape -- thanks for that.  Amit also helped correct the EvalPlanQual
> behavior, which wasn't quite up to snuff.
>

Thanks!

> There are a few things that can still be improved here.  For one, I need
> to clean up the interactions with table AM (and thus heapam.c etc).
> Secondarily, and I'm now not sure that I really want to do it, is change
> the representation for executor: instead of creating a fake join between
> target and source, perhaps we should have just source, and give
> optimizer a separate query to fetch tuples from target.
> 

When you say you're not sure you want to change this, is that because 
you don't have time for that, or because you think the current approach 
is better?

> What I did do is change how the target table is represented from parse
> analysis to executor.  For example, in the original code, there were two
> RTEs that represented the target table; that is gone.  Now the target
> table is always just the query's resultRelation.  This removes a good
> number of ugly hacks that had been objected to.
> 
> I'll park this in the January commitfest now.
> 


regards

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

On 2021-Nov-12, Tomas Vondra wrote:

> On 11/12/21 18:57, Alvaro Herrera wrote:

> > Secondarily, and I'm now not sure that I really want to do it, is change
> > the representation for executor: instead of creating a fake join between
> > target and source, perhaps we should have just source, and give
> > optimizer a separate query to fetch tuples from target.
> 
> When you say you're not sure you want to change this, is that because you
> don't have time for that, or because you think the current approach is
> better?

I'm not sure that doing it the other way will be better.  We'll have two
queries to optimize: one is reading from the data source, and the other
is fetching tuples from the target table based on the conditions applied
to each row obtained form the data source.  Right now, we just apply a
join and let the optimizer do it's thing.  It's simpler, but ...

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 2021-Nov-12, Zhihong Yu wrote:

> Hi,
> 
> +           skipped_path = total - insert_path - update_path - delete_path;
> 
> Should there be an assertion that skipped_path is not negative ?

Hm, yeah, added.

> +    * We maintain separate transaction tables for UPDATE/INSERT/DELETE since
> +    * MERGE can run all three actions in a single statement. Note that UPDATE
> +    * needs both old and new transition tables
> 
> Should the 'transaction' in the first line be transition ?

Oh, of course.

Uploaded fixup commits to
https://github.com/alvherre/postgres/commits/merge-15

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

Hi,

I got these warnings when compiling against current head:

tion -Wno-stringop-truncation -O2 -I../../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2   -c -o execMerge.o
execMerge.c
execMerge.c: In function ‘ExecMerge’:
execMerge.c:54:8: warning: unused variable ‘relkind’ [-Wunused-variable]
   54 |  char  relkind = resultRelInfo->ri_RelationDesc->rd_rel->relkind;
      |        ^~~~~~~

/usr/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2  -I../../../src/include  -D_GNU_SOURCE
-I/usr/include/libxml2 -flto=thin -emit-llvm -c -o execMerge.bc execMerge.c
 
execMerge.c:552:32: warning: if statement has empty body [-Wempty-body]
                                        RELKIND_PARTITIONED_TABLE);
                                                                  ^
execMerge.c:552:32: note: put the semicolon on a separate line to silence this warning
1 warning generated.


Regards
Daniel

On 2021-Nov-13, Daniel Westermann wrote:

> /usr/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2  -I../../../src/include  -D_GNU_SOURCE
-I/usr/include/libxml2 -flto=thin -emit-llvm -c -o execMerge.bc execMerge.c
 
> execMerge.c:552:32: warning: if statement has empty body [-Wempty-body]
>                                         RELKIND_PARTITIONED_TABLE);
>                                                                   ^
> execMerge.c:552:32: note: put the semicolon on a separate line to silence this warning

Oh wow, this may be a pretty serious problem actually.  I think it
represents a gap in testing.  Thanks for reporting.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

On 2021-Nov-12, Zhihong Yu wrote:

> This is continuation of review.
> 
> +                   elog(WARNING, "hoping nothing needed here");
> 
> the above warning can be dropped.

Hmm, yeah, the fact that this doesn't show up in the logs anywhere
suggests that there is a gap in testing.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 2021-Nov-12, Zhihong Yu wrote:

> +lmerge_matched:
> ...
> +   foreach(l, resultRelInfo->ri_matchedMergeAction)
> 
> I suggest expanding the foreach macro into the form of for loop where the
> loop condition has extra boolean variable merge_matched.
> Initial value for merge_matched can be true.
> Inside the loop, we can adjust merge_matched's value to control whether the
> for loop continues.
> This would avoid using goto label.

Heh, have you seen the definition of foreach() lately?  I do *not* want
to expand that anywhere.  Anyway, even if we had the old foreach()
(before commit 1cff1b95ab6d), ISTM that the goto makes the whole thing
much clearer.  For example, where would you do the
table_tuple_fetch_row_version call that currently lies after the goto
label but before the foreach loop starts?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"If you have nothing to say, maybe you need just the right tool to help you
not say it."                   (New York Times, about Microsoft PowerPoint)

On Thu, Dec 31, 2020 at 10:47:36AM -0300, 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).
> 
> Adding to commitfest.

I reviewed the documentation to learn about the feature,
and fixed some typos.

+notpatch.

-- 
Justin

On Fri, Nov 12, 2021 at 02:57:57PM -0300, Alvaro Herrera wrote:
> Here's a new version.  Many of the old complaints have been fixed;

I should've replied to this newer message.

I also read through the code a bit and have some more language fixes, mostly.

I ran sqlsmith for a few hours with no apparent issue - good.

It seems like there's an opened question about how RULES should be handled?

-- 
Justin

On Sun, Nov 14, 2021 at 12:23 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2021-Nov-13, Daniel Westermann wrote:
> > /usr/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2  -I../../../src/include  -D_GNU_SOURCE
-I/usr/include/libxml2 -flto=thin -emit-llvm -c -o execMerge.bc execMerge.c 
> > execMerge.c:552:32: warning: if statement has empty body [-Wempty-body]
> >                                         RELKIND_PARTITIONED_TABLE);
> >                                                                   ^
> > execMerge.c:552:32: note: put the semicolon on a separate line to silence this warning
>
> Oh wow, this may be a pretty serious problem actually.  I think it
> represents a gap in testing.  Thanks for reporting.

Ah, thanks indeed.  It seems that I fat-fingered that semicolon in.
Though, it's not as serious as it would've been if I had instead
fat-fingered a `&& false` into that condition and not the semicolon.
;)

The only problem caused by the code block that follows the buggy if
statement unconditionally executing is wasted cycles.  Fortunately,
there's no correctness issue, because rootRelInfo is the same as the
input result relation in the cases where the latter is not partitioned
and there'd be no map to convert the tuple, so the block is basically
a no-op.   I was afraid about the case where the input relation is a
regular inheritance parent, though apparently we don't support MERGE
in that case to begin with.

--
Amit Langote
EDB: http://www.enterprisedb.com

Thanks everyone for the feedback.  I attach a version with the fixes
that were submitted, as well as some additional changes:

- I removed the restriction for tables inheritance and added the sample
  I showed to regression.

- I added DO NOTHING support to the WHERE MATCHED case; it previously
  only covered WHERE NOT MATCHED.

I was thinking earlier that it may be possible to clean up the
parse_merge.c code by using another RangeTblRef to process the data
source RTE.  Haven't tried yet.

This stuff is all in
https://github.com/alvherre/postgres/commits/merge-15

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor.  Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

On 2021-Nov-15, Alvaro Herrera wrote:

> Thanks everyone for the feedback.  I attach a version with the fixes
> that were submitted, as well as some additional changes:

Attachment failure.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)

Hi Álvaro,

On Tue, Nov 16, 2021 at 6:01 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2021-Nov-14, Amit Langote wrote:
> > The only problem caused by the code block that follows the buggy if
> > statement unconditionally executing is wasted cycles.  Fortunately,
> > there's no correctness issue, because rootRelInfo is the same as the
> > input result relation in the cases where the latter is not partitioned
> > and there'd be no map to convert the tuple, so the block is basically
> > a no-op.   I was afraid about the case where the input relation is a
> > regular inheritance parent, though apparently we don't support MERGE
> > in that case to begin with.
>
> Well, I noticed that if we simply remove the ERROR that prevents that
> case, it works fine as far as I can tell.

Thanks for looking into that.

>  Example (partly cribbed from
> the documentation):
>
> CREATE TABLE measurement (
>     city_id         int not null,
>     logdate         date not null,
>     peaktemp        int,
>     unitsales       int
> );
> CREATE TABLE measurement_y2006m02 (
>     CHECK ( logdate >= DATE '2006-02-01' AND logdate < DATE '2006-03-01' )
> ) INHERITS (measurement);
> CREATE TABLE measurement_y2006m03 (
>     CHECK ( logdate >= DATE '2006-03-01' AND logdate < DATE '2006-04-01' )
> ) INHERITS (measurement);
> CREATE TABLE measurement_y2007m01 (
>     filler          text,
>     peaktemp        int,
>     logdate         date not null,
>     city_id         int not null,
>     unitsales       int
>     CHECK ( logdate >= DATE '2007-01-01' AND logdate < DATE '2007-02-01')
> );
> ALTER TABLE measurement_y2007m01 DROP COLUMN filler;
> ALTER TABLE measurement_y2007m01 INHERIT measurement;
>
> CREATE OR REPLACE FUNCTION measurement_insert_trigger()
> RETURNS TRIGGER AS $$
> BEGIN
>     IF ( NEW.logdate >= DATE '2006-02-01' AND
>          NEW.logdate < DATE '2006-03-01' ) THEN
>         INSERT INTO measurement_y2006m02 VALUES (NEW.*);
>     ELSIF ( NEW.logdate >= DATE '2006-03-01' AND
>             NEW.logdate < DATE '2006-04-01' ) THEN
>         INSERT INTO measurement_y2006m03 VALUES (NEW.*);
>     ELSIF ( NEW.logdate >= DATE '2007-01-01' AND
>             NEW.logdate < DATE '2007-02-01' ) THEN
>         INSERT INTO measurement_y2007m01 (city_id, logdate, peaktemp, unitsales)
>             VALUES (NEW.*);
>     ELSE
>         RAISE EXCEPTION 'Date out of range.  Fix the measurement_insert_trigger() function!';
>     END IF;
>     RETURN NULL;
> END;
> $$ LANGUAGE plpgsql ;
> CREATE TRIGGER insert_measurement_trigger
>     BEFORE INSERT ON measurement
>     FOR EACH ROW EXECUTE PROCEDURE measurement_insert_trigger();
> INSERT INTO measurement VALUES (1, '2006-02-10', 35, 10);
> INSERT INTO measurement VALUES (1, '2006-02-16', 45, 20);
> INSERT INTO measurement VALUES (1, '2006-03-17', 25, 10);
> INSERT INTO measurement VALUES (1, '2006-03-27', 15, 40);
> INSERT INTO measurement VALUES (1, '2007-01-15', 10, 10);
> INSERT INTO measurement VALUES (1, '2007-01-17', 10, 10);
>
> alvherre=# select tableoid::regclass, * from measurement order by city_id, logdate;
>        tableoid       | city_id |  logdate   | peaktemp | unitsales
> ----------------------+---------+------------+----------+-----------
>  measurement_y2006m02 |       1 | 2006-02-10 |       35 |        10
>  measurement_y2006m02 |       1 | 2006-02-16 |       45 |        20
>  measurement_y2006m03 |       1 | 2006-03-17 |       25 |        10
>  measurement_y2006m03 |       1 | 2006-03-27 |       15 |        40
>  measurement_y2007m01 |       1 | 2007-01-15 |       10 |        10
>  measurement_y2007m01 |       1 | 2007-01-17 |       10 |        10
>
>
> CREATE TABLE new_measurement (LIKE measurement);
> INSERT INTO new_measurement VALUES (1, '2006-03-01', 20, 10);
> INSERT INTO new_measurement VALUES (1, '2006-02-16', 50, 10);
> INSERT INTO new_measurement VALUES (2, '2006-02-10', 20, 20);
> INSERT INTO new_measurement VALUES (1, '2006-03-27', NULL, NULL);
> INSERT INTO new_measurement VALUES (1, '2007-01-17', NULL, NULL);
> INSERT INTO new_measurement VALUES (1, '2007-01-15', 5, NULL);
> INSERT INTO new_measurement VALUES (1, '2007-01-16', 10, 10);
>
> MERGE into measurement m
>  USING new_measurement nm ON
>       (m.city_id = nm.city_id and m.logdate=nm.logdate)
> WHEN MATCHED AND nm.peaktemp IS NULL THEN DELETE
> WHEN MATCHED THEN UPDATE
>      SET peaktemp = greatest(m.peaktemp, nm.peaktemp),
>         unitsales = m.unitsales + coalesce(nm.unitsales, 0)
> WHEN NOT MATCHED THEN INSERT
>      (city_id, logdate, peaktemp, unitsales)
>    VALUES (city_id, logdate, peaktemp, unitsales);
>
> alvherre=# select tableoid::regclass, * from measurement order by city_id, logdate;
>        tableoid       | city_id |  logdate   | peaktemp | unitsales
> ----------------------+---------+------------+----------+-----------
>  measurement_y2006m02 |       1 | 2006-02-10 |       35 |        10
>  measurement_y2006m02 |       1 | 2006-02-16 |       50 |        30
>  measurement_y2006m03 |       1 | 2006-03-01 |       20 |        10
>  measurement_y2006m03 |       1 | 2006-03-17 |       25 |        10
>  measurement_y2007m01 |       1 | 2007-01-15 |       10 |        10
>  measurement_y2007m01 |       1 | 2007-01-16 |       10 |        10
>  measurement_y2006m02 |       2 | 2006-02-10 |       20 |        20
>
>
> Even the DELETE case worked correctly (I mean, it deletes from the right
> partition).
>
> So I'm considering adding this case to the regression tests and remove
> the limitation.  If anybody wants to try some more sophisticated
> examples, I'll welcome proposed additions to the regression tests.
>
> (Don't get me wrong -- I don't think this is terribly useful
> functionality.  I just think that if the code is already prepared to
> handle it, we may as well let it.)
>
> One thing I didn't quite like is that the count of affected tuples seems
> off, but IIRC that's already the case for trigger-redirected tuples in
> legacy-style partitioning, so I'm not too worried about that.

AFAICS, MERGE operating on an inheritance parent that is not
partitioned should work mostly the same as the case where it is
partitioned (good thing that it works at all without needing any
special code!), though only the INSERT actions would have to be
handled appropriately by the user using triggers and such.  And also I
guess any UPDATE actions that need to move rows between child tables
because that too involves tuple routing logic.  As long as we're clear
on that in the documentation, I don't see why this case should not be
covered in the initial version.

I thought for a second about the cases where child tables have columns
not present in the root parent mentioned in the command, but I guess
that possibility doesn't present problems given that the command
wouldn't be able to mention such columns to begin with; it can only
refer to the root parent's column which must be present in all of the
affected child tables.  In any case, I have a feeling that the planner
would catch any problematic cases if there're any while converting
MergeAction expressions into the individual child table layouts.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Wed, 22 Dec 2021 at 11:35, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
>
> On Mon, 15 Nov 2021 at 22:45, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2021-Nov-15, Alvaro Herrera wrote:
> >
> > > Thanks everyone for the feedback.  I attach a version with the fixes
> > > that were submitted, as well as some additional changes:
> >
> > Attachment failure.
>
> I rebased this, please check.
>
> And 2 patch-on-patches that resolve a few minor questions/docs wording.

Here is another patch-on-patch to fix a syntax error in the MERGE docs.

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

On Wed, Dec 22, 2021 at 11:35:56AM +0000, Simon Riggs wrote:
> On Mon, 15 Nov 2021 at 22:45, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2021-Nov-15, Alvaro Herrera wrote:
> >
> > > Thanks everyone for the feedback.  I attach a version with the fixes
> > > that were submitted, as well as some additional changes:
> >
> > Attachment failure.
> 
> I rebased this, please check.
> 

Hi,

I found two crashes, actually I found them on the original patch Álvaro
sent on november but just checked that those already exists.

I configured with:

CFLAGS="-ggdb -Og -g3 -fno-omit-frame-pointer" ./configure --prefix=/opt/var/pgdg/15/merge --enable-debug
--enable-depend--enable-cassert --with-llvm --enable-tap-tests --with-pgport=54315
 

And tested on the regression database.

Attached the SQL files for the crashes and its respective stacktraces.
FWIW, the second crash doesn't appear to be caused by the MERGE patch
but I cannot trigger it other way.


-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

On 1/5/22 02:53, Simon Riggs wrote:
> On Wed, 22 Dec 2021 at 11:35, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
>> On Mon, 15 Nov 2021 at 22:45, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>> On 2021-Nov-15, Alvaro Herrera wrote:
>>>
>>>> Thanks everyone for the feedback.  I attach a version with the fixes
>>>> that were submitted, as well as some additional changes:
>>> Attachment failure.
>> I rebased this, please check.
>>
>> And 2 patch-on-patches that resolve a few minor questions/docs wording.
> Here is another patch-on-patch to fix a syntax error in the MERGE docs.



The problem with patches like this is that they seriously screw up the
cfbot, which doesn't know about the previous patches you want this based
on top of. See <http://cfbot.cputube.org/patch_36_3408.log>


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2022-Jan-12, Andrew Dunstan wrote:

> The problem with patches like this is that they seriously screw up the
> cfbot, which doesn't know about the previous patches you want this based
> on top of. See <http://cfbot.cputube.org/patch_36_3408.log>

Here's a v5 that includes Simon's changes.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Apologies, there was a merge failure and I failed to notice.  Here's the
correct patch.

(This is also in github.com/alvherre/postgres/tree/merge-15)

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

Op 13-01-2022 om 13:43 schreef Alvaro Herrera:
> Apologies, there was a merge failure and I failed to notice.  Here's the
> correct patch.
> 
> (This is also in github.com/alvherre/postgres/tree/merge-15)

> [20220113/v6-0001-MERGE-SQL-Command-following-SQL-2016.patch]

Good morning,


I got into this crash (may be the same as Jaime's):

#-----
# bash

t1=table1
t2=table2

psql -qX << SQL
drop table if exists $t1 cascade;
drop table if exists $t2 cascade;
create table $t1 as select /*cast(id::text as jsonb) js,*/ id from 
generate_series(1,20) as f(id);
create table $t2 as select /*cast(id::text as jsonb) js,*/ id from 
generate_series(1,20) as f(id);
delete from $t1 where id % 2 = 1;
delete from $t2 where id % 2 = 0;
(           select 't1 - target', count(*) t1 from $t1
   union all select 't2 - source', count(*) t2 from $t2 ) order by 1;

merge into $t1 as t1 using $t2 as t2 on t1.id = t2.id
when not matched and (t2.id > 10) and (t1.id > 10)
                  then  do nothing
when not matched then  insert values ( id )
when matched     then  do nothing ;

(           select 't1 - target', count(*) t1 from $t1
   union all select 't2 - source', count(*) t2 from $t2 ) order by 1 ;

SQL
#-----

Referencing alias 't1' in the WHEN NOT MATCHED member seems what 
triggers his crash: when I remove the phrase 'and (t1.id > 10)', the 
statement finishes correctly.


And I don't know if it is related but if I use this phrase:

when not matched and (id > 10)

I get:

ERROR:  column "id" does not exist
LINE 2: when not matched and id > 0 -- (t2.id > 10) and (t1.id > 10)
                              ^
HINT:  There is a column named "id" in table "t1", but it cannot be 
referenced from this part of the query.

Is that hint correct? Seems a bit strange.


Thanks,

Erik Rijkers

Op 13-01-2022 om 13:43 schreef Alvaro Herrera:
> Apologies, there was a merge failure and I failed to notice.  Here's the
> correct patch.
> 
> (This is also in github.com/alvherre/postgres/tree/merge-15)

> [v6-0001-MERGE-SQL-Command-following-SQL-2016]

I read though the MERGE-docs; some typos:

'For example, given MERGE foo AS f'
'For example, given MERGE INTO foo AS f'

'that clause clause'
'that clause'

'This is same as'
'This is the same as'

'for certain type of action'
'for certain types of action'

'The MERGE allows the user'
'MERGE allows the user'
     (from the paragraph 'Read Committed Isolation Level'.  Likely 
copied from the paragraph above: 'The DELETE'; but there it refers to an 
earlier mention of DELETE.)


Erik Rijkers

On Fri, 14 Jan 2022 at 17:11, Erik Rijkers <er@xs4all.nl> wrote:
> I got into this crash (may be the same as Jaime's):
>
> #-----
> # bash
>
> t1=table1
> t2=table2
>
> psql -qX << SQL
> drop table if exists $t1 cascade;
> drop table if exists $t2 cascade;
> create table $t1 as select /*cast(id::text as jsonb) js,*/ id from
> generate_series(1,20) as f(id);
> create table $t2 as select /*cast(id::text as jsonb) js,*/ id from
> generate_series(1,20) as f(id);
> delete from $t1 where id % 2 = 1;
> delete from $t2 where id % 2 = 0;
> (           select 't1 - target', count(*) t1 from $t1
>   union all select 't2 - source', count(*) t2 from $t2 ) order by 1;
>
> merge into $t1 as t1 using $t2 as t2 on t1.id = t2.id
> when not matched and (t2.id > 10) and (t1.id > 10)
>                  then  do nothing
> when not matched then  insert values ( id )
> when matched     then  do nothing ;
>
> (           select 't1 - target', count(*) t1 from $t1
>   union all select 't2 - source', count(*) t2 from $t2 ) order by 1 ;
>
> SQL
> #-----
>
> Referencing alias 't1' in the WHEN NOT MATCHED member seems what
> triggers his crash: when I remove the phrase 'and (t1.id > 10)', the 
> statement finishes correctly.
>

The MERGE documentation says:

  A condition on a WHEN MATCHED clause can refer to columns in both the source
  and the target relations. A condition on a WHEN NOT MATCHED clause can only
  refer to columns from the source relation, since by definition there is no
  matching target row. Only the system attributes from the target table are
  accessible.

So for NOT MATCHED, we are expected not use the target table columns.

The code comes from execMerge.c says:

    /*
     * Make source tuple available to ExecQual and ExecProject. We don't need
     * the target tuple, since the WHEN quals and the targetlist can't refer to
     * the target columns.
     */
    econtext->ecxt_scantuple = NULL;
    econtext->ecxt_innertuple = slot;
    econtext->ecxt_outertuple = NULL;

It will set econtext->ecxt_scantuple to NULL, which leads the crash.

>
> And I don't know if it is related but if I use this phrase:
>
> when not matched and (id > 10)
>
> I get:
>
> ERROR:  column "id" does not exist
> LINE 2: when not matched and id > 0 -- (t2.id > 10) and (t1.id > 10)
>                              ^
> HINT:  There is a column named "id" in table "t1", but it cannot be
> referenced from this part of the query.
>
> Is that hint correct? Seems a bit strange.

I find the setNamespaceForMergeWhen() do not set the visiblity for RTE if
the command type is CMD_NOTHING, and both target and source tables can
be accessible for CMD_NOTHING.
Should we setNamespaceVisibilityForRTE() for CMD_NOTHING?  I try to set it
and it works as expected.  OTOH, the system attributes from target table
also cannot be accessible.  I'm not sure the v6 patch how to implement this
limitation.

Here is my modification:

diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c
index a3514053d4..415113157d 100644
--- a/src/backend/parser/parse_merge.c
+++ b/src/backend/parser/parse_merge.c
@@ -172,6 +172,18 @@ setNamespaceForMergeWhen(ParseState *pstate, MergeWhenClause *mergeWhenClause)
             break;
 
         case CMD_NOTHING:
+            /*
+             * We can use the WHEN condition for DO NOTHING, so we should
+             * make sure the relation can be visible for certain action.
+             */
+            if (mergeWhenClause->matched)
+                setNamespaceVisibilityForRTE(pstate->p_namespace,
+                                             targetRelRTE, true, true);
+            else
+                setNamespaceVisibilityForRTE(pstate->p_namespace,
+                                             targetRelRTE, false, false);
+            setNamespaceVisibilityForRTE(pstate->p_namespace,
+                                         sourceRelRTE, true, true);
             break;
         default:
             elog(ERROR, "unknown action in MERGE WHEN clause");

Thoughts?

Attached v7 patch. Fix some typos from [1].

[1] https://www.postgresql.org/message-id/7d7a5e1b-402a-5685-2c28-2f4e44ad186d%40xs4all.nl

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On 13.01.22 13:43, Alvaro Herrera wrote:
> Apologies, there was a merge failure and I failed to notice.  Here's the
> correct patch.

I looked through this a bit.  I wonder why there is a check for 
"unreachable WHEN clause".  I don't see this in the SQL standard.

On the other hand, there is a requirement in the SQL standard that the 
correlation names exposed by the source and target tables are different. 
  This is currently caught indirectly with a message like

ERROR:  table name "t" specified more than once

because of the way everything ends up in a join tree.  But that seems 
implementation-dependent, and a more explicit check might be better.

On 2022-Jan-12, Jaime Casanova wrote:

> I found two crashes, actually I found them on the original patch Álvaro
> sent on november but just checked that those already exists.
> 
> I configured with:
> 
> CFLAGS="-ggdb -Og -g3 -fno-omit-frame-pointer" ./configure --prefix=/opt/var/pgdg/15/merge --enable-debug
--enable-depend--enable-cassert --with-llvm --enable-tap-tests --with-pgport=54315
 
> 
> And tested on the regression database.
> 
> Attached the SQL files for the crashes and its respective stacktraces.
> FWIW, the second crash doesn't appear to be caused by the MERGE patch
> but I cannot trigger it other way.

Thanks for this!  The problem in the first crash was that when
partitioned tables are being used and the topmost one has a tuple
descriptor different from the partitions, we were doing the projection
to the partition's slot using the root's tupledesc and a targetlist
written for the root.  The reason this crashed in such ugly way is that
in this case the parent has 3 columns (2 dropped) while the partitions
only have one, so the projection was trying to write to an attribute
that didn't exist.

I fixed it by making all NOT MATCHED actions use the root table's
descriptor and slot.

This change fixes both your reported crashes.  I didn't look closely to
see if the second one is caused by exactly the same issue.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes...  Fixed.
        http://archives.postgresql.org/message-id/482D1632.8010507@sigaev.ru

Here's v8 of this patch.  I have fixed the problems pointed out by Jaime
and Erik, as well as incorporated Zhihong, Erik and Justin's
documentation fixes (typos and otherwise).

Not yet included: a fix for Peter's suggestion to raise a good error
when both tables use the same name.

Individual changes can also be seen in 
https://github.com/alvherre/postgres/commits/merge-15

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Porque Kim no hacía nada, pero, eso sí,
con extraordinario éxito" ("Kim", Kipling)

On Fri, 21 Jan 2022 at 05:06, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Here's v8 of this patch.  I have fixed the problems pointed out by Jaime
> and Erik, as well as incorporated Zhihong, Erik and Justin's
> documentation fixes (typos and otherwise).
>
> Not yet included: a fix for Peter's suggestion to raise a good error
> when both tables use the same name.
>
> Individual changes can also be seen in 
> https://github.com/alvherre/postgres/commits/merge-15

+               /*
+                * NOT MATCHED actions can't see target relation, but they can see
+                * source relation.
+                */
+               Assert(mergeWhenClause->commandType == CMD_INSERT ||
+                          mergeWhenClause->commandType == CMD_DELETE ||
+                          mergeWhenClause->commandType == CMD_NOTHING);
+               setNamespaceVisibilityForRTE(pstate->p_namespace,
+                                                                        targetRelRTE, false, false);
+               setNamespaceVisibilityForRTE(pstate->p_namespace,
+                                                                        sourceRelRTE, true, true);

Should we remove the CMD_DELETE from Assert(), since it will not happened
according to MERGE syntax?

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On 2022-Jan-21, Japin Li wrote:

> +               /*
> +                * NOT MATCHED actions can't see target relation, but they can see
> +                * source relation.
> +                */
> +               Assert(mergeWhenClause->commandType == CMD_INSERT ||
> +                          mergeWhenClause->commandType == CMD_DELETE ||
> +                          mergeWhenClause->commandType == CMD_NOTHING);
> +               setNamespaceVisibilityForRTE(pstate->p_namespace,
> +                                                                        targetRelRTE, false, false);
> +               setNamespaceVisibilityForRTE(pstate->p_namespace,
> +                                                                        sourceRelRTE, true, true);
> 
> Should we remove the CMD_DELETE from Assert(), since it will not happened
> according to MERGE syntax?

Absolutely --- silly copy&paste mistake.  Pushed fix.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Ni aún el genio muy grande llegaría muy lejos
si tuviera que sacarlo todo de su propio interior" (Goethe)

Here's MERGE v9.

The main difference in this version is that I have changed the way MERGE
is processed at parse analysis.  In previous versions, a fake JOIN was
constructed at that point; this was critiziced a long time ago ([1] is
an example, but IIRC there were others) and had not been addressed.

The new code is ~30 lines shorter.  I think those can be attributed to
comments explaining why the previous thing was so strange; with the new
code we don't need to explain as much.

In this rewrite, the two relations (target and source) are preserved and
passed down separately, and the JOIN is constructed in early optimizer.
This is what was suggested in those earlier sub-threads.

The new code looks a bit simpler, though some things are not completely
clear to me, such as why it works even though we have an empty
'joinaliasvars' for that join.  Another odd thing is the way we pass the
join condition from parse analysis to optimizer.  In this code we're
using the regular 'jointree' to store the source relation, and the
'quals' there refer to both the source relation and the target relation
-- which is not in the jointree.  Later at optimizer time we swap that
jointree out with the manufactured one; and the quals are moved one
layer down.  So for a brief time, the quals can refer to Vars that are
not part of the rangetable they are attached to.

I still have some things to clean up, but it seems worth sharing at
this point as the remaining items that I'm aware of are pretty minor.

[1] https://www.postgresql.org/message-id/CA%2BTgmoZj8fyJGAFxs%3D8Or9LeNyKe_xtoSN_zTeCSgoLrUye%3D9Q%40mail.gmail.com

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)

MERGE, v10.  I am much more comfortable with this version; I have
removed a bunch of temporary hacks and cleaned up the interactions with
table AM and executor, which is something that had been bothering me for
a while.  The complete set of changes can be seen in github,
https://github.com/alvherre/postgres/commits/merge-15

The most important one is probably
https://github.com/alvherre/postgres/commit/1bc92bd3f5af8b0406c5a633a68b2f76ba5a2616
where I introduced a new struct used at executor time to pass to
ExecUpdate et al where they can install the various bits of status info
on its way out; this allowed cleanup of the function signatures, as well
as TM_FailureData which was being modified in a somewhat strange way.

I am not aware of anything of significance in terms of remaining work
for this project.  The one thing I'm a bit bothered about is the fact
that we expose a lot of executor functions previously static.  I am now
wondering if it would be better to move the MERGE executor support
functions into nodeModifyTable.c, which I think would mean we would not
have to expose those function prototypes.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Hi,

On 2022-01-28 17:27:37 -0300, Alvaro Herrera wrote:
> MERGE, v10.  I am much more comfortable with this version; I have
> removed a bunch of temporary hacks and cleaned up the interactions with
> table AM and executor, which is something that had been bothering me for
> a while.  The complete set of changes can be seen in github,
> https://github.com/alvherre/postgres/commits/merge-15

Any chance you could split this into something more reviewable? The overall
diff stats are: 102 files changed, 8589 insertions(+), 234 deletions(-) thats
pretty hard to really review. Incremental commits don't realy help with that.


> I am not aware of anything of significance in terms of remaining work
> for this project.

One thing from skimming: There's not enough documentation about the approach
imo - it's a complicated enough feature to deserve more than the one paragraph
in src/backend/executor/README.


I'm still [1],[2] uncomfortable with execMerge.c kinda-but-not-really being an
executor node.


> The one thing I'm a bit bothered about is the fact that we expose a lot of
> executor functions previously static.  I am now wondering if it would be
> better to move the MERGE executor support functions into nodeModifyTable.c,
> which I think would mean we would not have to expose those function
> prototypes.

I'm worried about the complexity of nodeModifyTuple.c as is. Moving more code
in there does not seem like the right call to me - we should do the opposite
if anything.


A few inline comments below. No real review, just stuff noticed while skimming
to see where this is at.


Greetings,

Andres


[1] https://www.postgresql.org/message-id/20180405200003.gar3j26gsk32gqpe@alap3.anarazel.de
[2] https://www.postgresql.org/message-id/20180403021800.b5nsgiclzanobiup@alap3.anarazel.de


> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> index 1a9c1ac290..280ac40e63 100644
> --- a/src/backend/commands/trigger.c
> +++ b/src/backend/commands/trigger.c

This stuff seems like one candidate for splitting out.


> +    /*
> +     * We maintain separate transition tables for UPDATE/INSERT/DELETE since
> +     * MERGE can run all three actions in a single statement. Note that UPDATE
> +     * needs both old and new transition tables whereas INSERT needs only new,
> +     * and DELETE needs only old.
> +     */
> +
> +    /* "old" transition table for UPDATE, if any */
> +    Tuplestorestate *old_upd_tuplestore;
> +    /* "new" transition table for UPDATE, if any */
> +    Tuplestorestate *new_upd_tuplestore;
> +    /* "old" transition table for DELETE, if any */
> +    Tuplestorestate *old_del_tuplestore;
> +    /* "new" transition table INSERT, if any */
> +    Tuplestorestate *new_ins_tuplestore;
> +
>      TupleTableSlot *storeslot;    /* for converting to tuplestore's format */
>  };

Do we need to do something slightly clever about the memory limits for the
tuplestore? Each of them having a separate work_mem makes nodeModifyTable.c
one memory hungry node (the worst of all maybe?).




> +
> +/*
> + * Perform MERGE.
> + */
> +TupleTableSlot *
> +ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
> +          EState *estate, TupleTableSlot *slot)
> +{
> +    ExprContext *econtext = mtstate->ps.ps_ExprContext;
> +    ItemPointer tupleid;
> +    ItemPointerData tuple_ctid;
> +    bool        matched = false;
> +    Datum        datum;
> +    bool        isNull;
> +
> +    /*
> +     * Reset per-tuple memory context to free any expression evaluation
> +     * storage allocated in the previous cycle.
> +     */
> +    ResetExprContext(econtext);

Hasn't this already happened in ExecModifyTable()?


> +    /*
> +     * We run a JOIN between the target relation and the source relation to
> +     * find a set of candidate source rows that have a matching row in the
> +     * target table and a set of candidate source rows that do not have a
> +     * matching row in the target table. If the join returns a tuple with the
> +     * target relation's row-ID set, that implies that the join found a
> +     * matching row for the given source tuple. This case triggers the WHEN
> +     * MATCHED clause of the MERGE. Whereas a NULL in the target relation's
> +     * row-ID column indicates a NOT MATCHED case.
> +     */
> +    datum = ExecGetJunkAttribute(slot,
> +                                 resultRelInfo->ri_RowIdAttNo,
> +                                 &isNull);

Could this be put somewhere common with the equivalent call in
ExecModifyTable()?


> +     * A concurrent update can:
> +     *
> +     * 1. modify the target tuple so that it no longer satisfies the
> +     *    additional quals attached to the current WHEN MATCHED action
> +     *
> +     *    In this case, we are still dealing with a WHEN MATCHED case.
> +     *    In this case, we recheck the list of WHEN MATCHED actions from
> +     *    the start and choose the first one that satisfies the new target
> +     *    tuple.

Duplicated "in this case"


> +                case TM_Invisible:
> +
> +                    /*
> +                     * This state should never be reached since the underlying
> +                     * JOIN runs with a MVCC snapshot and EvalPlanQual runs
> +                     * with a dirty snapshot. So such a row should have never
> +                     * been returned for MERGE.
> +                     */
> +                    elog(ERROR, "unexpected invisible tuple");
> +                    break;

Seems like it was half duplicated at some point?


> +                case TM_Updated:
> +                case TM_Deleted:
> +
> +                    /*
> +                     * The target tuple was concurrently updated/deleted by
> +                     * some other transaction.
> +                     *
> +                     * If the current tuple is the last tuple in the update
> +                     * chain, then we know that the tuple was concurrently
> +                     * deleted. Just return and let the caller try NOT MATCHED
> +                     * actions.

Is it really correct to behave that way when using repeatable read /
serializable?


> +                /*
> +                 * Project the tuple.  In case of a partitioned table, the
> +                 * projection was already built to use the root's descriptor,
> +                 * so we don't need to map the tuple here.
> +                 */
> +                actionInfo.actionState = action;
> +                insert_slot = ExecProject(action->mas_proj);
> +
> +                (void) ExecInsert(mtstate, rootRelInfo,
> +                                  insert_slot, slot,
> +                                  estate, &actionInfo,
> +                                  mtstate->canSetTag);
> +                InstrCountFiltered1(&mtstate->ps, 1);

What happens if somebody concurrently inserts a conflicting row?

> --- a/src/include/executor/instrument.h
> +++ b/src/include/executor/instrument.h
> @@ -82,8 +82,11 @@ typedef struct Instrumentation
>      double        ntuples;        /* total tuples produced */
>      double        ntuples2;        /* secondary node-specific tuple counter */
>      double        nloops;            /* # of run cycles for this node */
> -    double        nfiltered1;        /* # of tuples removed by scanqual or joinqual */
> -    double        nfiltered2;        /* # of tuples removed by "other" quals */
> +    double        nfiltered1;        /* # tuples removed by scanqual or joinqual OR
> +                                 * # tuples inserted by MERGE */
> +    double        nfiltered2;        /* # tuples removed by "other" quals OR #
> +                                 * tuples updated by MERGE */
> +    double        nfiltered3;        /* # tuples deleted by MERGE */

It was a bad idea to have nfiltered1/2. Arriving at nfiltered3, it seems we
really should rename them...

On Fri, Jan 28, 2022 at 05:27:37PM -0300, Alvaro Herrera wrote:
> The one thing I'm a bit bothered about is the fact
> that we expose a lot of executor functions previously static.  I am now
> wondering if it would be better to move the MERGE executor support
> functions into nodeModifyTable.c, which I think would mean we would not
> have to expose those function prototypes.

It's probably a good idea.  

If you wanted to avoid bloating nodeModifyTable.c, maybe you could
#include "execMerge.c"

From commit message:

> MERGE does not yet support inheritance,

It does support it now, right ?

From merge.sgml:

"If you specify an update action...":
=> should say "If an update action is specified, ..."

s/an delete/a delete/

".. the WHEN clause is executed"
=> should say "the WHEN clause's action is executed" ?

" If a later WHEN clause of that kind is specified"
=> + COMMA

> --- a/doc/src/sgml/ref/allfiles.sgml
> +++ b/doc/src/sgml/ref/allfiles.sgml
> @@ -159,6 +159,7 @@ Complete list of usable sgml source files in this directory.
>  <!ENTITY load               SYSTEM "load.sgml">
>  <!ENTITY lock               SYSTEM "lock.sgml">
>  <!ENTITY move               SYSTEM "move.sgml">
> +<!ENTITY merge              SYSTEM "merge.sgml">
>  <!ENTITY notify             SYSTEM "notify.sgml">
>  <!ENTITY prepare            SYSTEM "prepare.sgml">
>  <!ENTITY prepareTransaction SYSTEM "prepare_transaction.sgml">

Looks like this is intended to be in alpha order.

> +  <refpurpose>insert, update, or delete rows of a table based upon source data</refpurpose>

based on ?

> --- a/src/backend/executor/README
> +++ b/src/backend/executor/README
> @@ -41,6 +41,19 @@ be used for other table types.)  For DELETE, the plan tree need only deliver
>  junk row-identity column(s), and the ModifyTable node visits each of those
>  rows and marks the row deleted.
>  
> +MERGE runs one generic plan that returns candidate change rows. Each row
> +consists of the output of the data-source table or query, plus CTID and
> +(if the target table is partitioned) TABLEOID junk columns.  If the target

s/partitioned/has child tables/ ?

>          case CMD_INSERT:
>          case CMD_DELETE:
>          case CMD_UPDATE:
> +        case CMD_MERGE:

Is it intended to stay in alpha order (?)

> +                case WCO_RLS_MERGE_UPDATE_CHECK:
> +                case WCO_RLS_MERGE_DELETE_CHECK:
> +                    if (wco->polname != NULL)
> +                        ereport(ERROR,
> +                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                                 errmsg("target row violates row-level security policy \"%s\" (USING expression) for
table\"%s\"",
 
> +                                        wco->polname, wco->relname)));

The parens around errcode are optional and IMO should be avoided for new code.

> +     * This duplicates much of the logic in ExecInitMerge(), so something
> +     * changes there, look here too.

so *if* ?

>          case T_InsertStmt:
>          case T_DeleteStmt:
>          case T_UpdateStmt:
> +        case T_MergeStmt:
>              lev = LOGSTMT_MOD;
>              break;

alphabetize (?)

> +    /* selcondition */
> +    "c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
> +    CppAsString2(RELKIND_PARTITIONED_TABLE) ") AND "
> +    "c.relhasrules = false AND "
> +    "(c.relhassubclass = false OR "
> +    " c.relkind = " CppAsString2(RELKIND_PARTITIONED_TABLE) ")",

relhassubclass=false is wrong now ?

> +-- prepare
> +RESET SESSION AUTHORIZATION;
> +DROP TABLE target, target2;
> +DROP TABLE source, source2;
> +DROP FUNCTION merge_trigfunc();
> +DROP USER merge_privs;
> +DROP USER merge_no_privs;

Why does it say "prepare" ?
I think it means to say "Clean up"

WRITE_READ_PARSE_PLAN_TREES exposes errors in make check:
+ERROR:  targetColnos does not match subplan target list

Have you looked at code coverage ?  I have an experimental patch to add that to
cirrus, and ran it with this patch; visible here:
https://cirrus-ci.com/task/6362512059793408

-- 
Justin

Op 28-01-2022 om 21:27 schreef Alvaro Herrera:
> MERGE, v10.  I am much more comfortable with this version; I have
> removed a bunch of temporary hacks and cleaned up the interactions with
> table AM and executor, which is something that had been bothering me for
> a while.  The complete set of changes can be seen in github,
> https://github.com/alvherre/postgres/commits/merge-15

> [v10-0001-MERGE-SQL-Command-following-SQL-2016.patch]

The patch doesnt apply smoothly:

patching file src/backend/tcop/pquery.c
patching file src/backend/tcop/utility.c
patching file src/backend/utils/adt/ruleutils.c
patching file src/bin/psql/tab-complete.c
Hunk #3 FAILED at 1714.
Hunk #4 succeeded at 3489 (offset 6 lines).
Hunk #5 succeeded at 3508 (offset 6 lines).
Hunk #6 succeeded at 3776 (offset 6 lines).
Hunk #7 succeeded at 3855 (offset 6 lines).
1 out of 7 hunks FAILED -- saving rejects to file 
src/bin/psql/tab-complete.c.rej
patching file src/include/commands/trigger.h
patching file src/include/executor/execMerge.h
patching file src/include/executor/instrument.h
patching file src/include/executor/nodeModifyTable.h



tab-complete.c.rej attached


Erik

MERGE v11.  This is only a trivial rebase where I include a fix from
Justin Pryzby for WRITE_READ_PARSE_PLAN_TREES correctness, and generated
without the tab-complete.c bogus hunk.  I'll be looking at the other
review comments next week.  Thanks!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"[PostgreSQL] is a great group; in my opinion it is THE best open source
development communities in existence anywhere."                (Lamar Owen)

On 2022-Jan-28, Justin Pryzby wrote:

> Have you looked at code coverage ?  I have an experimental patch to add that to
> cirrus, and ran it with this patch; visible here:
> https://cirrus-ci.com/task/6362512059793408

Ah, thanks, this is useful.  I think it is showing that the new code is
generally well covered, but there are some exceptions, particularly
ExecMergeMatched in some concurrent cases (TM_Deleted and
TM_SelfModified after table_tuple_lock -- those code pages have
user-facing errors but are not covered by any tests.)


How does this work?  I notice it is reporting for
src/bin/pg_upgrade/relfilenode.c, but that file is not changed by this
patch.

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

On Fri, Feb 11, 2022 at 03:21:43PM -0300, Alvaro Herrera wrote:
> On 2022-Jan-28, Justin Pryzby wrote:
> > Have you looked at code coverage ?  I have an experimental patch to add that to
> > cirrus, and ran it with this patch; visible here:
> > https://cirrus-ci.com/task/6362512059793408
> 
> Ah, thanks, this is useful.  I think it is showing that the new code is
> generally well covered, but there are some exceptions, particularly
> ExecMergeMatched in some concurrent cases (TM_Deleted and
> TM_SelfModified after table_tuple_lock -- those code pages have
> user-facing errors but are not covered by any tests.)
> 
> How does this work?  I notice it is reporting for
> src/bin/pg_upgrade/relfilenode.c, but that file is not changed by this
> patch.

Because I put your patch on top of some other branch with the CI coverage (and
other stuff).

It tries to only show files changed by the branch being checked:
https://github.com/justinpryzby/postgres/commit/d668142040031915

But it has to figure out where the branch "starts".  Which I did by looking at
"git diff --cherry-pick origin..."

Andres thinks that does the wrong thing if CI is run manually (not by CFBOT)
for patches against backbranches.  I'm not sure git diff --cherry-pick is
widely known/used, but I think using that relative to master may be good
enough.  

Ongoing discussion here.
https://www.postgresql.org/message-id/20220203035827.GG23027%40telsasoft.com

-- 
Justin

On 2022-Feb-11, Justin Pryzby wrote:

> Because I put your patch on top of some other branch with the CI coverage (and
> other stuff).

Ah, that makes sense.

> But it has to figure out where the branch "starts".  Which I did by looking at
> "git diff --cherry-pick origin..."
>
> I'm not sure git diff --cherry-pick is widely known/used, but I think
> using that relative to master may be good enough.  

I had never heard of git diff --cherry-pick, and the manpages I found
don't document it, so frankly I doubt it's known.  I still have no idea
what does it do.

I suppose there is an obvious reason why using git diff with
$(git merge-base ...) as one of the arguments doesn't work for these purposes.

> Andres thinks that does the wrong thing if CI is run manually (not by CFBOT)
> for patches against backbranches.

I wonder if it's sufficient to handle these things (coverage
specifically) for branch master only.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"

On Fri, Feb 11, 2022 at 05:25:49PM -0300, Alvaro Herrera wrote:
> > I'm not sure git diff --cherry-pick is widely known/used, but I think
> > using that relative to master may be good enough.  
> 
> I had never heard of git diff --cherry-pick, and the manpages I found
> don't document it, so frankly I doubt it's known.  I still have no idea
> what does it do.

See git-log(1)

       --cherry-pick
           Omit any commit that introduces the same change as another commit on the “other side” when the set of
commitsare limited with symmetric difference.
 

The "symmetric difference" is the triple-dot notation.

The last few years I've used this to check for missing bits in the draft
release notes.  (Actually, I tend to start my own list of features before
that).  It's doing a generic version of what git_changelog does.

https://www.postgresql.org/message-id/20210510144045.GC27406@telsasoft.com

> I suppose there is an obvious reason why using git diff with
> $(git merge-base ...) as one of the arguments doesn't work for these purposes.
> 
> > Andres thinks that does the wrong thing if CI is run manually (not by CFBOT)
> > for patches against backbranches.
> 
> I wonder if it's sufficient to handle these things (coverage
> specifically) for branch master only.

Or default to master, and maybe try to parse the commit message and pull out
Backpatch-through: NN.  It's intended to be machine-readable, after all.

Let's talk about it more on the/another thread :)

-- 
Justin

I attach v12 of MERGE.  Significant effort went into splitting
ExecUpdate and ExecDelete into parts that can be reused from the MERGE
routines in a way that doesn't involve jumping out in the middle of
TM_Result processing to merge-specific EvalPlanQual handling.  So in
terms of code flow, this is much cleaner.  It does mean that MERGE now
has to call three routines instead of just one, but that doesn't seem a
big deal.

So now the main ExecUpdate first has to call ExecUpdatePrologue, then
ExecUpdateAct, and complete with ExecUpdateEpilogue.  In the middle of
all this, it does its own thing to deal with foreign tables, and result
processing for regular tables including EvalPlanQual.  ExecUpdate has
been split similarly.

So MERGE now does these three things, and then its own result
processing.

Now, naively patching in that way results in absurd argument lists for
these routines, so I introduced a new ModifyTableContext struct to carry
the context for each operation.  I think this is good, but perhaps it
could stand some more improvement in terms of what goes in there and
what doesn't.  It took me a while to find an arrangement that really
worked.  (Initially I had resultRelInfo and the tuple slot and some
flags for DELETE, but it turned out to be better to keep them out.)

Regarding code arrangement: I decided that exporting all those
ModifyTable internal functions was a bad idea, so I made it all static
again.  I think the MERGE routines are merely additional ModifyTable
methods; trying to make it a separate executor node doesn't seem like
it'd be an improvement.  For now, I just made nodeModifyTable.c #include
execMerge.c, though I'm not sure if moving all the code into
nodeModifyTable.c would be a good idea, or whether we should create
separate files for each of those methods.  Generally speaking I'm not
enthusiastic about creating an exported API of these methods.  If we
think nodeModifyTable.c is too large, maybe we can split it in smaller
files and smash them together with #include "foo.c".  (If we do expose
it in some .h then the ModifyTableContext would have to be exported as
well, which doesn't sound too exciting.)


Sadly, because I've been spending a lot of time preparing for an
international move, I didn't have time to produce a split patch that
would first restructure nodeModifyTable.c making this more reviewable,
but I'll do that as soon as I'm able.  There is also a bit of a bug in a
corner case of an UPDATE that involves a cross-partition tuple migration
with another concurrent update.  I was unable to figure out how to fix
that, so I commented out the affected line from merge-update.spec.
Again, I'll get that fixed as soon as the dust has settled here.

There are some review points that are still unaddressed, such as
Andres' question of what happens in case of a concurrent INSERT.

Thanks

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"La fuerza no está en los medios físicos
sino que reside en una voluntad indomable" (Gandhi)

On 2022-Jan-28, Andres Freund wrote:

> Any chance you could split this into something more reviewable? The overall
> diff stats are: 102 files changed, 8589 insertions(+), 234 deletions(-) thats
> pretty hard to really review. Incremental commits don't realy help with that.

I'll work on splitting this next week.

> One thing from skimming: There's not enough documentation about the approach
> imo - it's a complicated enough feature to deserve more than the one paragraph
> in src/backend/executor/README.

Sure, I'll have a look at that.

> I'm still [1],[2] uncomfortable with execMerge.c kinda-but-not-really being an
> executor node.

I think we should make a decision on code arrangement here.  From my
perspective, MERGE isn't its own executor node; rather it's just another
"method" in ModifyTable.  Which makes sense, given that all it does is
call parts of INSERT, UPDATE, DELETE which are the other ModifyTable
methods.  Having a separate file doesn't strike me as great, but on the
other hand it's true that merely moving all the execMerge.c code into
nodeModifyTable.c makes the latter too large.  However I don't want to
create a .h file that means exposing all those internal functions to the
outside world.  My ideal would be to have each INSERT, UPDATE, DELETE,
MERGE as its own separate .c file, which would be #included from
nodeModifyTable.c.  We don't use that pattern anywhere though.  Any
opposition to that?  (The prototypes for each file would have to live in
nodeModifyTable.c itself.)


> > diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> > index 1a9c1ac290..280ac40e63 100644
> > --- a/src/backend/commands/trigger.c
> > +++ b/src/backend/commands/trigger.c
> 
> This stuff seems like one candidate for splitting out.

Yeah, I had done that.  It's now posted as 0001.

> > +    /*
> > +     * We maintain separate transition tables for UPDATE/INSERT/DELETE since
> > +     * MERGE can run all three actions in a single statement. Note that UPDATE
> > +     * needs both old and new transition tables whereas INSERT needs only new,
> > +     * and DELETE needs only old.
> > +     */
> > +
> > +    /* "old" transition table for UPDATE, if any */
> > +    Tuplestorestate *old_upd_tuplestore;
> > +    /* "new" transition table for UPDATE, if any */
> > +    Tuplestorestate *new_upd_tuplestore;
> > +    /* "old" transition table for DELETE, if any */
> > +    Tuplestorestate *old_del_tuplestore;
> > +    /* "new" transition table INSERT, if any */
> > +    Tuplestorestate *new_ins_tuplestore;
> > +
> >      TupleTableSlot *storeslot;    /* for converting to tuplestore's format */
> >  };
> 
> Do we need to do something slightly clever about the memory limits for the
> tuplestore? Each of them having a separate work_mem makes nodeModifyTable.c
> one memory hungry node (the worst of all maybe?).

Not sure how that would work.  The memory handling is inside
tuplestore.c IIRC ... I think that would require some largish
refactoring.  Merely dividing by four doesn't seem like a great answer
either.  Perhaps we could divide by two and be optimistic about it.

> > +                /*
> > +                 * Project the tuple.  In case of a partitioned table, the
> > +                 * projection was already built to use the root's descriptor,
> > +                 * so we don't need to map the tuple here.
> > +                 */
> > +                actionInfo.actionState = action;
> > +                insert_slot = ExecProject(action->mas_proj);
> > +
> > +                (void) ExecInsert(mtstate, rootRelInfo,
> > +                                  insert_slot, slot,
> > +                                  estate, &actionInfo,
> > +                                  mtstate->canSetTag);
> > +                InstrCountFiltered1(&mtstate->ps, 1);
> 
> What happens if somebody concurrently inserts a conflicting row?

An open question to which I don't have any good answer RN.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I think we should make a decision on code arrangement here.  From my
> perspective, MERGE isn't its own executor node; rather it's just another
> "method" in ModifyTable.  Which makes sense, given that all it does is
> call parts of INSERT, UPDATE, DELETE which are the other ModifyTable
> methods.  Having a separate file doesn't strike me as great, but on the
> other hand it's true that merely moving all the execMerge.c code into
> nodeModifyTable.c makes the latter too large.  However I don't want to
> create a .h file that means exposing all those internal functions to the
> outside world.  My ideal would be to have each INSERT, UPDATE, DELETE,
> MERGE as its own separate .c file, which would be #included from
> nodeModifyTable.c.  We don't use that pattern anywhere though.  Any
> opposition to that?  (The prototypes for each file would have to live in
> nodeModifyTable.c itself.)

Yeah, I don't like that.  The point of having separate .c files is that
you know that there's no interactions of non-global symbols across files.
This pattern breaks that assumption, making it harder to see what's
connected to what; and what's it buying exactly?  I'd rather keep all the
ModifyTable code in one .c file, even if that one is bigger than our
usual practice.

            regards, tom lane

> On 27 Feb 2022, at 18:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I'd rather keep all the ModifyTable code in one .c file, even if that one is
> bigger than our usual practice.

Agreed, I also prefer a (too) large file over a set of .c #include's.

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

On Sun, Feb 27, 2022 at 09:17:13PM +0100, Daniel Gustafsson wrote:
> > On 27 Feb 2022, at 18:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > I'd rather keep all the ModifyTable code in one .c file, even if that one is
> > bigger than our usual practice.
> 
> Agreed, I also prefer a (too) large file over a set of .c #include's.

+1.  Also, if a split is really needed isn't our usual approach to create some
*_private.h files so that third-party code is aware that this is in no way an
API meant for them?

I attach v13 here.  This version includes a 0002 that's does the split of nodeModifyTable.c routines, then 0003 implements MERGE on top of that.  0001 is as before.

I attach MERGE v14.  This includes a fix from Amit Langote for the
problem I described previously, with EvalPlanQual not working correctly.
(I had failed to short-circuit the cross-partition update correctly.)
Now the test case is enabled again, and it passes with the correct
output.

0001 is as before; the only change is that I've added a commit message.
Since this just moves some code around, I intend to get it pushed very
soon.

0002 is the ExecUpdate/ExecDelete split.  I cleaned up the struct
definitions a bit more, but it's pretty much as in the previous version.

0003 is the actual MERGE implementation.  Many previous review comments
have been handled, and several other things are cleaner than before.
I haven't made any changes for work_mem handling in ModifyTable's
transition tables, though, and haven't attempted to address concurrent
INSERT.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

On 2022-Mar-07, Zhihong Yu wrote:

> For v13-0003-MERGE-SQL-Command-following-SQL-2016.patch :
> 
> +    * Reset per-tuple memory context to free any expression evaluation
> +    * storage allocated in the previous cycle.
> +    */
> +   ResetExprContext(econtext);
> 
> Why is the memory cleanup done in the next cycle ? Can the cleanup be done
> at the end of the current cycle ?

I have removed that, because Andres had already pointed out that it was
redundant with the reset done in the caller.

> +            * XXX Should this explain why MERGE has the same logic as UPDATE?
> 
> I think explanation should be given.

Actually, the routine in question is only handling insert, not UPDATE,
so MERGE is not relevant to the function.  I have removed the comment.
This was probably a leftover from work prior to 86dc90056dfd; that
commit made it all irrelevant.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)

On Sun, 27 Feb 2022 at 17:35, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > > +                           /*
> > > +                            * Project the tuple.  In case of a partitioned table, the
> > > +                            * projection was already built to use the root's descriptor,
> > > +                            * so we don't need to map the tuple here.
> > > +                            */
> > > +                           actionInfo.actionState = action;
> > > +                           insert_slot = ExecProject(action->mas_proj);
> > > +
> > > +                           (void) ExecInsert(mtstate, rootRelInfo,
> > > +                                                             insert_slot, slot,
> > > +                                                             estate, &actionInfo,
> > > +                                                             mtstate->canSetTag);
> > > +                           InstrCountFiltered1(&mtstate->ps, 1);
> >
> > What happens if somebody concurrently inserts a conflicting row?
>
> An open question to which I don't have any good answer RN.

Duplicate rows should produce a uniqueness violation error in one of
the transactions, assuming there is a constraint to define the
conflict. Without such a constraint there is no conflict.

Concurrent inserts are checked by merge-insert-update.spec, which
correctly raises an ERROR in this case, as documented.

Various cases were not tested in the patch - additional patch
attached, but nothing surprising there.

ExecInsert() does not return from such an ERROR, so the code fragment
appears correct to me.

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

On 2022-Mar-09, Zhihong Yu wrote:

> Hi,
> For v14-0002-Split-ExecUpdate-and-ExecDelete-in-reusable-piec.patch :
> 
> +   TupleTableSlot *insertProjectedTuple;
> 
> With `insert` at the beginning of the variable name, someone may think it
> is a verb. How about naming the variable projectedTupleFromInsert (or
> something similar) ?

I went with projectedFromInsert.  I don't feel a need to call it a
"tuple" because it's already a TupleTableSlot * ...

> +       if (!ExecBRDeleteTriggers(context->estate, context->epqstate,
> +                                 resultRelInfo, tupleid, oldtuple,
> +                                 epqreturnslot))
> +           /* some trigger made the delete a no-op; let caller know */
> +           return false;
> 
> It seems you can directly return the value returned
> from ExecBRDeleteTriggers().

True, let's do that.

On 2022-Mar-10, Simon Riggs wrote:

> Various cases were not tested in the patch - additional patch
> attached, but nothing surprising there.

Thanks, incorporated here.

This is mostly just a rebase to keep cfbot happy.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

FYI I intend to get the ModifyTable split patch (0001+0003) pushed
hopefully on Tuesday or Wednesday next week, unless something really
ugly is found on it.

As for MERGE proper, I'm aiming at getting that one pushed on the week
starting March 21st, though of course I'll spend some more time on it
and will probably post further versions of it before that.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)

On Sat, Jan 29, 2022 at 12:03:35AM -0600, Justin Pryzby wrote:
> Note that MergeWhenClause and MergeAction have the node definition in a
> different order than the header, which is a bit confusing.

The .h files still order these fields differently than the other .h files, and
then the node funcs (at least MergeAction) also have a different order than the
.h files.

> diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
> index 1617702d9d..c8e8254b16 100644
> --- a/src/include/nodes/parsenodes.h
> +++ b/src/include/nodes/parsenodes.h
> @@ -117,7 +117,7 @@ typedef struct Query
> +typedef struct MergeWhenClause
> +{
> +    NodeTag        type;
> +    bool        matched;        /* true=MATCHED, false=NOT MATCHED */
> +    CmdType        commandType;    /* INSERT/UPDATE/DELETE/DO NOTHING */
> +    Node       *condition;        /* WHEN conditions (raw parser) */
> +    List       *targetList;        /* INSERT/UPDATE targetlist */
> +    /* the following members are only useful for INSERT action */
> +    List       *cols;            /* optional: names of the target columns */
> +    List       *values;            /* VALUES to INSERT, or NULL */
> +    OverridingKind override;    /* OVERRIDING clause */
> +} MergeWhenClause;

> +/*
> + * WHEN [NOT] MATCHED THEN action info
> + */
> +typedef struct MergeAction
> +{
> +    NodeTag        type;
> +    bool        matched;        /* true=MATCHED, false=NOT MATCHED */
> +    OverridingKind override;    /* OVERRIDING clause */
> +    Node       *qual;            /* transformed WHEN conditions */
> +    CmdType        commandType;    /* INSERT/UPDATE/DELETE/DO NOTHING */
> +    List       *targetList;        /* the target list (of TargetEntry) */
> +    List       *updateColnos;    /* target attribute numbers of an UPDATE */
> +} MergeAction;

> diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
> index d4f8455a2b..234a701045 100644
> --- a/src/backend/nodes/copyfuncs.c
> +++ b/src/backend/nodes/copyfuncs.c
> +static MergeAction *
> +_copyMergeAction(const MergeAction *from)
> +{
> +    MergeAction *newnode = makeNode(MergeAction);
> +
> +    COPY_SCALAR_FIELD(matched);
> +    COPY_SCALAR_FIELD(commandType);
> +    COPY_SCALAR_FIELD(override);
> +    COPY_NODE_FIELD(qual);
> +    COPY_NODE_FIELD(targetList);
> +    COPY_NODE_FIELD(updateColnos);

> +static MergeWhenClause *
> +_copyMergeWhenClause(const MergeWhenClause *from)
> +{
> +    MergeWhenClause *newnode = makeNode(MergeWhenClause);
> +
> +    COPY_SCALAR_FIELD(matched);
> +    COPY_SCALAR_FIELD(commandType);
> +    COPY_NODE_FIELD(condition);
> +    COPY_NODE_FIELD(targetList);
> +    COPY_NODE_FIELD(cols);
> +    COPY_NODE_FIELD(values);
> +    COPY_SCALAR_FIELD(override);
> +    return newnode;
> +}

> diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
> index f1002afe7a..5e1ff02a55 100644
> --- a/src/backend/nodes/equalfuncs.c
> +++ b/src/backend/nodes/equalfuncs.c
> @@ -841,6 +841,20 @@ _equalOnConflictExpr(const OnConflictExpr *a, const OnConflictExpr *b)
> +static bool
> +_equalMergeAction(const MergeAction *a, const MergeAction *b)
> +{
> +    COMPARE_SCALAR_FIELD(matched);
> +    COMPARE_SCALAR_FIELD(commandType);
> +    COMPARE_SCALAR_FIELD(override);
> +    COMPARE_NODE_FIELD(qual);
> +    COMPARE_NODE_FIELD(targetList);
> +    COMPARE_NODE_FIELD(updateColnos);

> +static bool
> +_equalMergeWhenClause(const MergeWhenClause *a, const MergeWhenClause *b)
> +{
> +    COMPARE_SCALAR_FIELD(matched);
> +    COMPARE_SCALAR_FIELD(commandType);
> +    COMPARE_NODE_FIELD(condition);
> +    COMPARE_NODE_FIELD(targetList);
> +    COMPARE_NODE_FIELD(cols);
> +    COMPARE_NODE_FIELD(values);
> +    COMPARE_SCALAR_FIELD(override);
> +
> +    return true;
> +}

> diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
> index 6bdad462c7..7549b27b39 100644
> --- a/src/backend/nodes/outfuncs.c
> +++ b/src/backend/nodes/outfuncs.c
> @@ -429,6 +429,21 @@ _outModifyTable(StringInfo str, const ModifyTable *node)
> +static void
> +_outMergeWhenClause(StringInfo str, const MergeWhenClause *node)
> +{
> +    WRITE_NODE_TYPE("MERGEWHENCLAUSE");
> +
> +    WRITE_BOOL_FIELD(matched);
> +    WRITE_ENUM_FIELD(commandType, CmdType);
> +    WRITE_NODE_FIELD(condition);
> +    WRITE_NODE_FIELD(targetList);
> +    WRITE_NODE_FIELD(cols);
> +    WRITE_NODE_FIELD(values);
> +    WRITE_ENUM_FIELD(override, OverridingKind);

> +static void
> +_outMergeAction(StringInfo str, const MergeAction *node)
> +{
> +    WRITE_NODE_TYPE("MERGEACTION");
> +
> +    WRITE_BOOL_FIELD(matched);
> +    WRITE_ENUM_FIELD(commandType, CmdType);
> +    WRITE_ENUM_FIELD(override, OverridingKind);
> +    WRITE_NODE_FIELD(qual);
> +    WRITE_NODE_FIELD(targetList);
> +    WRITE_NODE_FIELD(updateColnos);
> +}

> diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
> index 3f68f7c18d..d3ca5f0e45 100644
> --- a/src/backend/nodes/readfuncs.c
> +++ b/src/backend/nodes/readfuncs.c
> +static MergeAction *
> +_readMergeAction(void)
> +{
> +    READ_LOCALS(MergeAction);
> +
> +    READ_BOOL_FIELD(matched);
> +    READ_ENUM_FIELD(commandType, CmdType);
> +    READ_ENUM_FIELD(override, OverridingKind);
> +    READ_NODE_FIELD(qual);
> +    READ_NODE_FIELD(targetList);
> +    READ_NODE_FIELD(updateColnos);
> +
> +    READ_DONE();
> +}

> +static MergeWhenClause *
> +_readMergeWhenClause(void)
> +{
> +    READ_LOCALS(MergeWhenClause);
> +
> +    READ_BOOL_FIELD(matched);
> +    READ_ENUM_FIELD(commandType, CmdType);
> +    READ_NODE_FIELD(condition);
> +    READ_NODE_FIELD(targetList);
> +    READ_NODE_FIELD(cols);
> +    READ_NODE_FIELD(values);
> +    READ_ENUM_FIELD(override, OverridingKind);

On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> FYI I intend to get the ModifyTable split patch (0001+0003) pushed
> hopefully on Tuesday or Wednesday next week, unless something really
> ugly is found on it.

I looked at 0001 and found a few things that could perhaps be improved.

+   /* Slot containing tuple obtained from subplan, for RETURNING */
+   TupleTableSlot *planSlot;

I think planSlot is also used by an FDW to look up any FDW-specific
junk attributes that the FDW's scan method would have injected into
the slot, so maybe no need to specifically mention only RETURNING here
as what cares about it.

+   /*
+    * The tuple produced by EvalPlanQual to retry from, if a cross-partition
+    * UPDATE requires it
+    */
+   TupleTableSlot *retrySlot;

Maybe a bit long name, though how about calling this updateRetrySlot
to make it apparent that what is to be retried with the slot is the
whole UPDATE operation, not some sub-operation (like say the
cross-partition portion)?

+   /*
+    * The tuple projected by the INSERT's RETURNING clause, when doing a
+    * cross-partition UPDATE
+    */
+   TupleTableSlot *projectedFromInsert;

I'd have gone with cpUpdateReturningSlot here, because that is what it
looks to me to be.  The fact that it is ExecInsert() that computes the
RETURNING targetlist projection seems like an implementation detail.

I know you said you don't like having "Slot" in the name, but then we
also have retrySlot. :)

+/*
+ * Context struct containing output data specific to UPDATE operations.
+ */
+typedef struct UpdateContext
+{
+   bool        updateIndexes;  /* index update required? */
+   bool        crossPartUpdate;    /* was it a cross-partition update? */
+} UpdateContext;

I wonder if it isn't more readable to just have these two be the
output arguments of ExecUpdateAct()?

+redo_act:
+       /* XXX reinit ->crossPartUpdate here? */

Why not just make ExecUpdateAct() always set the flag, not only when a
cross-partition update does occur?

Will look some more in the morning tomorrow.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

On Sat, 12 Mar 2022 at 11:53, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Mar-09, Zhihong Yu wrote:
>
>> Hi,
>> For v14-0002-Split-ExecUpdate-and-ExecDelete-in-reusable-piec.patch :
>>
>> +   TupleTableSlot *insertProjectedTuple;
>>
>> With `insert` at the beginning of the variable name, someone may think it
>> is a verb. How about naming the variable projectedTupleFromInsert (or
>> something similar) ?
>
> I went with projectedFromInsert.  I don't feel a need to call it a
> "tuple" because it's already a TupleTableSlot * ...
>
>> +       if (!ExecBRDeleteTriggers(context->estate, context->epqstate,
>> +                                 resultRelInfo, tupleid, oldtuple,
>> +                                 epqreturnslot))
>> +           /* some trigger made the delete a no-op; let caller know */
>> +           return false;
>>
>> It seems you can directly return the value returned
>> from ExecBRDeleteTriggers().
>
> True, let's do that.
>
> On 2022-Mar-10, Simon Riggs wrote:
>
>> Various cases were not tested in the patch - additional patch
>> attached, but nothing surprising there.
>
> Thanks, incorporated here.
>
> This is mostly just a rebase to keep cfbot happy.

Hi, hackers

+       ar_delete_trig_tcs = mtstate->mt_transition_capture;
+       if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture &&
+               mtstate->mt_transition_capture->tcs_update_old_table)
+       {
+               ExecARUpdateTriggers(estate, resultRelInfo, tupleid, oldtuple,
+                                                        NULL, NULL, mtstate->mt_transition_capture);
+
+               /*
+                * We've already captured the NEW TABLE row, so make sure any AR
+                * DELETE trigger fired below doesn't capture it again.
+                */
+               ar_delete_trig_tcs = NULL;
+       }

Maybe we can use ar_delete_trig_tcs in if condition and ExecARUpdateTriggers() to make the code shorter.


+       /* "old" transition table for DELETE, if any */
+       Tuplestorestate *old_del_tuplestore;
+       /* "new" transition table INSERT, if any */
+       Tuplestorestate *new_ins_tuplestore;

Should we add "for" for transition INSERT comment?

+MERGE is a multiple-table, multiple-action command: it specifies a target
+table and a source relation, and can contain multiple WHEN MATCHED and
+WHEN NOT MATCHED clauses, each of which specifies one UPDATE, INSERT,
+UPDATE, or DO NOTHING actions.  The target table is modified by MERGE,
+and the source relation supplies additional data for the actions

I think the "source relation" is inaccurate.  How about using "data source" like document?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, Mar 14, 2022 at 11:36 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > FYI I intend to get the ModifyTable split patch (0001+0003) pushed
> > hopefully on Tuesday or Wednesday next week, unless something really
> > ugly is found on it.
>
> I looked at 0001 and found a few things that could perhaps be improved.
>
> +   /* Slot containing tuple obtained from subplan, for RETURNING */
> +   TupleTableSlot *planSlot;
>
> I think planSlot is also used by an FDW to look up any FDW-specific
> junk attributes that the FDW's scan method would have injected into
> the slot, so maybe no need to specifically mention only RETURNING here
> as what cares about it.
>
> +   /*
> +    * The tuple produced by EvalPlanQual to retry from, if a cross-partition
> +    * UPDATE requires it
> +    */
> +   TupleTableSlot *retrySlot;
>
> Maybe a bit long name, though how about calling this updateRetrySlot
> to make it apparent that what is to be retried with the slot is the
> whole UPDATE operation, not some sub-operation (like say the
> cross-partition portion)?

I think I meant to suggest cpUpdateRetrySlot, as in, the slot
populated by ExecCrossPartitionUpdate() with a tuple to retry the
UPDATE operation with, at least the portion of it that ExecUpdateAct()
is charge of.

> +   /*
> +    * The tuple projected by the INSERT's RETURNING clause, when doing a
> +    * cross-partition UPDATE
> +    */
> +   TupleTableSlot *projectedFromInsert;
>
> I'd have gone with cpUpdateReturningSlot here, because that is what it
> looks to me to be.  The fact that it is ExecInsert() that computes the
> RETURNING targetlist projection seems like an implementation detail.
>
> I know you said you don't like having "Slot" in the name, but then we
> also have retrySlot. :)
>
> +/*
> + * Context struct containing output data specific to UPDATE operations.
> + */
> +typedef struct UpdateContext
> +{
> +   bool        updateIndexes;  /* index update required? */
> +   bool        crossPartUpdate;    /* was it a cross-partition update? */
> +} UpdateContext;
>
> I wonder if it isn't more readable to just have these two be the
> output arguments of ExecUpdateAct()?
>
> +redo_act:
> +       /* XXX reinit ->crossPartUpdate here? */
>
> Why not just make ExecUpdateAct() always set the flag, not only when a
> cross-partition update does occur?
>
> Will look some more in the morning tomorrow.

Here are some more improvement suggestions:

+/*
+ * Context struct for a ModifyTable operation, containing basic execution state
+ * and some output data.
+ */

Does it make sense to expand "some output data", maybe as:

...and some output variables populated by the ExecUpdateAct() and
ExecDeleteAct() to report the result of their actions to ExecUpdate()
and ExecDelete() respectively.

+   /* output */
+   TM_FailureData tmfd;        /* stock failure data */

Maybe we should be more specific here, say:

Information about the changes that were found to be made concurrently
to a tuple being updated or deleted

+   LockTupleMode lockmode;     /* tuple lock mode */

Also here, say:

Lock mode to acquire on the latest tuple before performing EvalPlanQual() on it

+/*
+ * ExecDeleteAct -- subroutine for ExecDelete
+ *
+ * Actually delete the tuple, when operating on a plain or partitioned table.

+/*
+ * ExecUpdateAct -- subroutine for ExecUpdate
+ *
+ * Actually update the tuple, when operating on a plain or partitioned table.

Hmm, ExecDelete() and ExecUpdate() never see a partitioned table,
because both DELETE and UPDATE are always performed directly on leaf
partitions.   The (root) partitioned table only gets involved if the
UPDATE of a leaf partition tuple causes it to move to another
partition, that too only for applying tuple routing algorithm to find
the destination leaf partition.

So the following suffices, for ExecDeleteAct():

Actually delete the tuple from a plain table.

and for ExecUpdateAct(), maybe it makes sense to mention the
possibility of a cross-partition partition.

Actually update the tuple of a plain table.  If the table happens to
be a leaf partition and the update causes its partition constraint to
be violated, ExecCrossPartitionUpdate() is invoked to move the updated
tuple into the appropriate partition.

+ * Closing steps of tuple deletion; this invokes AFTER FOR EACH ROW triggers,
+ * including the UPDATE triggers if there was a cross-partition tuple move.

How about:

...including the UPDATE triggers if the ExecDelete() is being done as
part of a cross-partition tuple move.

+           /* and project to create a current version of the new tuple */

How about:

and project the new tuple to retry the UPDATE with

--
Amit Langote
EDB: http://www.enterprisedb.com

v16.

I spent some time experimenting if I could make ExecUpdateEpilogue /
ExecDeleteEpilogue return the RETURNING tuple, but this turns out not to
work very well.  Maybe there's a way to not make it look completely
silly, but at least what I tried ended up strictly worse.

I realized we weren't testing "do nothing" triggers with MERGE, and lo
and behold, the previous coding misbehaved.  I added test cases for it
and fixed the code.  I made a few other smallish changes, too cosmetic
to mention.

Thanks to Justin, Japin, Amit for their reviews.  Answers inline below.

On 2022-Mar-12, Justin Pryzby wrote:

> The .h files still order these fields differently than the other .h files, and
> then the node funcs (at least MergeAction) also have a different order than the
> .h files.

Fixed.  I also moved functions around to make everything consistent.  I
decided to put MergeWhereClause (raw parser output) followed by
MergeAction (output of parse analysis) and also put the fields of both
in sync and in consistent order everywhere.

On 2022-Mar-14, Amit Langote wrote:

> On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> I looked at 0001 and found a few things that could perhaps be improved.
> 
> +   /* Slot containing tuple obtained from subplan, for RETURNING */
> +   TupleTableSlot *planSlot;
> 
> I think planSlot is also used by an FDW to look up any FDW-specific
> junk attributes that the FDW's scan method would have injected into
> the slot, so maybe no need to specifically mention only RETURNING here
> as what cares about it.

I agree; I had already rewritten this comment on the plane.

> +   /*
> +    * The tuple produced by EvalPlanQual to retry from, if a cross-partition
> +    * UPDATE requires it
> +    */
> +   TupleTableSlot *retrySlot;
> 
> Maybe a bit long name, though how about calling this updateRetrySlot
> to make it apparent that what is to be retried with the slot is the
> whole UPDATE operation, not some sub-operation (like say the
> cross-partition portion)?

The reason I didn't want to have "Slot" in there is that it's a bit of
Hungarian notation; we already know that it's a slot by its type.  But
apparently we do this almost everywhere, so I just took your suggestions
on naming these two fields.

> +/*
> + * Context struct containing output data specific to UPDATE operations.
> + */
> +typedef struct UpdateContext
> +{
> +   bool        updateIndexes;  /* index update required? */
> +   bool        crossPartUpdate;    /* was it a cross-partition update? */
> +} UpdateContext;
> 
> I wonder if it isn't more readable to just have these two be the
> output arguments of ExecUpdateAct()?

I decided not to do that; I had to add "bool updated" back into that
struct once I noticed the problem with do-nothing triggers, as mentioned
above.  Then we're talking about three output argument and that gets too
ugly IMV, but I didn't really try to write the code.

> +redo_act:
> +       /* XXX reinit ->crossPartUpdate here? */
> 
> Why not just make ExecUpdateAct() always set the flag, not only when a
> cross-partition update does occur?

Yeah, I ended up doing that.  I didn't want to do that because it's
wasted work in the vast majority of cases; but if we want to avoid it,
we need to add a reset to false in the "goto redo_act" place, which
could cause future bugs of omission if we later add similar gotos.

> +/*
> + * Context struct for a ModifyTable operation, containing basic execution state
> + * and some output data.
> + */
> 
> Does it make sense to expand "some output data", maybe as:
> 
> ...and some output variables populated by the ExecUpdateAct() and
> ExecDeleteAct() to report the result of their actions to ExecUpdate()
> and ExecDelete() respectively.

Looks good, applied.

> +   /* output */
> +   TM_FailureData tmfd;        /* stock failure data */
> 
> Maybe we should be more specific here, say:
> 
> Information about the changes that were found to be made concurrently
> to a tuple being updated or deleted

WFM.  I changed "were found to be made" to "were made".

> +   LockTupleMode lockmode;     /* tuple lock mode */
> 
> Also here, say:
> 
> Lock mode to acquire on the latest tuple before performing EvalPlanQual() on it

WFM; I ditched the parens and used "latest tuple version".

> Hmm, ExecDelete() and ExecUpdate() never see a partitioned table,
> because [...]

> So the following suffices, for ExecDeleteAct():
> 
> Actually delete the tuple from a plain table.

WFM.

> and for ExecUpdateAct(), maybe it makes sense to mention the
> possibility of a cross-partition partition.
> 
> Actually update the tuple of a plain table.  If the table happens to
> be a leaf partition and the update causes its partition constraint to
> be violated, ExecCrossPartitionUpdate() is invoked to move the updated
> tuple into the appropriate partition.

I used this:
 * Actually update the tuple, when operating on a plain table.  If the
 * table is a partition, and the command was called referencing an ancestor
 * partitioned table, this routine is in charge of migrating the resulting
 * tuple to another partition.

I also changed the other part of this comment, to read:

 * The caller is in charge of keeping indexes current as necessary.  The
 * caller is also in charge of doing EvalPlanQual if the tuple is found to
 * be concurrently updated.  However, in case of a cross-partition update,
 * this routine does it.
 *
 * Caller is in charge of doing EvalPlanQual as necessary, and of keeping
 * indexes current for the update.

This is sufficient for 0001, but needs a bit of a tweak for MERGE, since
we return early in that case.  (And this is TBH *the* point I'm
uncomfortable with the MERGE patch: this bit becomes squishy.  I've been
wondering if it's possible to split ExecUpdateAct in even smaller pieces
in order to avoid this, but I just don't see how.)


> + * Closing steps of tuple deletion; this invokes AFTER FOR EACH ROW triggers,
> + * including the UPDATE triggers if there was a cross-partition tuple move.
> 
> How about:
> 
> ...including the UPDATE triggers if the ExecDelete() is being done as
> part of a cross-partition tuple move.

WFM, but I'd rather not assume that we're being called from ExecDelete.
Better to say "if the deletion is being done ..."  (It remains true with
MERGE as currently, but do we know that it will forever?)

> +           /* and project to create a current version of the new tuple */
> 
> How about:
> 
> and project the new tuple to retry the UPDATE with

WFM.

On 2022-Mar-14, Japin Li wrote:

> +       ar_delete_trig_tcs = mtstate->mt_transition_capture;
> +       if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture &&
> +               mtstate->mt_transition_capture->tcs_update_old_table)
> +       {
> +               ExecARUpdateTriggers(estate, resultRelInfo, tupleid, oldtuple,
> +                                                        NULL, NULL, mtstate->mt_transition_capture);
> +
> +               /*
> +                * We've already captured the NEW TABLE row, so make sure any AR
> +                * DELETE trigger fired below doesn't capture it again.
> +                */
> +               ar_delete_trig_tcs = NULL;
> +       }
> 
> Maybe we can use ar_delete_trig_tcs in if condition and
> ExecARUpdateTriggers() to make the code shorter.

... Well, the code inside the block is about UPDATE triggers, so it's a
nasty to use the variable whose name says it's for DELETE triggers.
That variable really is just a clever flag that indicates whether or not
to call the DELETE part.  It's a bit of strange coding, but ...


> +       /* "old" transition table for DELETE, if any */
> +       Tuplestorestate *old_del_tuplestore;
> +       /* "new" transition table INSERT, if any */
> +       Tuplestorestate *new_ins_tuplestore;
> 
> Should we add "for" for transition INSERT comment?

Absolutely.  Fixed.

> +MERGE is a multiple-table, multiple-action command: it specifies a target
> +table and a source relation, and can contain multiple WHEN MATCHED and
> +WHEN NOT MATCHED clauses, each of which specifies one UPDATE, INSERT,
> +UPDATE, or DO NOTHING actions.  The target table is modified by MERGE,
> +and the source relation supplies additional data for the actions
> 
> I think the "source relation" is inaccurate.  How about using "data
> source" like document?

I debated this with myself when I started using the term "source
relation" here.  The result of a query is also a relation, so this term
is not incorrect; in fact, the glossary entry for "relation" explains
this:
https://www.postgresql.org/docs/14/glossary.html#GLOSSARY-RELATION

It's true that it's not the typical use of the term in our codebase.

Overall, I'm -0 for changing this.  (If we decide to change it, there
are other places that would need to change as well.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Sorry, brown paperbag bug.  This fixes it.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

On Thu, 17 Mar 2022 at 03:18, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> v16.
> On 2022-Mar-14, Japin Li wrote:
>
>> +       ar_delete_trig_tcs = mtstate->mt_transition_capture;
>> +       if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture &&
>> +               mtstate->mt_transition_capture->tcs_update_old_table)
>> +       {
>> +               ExecARUpdateTriggers(estate, resultRelInfo, tupleid, oldtuple,
>> +                                                        NULL, NULL, mtstate->mt_transition_capture);
>> +
>> +               /*
>> +                * We've already captured the NEW TABLE row, so make sure any AR
>> +                * DELETE trigger fired below doesn't capture it again.
>> +                */
>> +               ar_delete_trig_tcs = NULL;
>> +       }
>>
>> Maybe we can use ar_delete_trig_tcs in if condition and
>> ExecARUpdateTriggers() to make the code shorter.
>
> ... Well, the code inside the block is about UPDATE triggers, so it's a
> nasty to use the variable whose name says it's for DELETE triggers.
> That variable really is just a clever flag that indicates whether or not
> to call the DELETE part.  It's a bit of strange coding, but ...
>
>> +MERGE is a multiple-table, multiple-action command: it specifies a target
>> +table and a source relation, and can contain multiple WHEN MATCHED and
>> +WHEN NOT MATCHED clauses, each of which specifies one UPDATE, INSERT,
>> +UPDATE, or DO NOTHING actions.  The target table is modified by MERGE,
>> +and the source relation supplies additional data for the actions
>>
>> I think the "source relation" is inaccurate.  How about using "data
>> source" like document?
>
> I debated this with myself when I started using the term "source
> relation" here.  The result of a query is also a relation, so this term
> is not incorrect; in fact, the glossary entry for "relation" explains
> this:
> https://www.postgresql.org/docs/14/glossary.html#GLOSSARY-RELATION
>
> It's true that it's not the typical use of the term in our codebase.
>
> Overall, I'm -0 for changing this.  (If we decide to change it, there
> are other places that would need to change as well.)

Thanks for the detailed explanation.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

0001 pushed.  Here's 0002 again for cfbot, with no changes other than
pgindent cleanup.

I'll see what to do about Instrumentation->nfiltered{1,2,3} that was
complained about by Andres upthread.  Maybe some additional macros will
help.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 2022-Mar-17, Alvaro Herrera wrote:

> I'll see what to do about Instrumentation->nfiltered{1,2,3} that was
> complained about by Andres upthread.  Maybe some additional macros will
> help.

This turns out not to be as simple as I expected, mostly because we want
to keep Instrumentation as a node-agnostic struct.  Things are already a
bit wonky with nfiltered/nfiltered2, and the addition of nfiltered3
makes things a lot uglier, and my impulse of using a union to separate
what is used for scans/joins vs. what is used for MERGE results in an
even more node-specific definition rather than the other way around.

Looking at the history I came across this older thread where this was
discussed
https://postgr.es/m/20180409215851.idwc75ct2bzi6tea@alvherre.pgsql
particularly this message from Robert,
https://www.postgresql.org/message-id/CA%2BTgmoaE3R6%3DV0G6zbht2L_LE%2BTsuYuSTPJXjLW%2B9_tpMGubZQ%40mail.gmail.com

I'll keep looking at this in the coming days, see if I can come up with
something sensible.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
Y una voz del caos me habló y me dijo
"Sonríe y sé feliz, podría ser peor".
Y sonreí. Y fui feliz.
Y fue peor.

On 17.03.22 12:31, Alvaro Herrera wrote:
> 0001 pushed.  Here's 0002 again for cfbot, with no changes other than
> pgindent cleanup.

I did a cursory read through and want to offer some trivial amendments 
in the attached patches.  The 0001 adds back various serial commas, the 
0002 is assorted other stuff.

One functional change I recommend is the tab completion of the MERGE 
target.  I think the filtering in Query_for_list_of_mergetargets is a 
bit too particular.  For example, if a table is a possible MERGE target, 
and then someone adds a rule, it will disappear from the completions, 
without explanation, which could be confusing.  I think we can be 
generous in what we accept and then let the actual parse analysis 
provide suitable error messages.  Also, consider forward-compatibility 
if support for further targets is added.  I would consider dropping 
Query_for_list_of_mergetargets and just using Query_for_list_of_updatables.

In any case, the min_server_version could be dropped.  That is usually 
only used if the query would fail in an older version, but not if the 
command being completed wouldn't work.  For example, we don't restrict 
in what versions you can complete partitioned indexes.

On 2022-Mar-10, Simon Riggs wrote:

> Duplicate rows should produce a uniqueness violation error in one of
> the transactions, assuming there is a constraint to define the
> conflict. Without such a constraint there is no conflict.
> 
> Concurrent inserts are checked by merge-insert-update.spec, which
> correctly raises an ERROR in this case, as documented.

Agreed -- I think this is reasonable.

> Various cases were not tested in the patch - additional patch
> attached, but nothing surprising there.

Thank you, I've included this in all versions of the patch since you
sent it.

> ExecInsert() does not return from such an ERROR, so the code fragment
> appears correct to me.

I think trying to deal with it in a different way (namely: suspend
processing the inserting WHERE NOT MATCHED clause and switch to
processing the row using WHERE MATCHED clauses) would require us use
speculative tokens, similar to the way INSERT ON CONFLICT does.  I'm not
sure we want to go there, but it seems okay to leave that for a later
patch.  Moreover, there would be no compatibility hit from doing so.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
 en duras y empinadas sillas    / desprovistas, por cierto
 de blandos atenuantes"                          (Patricio Vogel)

On 2022-Mar-18, Peter Eisentraut wrote:

> I did a cursory read through and want to offer some trivial amendments in
> the attached patches.  The 0001 adds back various serial commas, the 0002 is
> assorted other stuff.

Thanks.  I've merged these changes into this new v19.

Apart from rebasing on top of current master (which it had conflicts
with), I've also changed strategy for the tuple counters: Andres
complained that the addition of nfiltered3 was too messy, and I have to
agree.  After trying to deal with it in other ways, I ended up deciding
that it wasn't worth it, and just added ad-hoc tuple counters to
ModifyTableState.  We use this technique in other executor state nodes,
so it's not anything new.

> One functional change I recommend is the tab completion of the MERGE target.
> I think the filtering in Query_for_list_of_mergetargets is a bit too
> particular.  For example, if a table is a possible MERGE target, and then
> someone adds a rule, it will disappear from the completions, without
> explanation, which could be confusing.  I think we can be generous in what
> we accept and then let the actual parse analysis provide suitable error
> messages.  Also, consider forward-compatibility if support for further
> targets is added.  I would consider dropping Query_for_list_of_mergetargets
> and just using Query_for_list_of_updatables.

This argument makes sense to me, so I'll be doing that unless somebody
objects to the idea.

> In any case, the min_server_version could be dropped.  That is usually only
> used if the query would fail in an older version, but not if the command
> being completed wouldn't work.  For example, we don't restrict in what
> versions you can complete partitioned indexes.

Too true, too true ...

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"El sudor es la mejor cura para un pensamiento enfermo" (Bardia)

Here's v20, another rebase with some small changes:

- Changed psql tab-completion as suggested by Peter.  However, I didn't
  use Query_for_list_of_updatables because that includes relation types
  that aren't supported by MERGE, so I kept a separate query definition
  including only plain and partitioned tables.  While at it, fix its
  quoting: use of quote_ident() is no longer necessary.

- I changed the MergeWhenClause productions in the grammar to be more
  self-contained.  In the original, there was too much stuff being done
  in its caller production.  For example, the production for DELETE was
  returning NULL and the caller was creating the struct .. not sure why.

  Also, set all the fields in the struct, not just some.  This is not
  strictly necessary since the struct is zeroed by makeNode anyway, but
  this is out standard practice and looks more tidy -- this is what led
  me to discover the next point.

- I noticed that node MergeWhenClause had a member 'cols' to store
  INSERT columns, but at the same time it was ignoring the targetList
  member, which was documented to be used for this.  I don't know the
  reason for this separate member.  I removed 'cols' and made the code
  use 'targetList' instead.

- parse_clause.[ch] still contained some changes that were no longer
  necessary due to my earlier hacking on parse analysis.  Put them back
  as they were.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

One more rebase, fixing a trivial conflict with recent JSON work.

I intend to get this pushed after lunch.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 2022-Mar-28, Alvaro Herrera wrote:

> I intend to get this pushed after lunch.

Pushed, with one more change: fetching the tuple ID junk attribute in
ExecMerge was not necessary, since we already had done that in
ExecModifyTable.  We just needed to pass that down to ExecMerge, and
make sure to handle the case where there isn't one.

Early builfarm results: I need to fix role names in the new test.  On it
now.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Find a bug in a program, and fix it, and the program will work today.
Show the program how to find and fix a bug, and the program
will work forever" (Oliver Silfridge)

Should it use it ?

It occured to me to ask when reading Bruce's release notes, which say:
| [MERGE] is similar to INSERT ... ON CONFLICT but more batch-oriented.

Currently, INSERT *never* uses bistate - even INSERT SELECT.

INSERTing 10k tuples will dirty 10k buffers - not limited to the size of a
strategy/ring buffer.  Currently, MERGE will do the same.

I had a patch for INSERT last year.
https://commitfest.postgresql.org/35/2553/

On 2022-May-11, Justin Pryzby wrote:

> Should it use it ?
> 
> It occured to me to ask when reading Bruce's release notes, which say:
> | [MERGE] is similar to INSERT ... ON CONFLICT but more batch-oriented.
> 
> Currently, INSERT *never* uses bistate - even INSERT SELECT.
> 
> INSERTing 10k tuples will dirty 10k buffers - not limited to the size of a
> strategy/ring buffer.  Currently, MERGE will do the same.

As I understand it, the point of using a ring is to throttle performance
for bulk operations such as vacuum.  I'm not sure why we would want to
throttle either MERGE or INSERT; it seems to me that we should want them
to go as fast as possible.

If MERGE were to use a ring buffer, why wouldn't UPDATE do the same?
There are some comments to that effect in src/backend/buffer/README --
they do mention UPDATE/DELETE and not INSERT.  It seems to me that these
three commands (MERGE/UPDATE/DELETE) should be handled in similar ways,
so I don't think we need to consider lack of MERGE using a ring buffer
an open item for pg15.

COPY uses a ring buffer too.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

I suggest to reference the mvcc docs from the merge docs, or to make the merge
docs themselves include the referenced information.

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 341fea524a1..4446e1c484e 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -425,7 +425,7 @@ COMMIT;
    <para>
     <command>MERGE</command> allows the user to specify various
     combinations of <command>INSERT</command>, <command>UPDATE</command>
-    or <command>DELETE</command> subcommands. A <command>MERGE</command>
+    and <command>DELETE</command> subcommands. A <command>MERGE</command>
     command with both <command>INSERT</command> and <command>UPDATE</command>
     subcommands looks similar to <command>INSERT</command> with an
     <literal>ON CONFLICT DO UPDATE</literal> clause but does not
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index f68aa09736c..99dd5814f36 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -544,6 +544,7 @@ MERGE <replaceable class="parameter">total_count</replaceable>
    <command>UPDATE</command> if a concurrent <command>INSERT</command>
    occurs.  There are a variety of differences and restrictions between
    the two statement types and they are not interchangeable.
+   See <xref linkend="mvcc"/> for more information.
   </para>
  </refsect1>
 

Also, EXPLAIN output currently looks like this:

| Merge on ex_mtarget t (actual rows=0 loops=1)
|   Tuples Inserted: 0
|   Tuples Updated: 50
|   Tuples Deleted: 0
|   Tuples Skipped: 0

Should the "zero" rows be elided from the text output ?
And/or, should it use a more compact output format ?

There are two output formats already in use, so the options would look like
this:

   Tuples: Inserted: 1  Updated: 2  Deleted: 3  Skipped: 4
or
   Tuples: inserted=1 updated=2 deleted=3 skipped=4

Note double spaces and capitals.
That's separate from the question about eliding zeros.

On Wed, May 11, 2022 at 05:23:27PM +0200, Alvaro Herrera wrote:
> As I understand it, the point of using a ring is to throttle performance
> for bulk operations such as vacuum.  I'm not sure why we would want to
> throttle either MERGE or INSERT; it seems to me that we should want them
> to go as fast as possible.

The point of the ring/strategy buffer is to avoid a seq scan/vacuum/COPY
clobbering the entire buffer cache when processing a table larger than
shared_buffers.

It also makes backends doing bulk operations responsible for their own writes,
rather than leaving other backends to clean up all their dirty buffers.

> If MERGE were to use a ring buffer, why wouldn't UPDATE do the same?
> There are some comments to that effect in src/backend/buffer/README --
> they do mention UPDATE/DELETE and not INSERT.  It seems to me that these
> three commands (MERGE/UPDATE/DELETE) should be handled in similar ways,
> so I don't think we need to consider lack of MERGE using a ring buffer
> an open item for pg15.

I tend to think that a bulk commad either should or, at least, should be *able*
to use a ring buffer.  The README doesn't consider INSERTs, but that's the one
that we'd be most interested in.  I guess you're right that MERGE ought to do
what the existing commands are doing.

-- 
Justin

On Wed, May 11, 2022 at 11:33:50AM -0500, Justin Pryzby wrote:
> I suggest to reference the mvcc docs from the merge docs, or to make the merge
> docs themselves include the referenced information.

No comments here ?

> Also, EXPLAIN output currently looks like this:
> 
> | Merge on ex_mtarget t (actual rows=0 loops=1)
> |   Tuples Inserted: 0
> |   Tuples Updated: 50
> |   Tuples Deleted: 0
> |   Tuples Skipped: 0
> 
> Should the "zero" rows be elided from the text output ?
> And/or, should it use a more compact output format ?
> 
> There are two output formats already in use, so the options would look like
> this:
> 
>    Tuples: Inserted: 1  Updated: 2  Deleted: 3  Skipped: 4
> or
>    Tuples: inserted=1 updated=2 deleted=3 skipped=4
> 
> Note double spaces and capitals.
> That's separate from the question about eliding zeros.

Actually, the existing uses suggest that these *aren't* separate.

The cases where 0 output is elided (like Heap Blocks and Buffers) uses "=" and
not ":".  The cases using ":" always show all fields (Sort Method, Buckets,
Hits, Batches).  I'm not sure which is preferable for MERGE, but here's one
way.

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index b902ef30c87..491105263ff 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -4119,10 +4119,20 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors,
             skipped_path = total - insert_path - update_path - delete_path;
             Assert(skipped_path >= 0);
 
-            ExplainPropertyFloat("Tuples Inserted", NULL, insert_path, 0, es);
-            ExplainPropertyFloat("Tuples Updated", NULL, update_path, 0, es);
-            ExplainPropertyFloat("Tuples Deleted", NULL, delete_path, 0, es);
-            ExplainPropertyFloat("Tuples Skipped", NULL, skipped_path, 0, es);
+            if (es->format == EXPLAIN_FORMAT_TEXT)
+            {
+                ExplainIndentText(es);
+                appendStringInfo(es->str,
+                        "Tuples Inserted: %.0f  Updated: %.0f  Deleted: %.0f  Skipped: %.0f\n",
+                        insert_path, update_path, delete_path, skipped_path);
+            }
+            else
+            {
+                ExplainPropertyFloat("Tuples Inserted", NULL, insert_path, 0, es);
+                ExplainPropertyFloat("Tuples Updated", NULL, update_path, 0, es);
+                ExplainPropertyFloat("Tuples Deleted", NULL, delete_path, 0, es);
+                ExplainPropertyFloat("Tuples Skipped", NULL, skipped_path, 0, es);
+            }
         }
     }
 

On 2022-May-18, Justin Pryzby wrote:

> On Wed, May 11, 2022 at 11:33:50AM -0500, Justin Pryzby wrote:
> 
> > That's separate from the question about eliding zeros.
> 
> Actually, the existing uses suggest that these *aren't* separate.
> 
> The cases where 0 output is elided (like Heap Blocks and Buffers) uses "=" and
> not ":".  The cases using ":" always show all fields (Sort Method, Buckets,
> Hits, Batches).  I'm not sure which is preferable for MERGE, but here's one
> way.

I chose the other way :-)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/