Horton

Thread

Commits

  1. Doc fixes for MERGE statement

  2. MERGE docs adjustments

  3. Link to MVCC docs in MERGE docs

  4. Fix EXPLAIN MERGE output when no tuples are processed

  5. Make EXPLAIN MERGE output format more compact

  6. Add support for MERGE SQL command

  7. Split ExecUpdate and ExecDelete into reusable pieces

  8. Add API of sorts for transition table handling in trigger.c

  9. Represent Lists as expansible arrays, not chains of cons-cells.

  1. support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2020-12-31T13:47:36Z

    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

  2. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-01-08T19:22:00Z

    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

  3. Re: support for MERGE

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-01-10T01:44:29Z

    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

  4. Re: support for MERGE

    Vik Fearing <vik@postgresfriends.org> — 2021-01-10T15:36:49Z

    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

  5. Re: support for MERGE

    Simon Riggs <simon.riggs@enterprisedb.com> — 2021-01-13T10:20:43Z

    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/

  6. Re: support for MERGE

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-01-13T12:43:31Z

    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

  7. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-01-13T12:58:30Z

    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)

  8. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-01-18T15:21:31Z

    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

  9. Re: support for MERGE

    Robert Haas <robertmhaas@gmail.com> — 2021-01-18T16:08:53Z

    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

  10. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-01-18T16:48:18Z

    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

  11. Re: support for MERGE

    David Steele <david@pgmasters.net> — 2021-03-19T14:09:45Z

    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

  12. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-19T14:44:24Z

    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

  13. Re: support for MERGE

    David Steele <david@pgmasters.net> — 2021-03-19T14:53:53Z

    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

  14. Re: support for MERGE

    Bruce Momjian <bruce@momjian.us> — 2021-03-19T17:03:08Z

    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.

  15. Re: support for MERGE

    Magnus Hagander <magnus@hagander.net> — 2021-03-21T13:30:03Z

    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/

  16. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-21T13:57:16Z

    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

  17. Re: support for MERGE

    Magnus Hagander <magnus@hagander.net> — 2021-03-21T14:30:04Z

    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/

  18. Re: support for MERGE

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-01T10:43:59Z

    > 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/

  19. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-09-01T12:25:51Z

    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/

  20. Re: support for MERGE

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-01T12:58:33Z

    > 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/

  21. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-12T17:57:57Z

    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)

  22. Re: support for MERGE

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-11-12T18:05:12Z

    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

  23. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-12T18:13:58Z

    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/

  24. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2021-11-12T21:12:13Z

    On Fri, Nov 12, 2021 at 9:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
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.
>
> 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)
>
Hi,

+           skipped_path = total - insert_path - update_path - delete_path;

Should there be an assertion that skipped_path is not negative ?

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

cheers

  25. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-12T23:13:10Z

    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/

  26. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2021-11-13T01:40:57Z

    On Fri, Nov 12, 2021 at 3:13 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> 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,

+   resultRelInfo->ri_notMatchedMergeAction = NIL;

ri_notMatchedMergeAction -> ri_unmatchedMergeAction

+static void ExecMergeNotMatched(ModifyTableState *mtstate,

ExecMergeNotMatched -> ExecMergeUnmatched

+    *    In this case, we are still dealing with a WHEN MATCHED case.
+    *    In this case, we recheck the list of WHEN MATCHED actions from

It seems the comment can be simplified to:

+    *    In this case, since we are still dealing with a WHEN MATCHED case,
+    *    we recheck the list of WHEN MATCHED actions from

+                                * If we got no tuple, or the tuple we get
has

'get' appears in different tenses. Better use either 'get' or 'got' in both
places.

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

+       if (commandType == CMD_UPDATE && tuple_updated)

Since commandType can only be update or delete, it seems tuple_updated
and tuple_deleted can be consolidated into one boolean variable
(tuple_modified).
The above point is personal preference.

+        * We've activated one of the WHEN clauses, so we don't search
+        * further. This is required behaviour, not an optimization.
+        */
+       break;

We can directly return instead of break'ing.

+    * Similar logic appears in ExecInitPartitionInfo(), so if changing
+    * anything here, do so there too.

The above implies code dedup via refactoring - can be done in a separate
patch.

To be continued ...

  27. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2021-11-13T02:23:52Z

    On Fri, Nov 12, 2021 at 9:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
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.
>
> 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)
>
Hi,
This is continuation of review.

+                   elog(WARNING, "hoping nothing needed here");

the above warning can be dropped.

+   ExprState  *mas_whenqual;   /* WHEN [NOT] MATCHED AND conditions */

In my earlier comment I suggested changing notMatched to unmatched - I
didn't pay attention to the syntax.
That suggestion can be ignored.

Cheers

  28. Re: support for MERGE

    dwe@dbi-services.com — 2021-11-13T13:53:58Z

    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

  29. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-13T15:23:37Z

    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/

  30. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-13T15:24:22Z

    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/

  31. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-13T15:41:42Z

    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)

  32. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2021-11-13T15:50:40Z

    On Sat, Nov 13, 2021 at 7:41 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> 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?
>

The table_tuple_fetch_row_version() can be called before the next iteration
starts (followed by the adjustment of loop control variables).

Since you feel using goto is clearer, it is Okay to keep the current
formulation.

Cheers

  33. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2021-11-13T18:33:49Z

    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

  34. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2021-11-13T22:58:37Z

    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

  35. Re: support for MERGE

    Amit Langote <amitlangote09@gmail.com> — 2021-11-14T11:45:15Z

    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

  36. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-15T21:01:25Z

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

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

  37. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-15T22:44:21Z

    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)

  38. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-15T22:45:13Z

    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)

  39. Re: support for MERGE

    Amit Langote <amitlangote09@gmail.com> — 2021-11-16T02:38:03Z

    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

  40. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-16T16:25:37Z

    Hi Amit

On 2021-Nov-16, Amit Langote wrote:

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

Yeah, I think the reason it works so cleanly is that the code you and/or
Tom added to be able to get rid of inheritance_planner is superb,
including the new row identity stuff.  For the same reason, I suspect
that adding support for foreign tables should be reasonably simple --
just add explicit support for handling "wholerow" in a few places.  I
have not tried.

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

Right.  On the other hand, if we did have a problem with extra columns,
ISTM that would be on the user's head, not our responsibility.  In the
example I added, there is one child table with variant column layout; it
did require that the insertion trigger explicitly lists the columns in
the INSERT statement for that table, but otherwise it work correctly.

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

Yeah, AFAICS it worked fine for the case I tried.  Maybe there are more
elaborate ones that I didn't think of, of course.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

  41. Re: support for MERGE

    Simon Riggs <simon.riggs@enterprisedb.com> — 2021-12-22T11:35:56Z

    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.

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

  42. Re: support for MERGE

    Simon Riggs <simon.riggs@enterprisedb.com> — 2022-01-05T07:53:05Z

    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/

  43. Re: support for MERGE

    Jaime Casanova <jcasanov@systemguards.com.ec> — 2022-01-12T06:12:58Z

    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

  44. Re: support for MERGE

    Andrew Dunstan <andrew@dunslane.net> — 2022-01-12T19:54:13Z

    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

  45. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-13T09:51:33Z

    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/

  46. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-13T12:43:29Z

    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/

  47. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2022-01-14T00:15:45Z

    On Thu, Jan 13, 2022 at 4:43 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> 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)
>
> Hi,
For v6-0001-MERGE-SQL-Command-following-SQL-2016.patch :

+    * Either we were dealing with a NOT MATCHED tuple or
+    * ExecMergeNotMatched() returned "false", indicating the previously

I think there was a typo above: ExecMergeMatched() returned "false"
because ExecMergeMatched() is called above the comment.

Cheers

  48. Re: support for MERGE

    Erik Rijkers <er@xs4all.nl> — 2022-01-14T09:11:57Z

    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

  49. Re: support for MERGE

    Erik Rijkers <er@xs4all.nl> — 2022-01-14T10:40:00Z

    
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

  50. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-14T14:44:03Z

    Resending some language fixes to the public documentation.

  51. Re: support for MERGE

    Japin Li <japinli@hotmail.com> — 2022-01-16T18:04:03Z

    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.

  52. Re: support for MERGE

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-01-18T10:38:38Z

    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.

  53. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-20T21:02:22Z

    On 2022-Jan-17, Japin Li wrote:

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

Right.  So this was broken by the fact that I recently allowed MATCHED
actions to target DO NOTHING; previously, only NOT MATCHED actions could
do so.  So the bug was present, but it wasn't accessible.

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

I changed this block so that it depends on whether the clause is MATCHED
or NOT MATCHED, rather than the action.  I think it was pretty
nonsensical for it to be keyed on action type, and it made the code
needlessly longer.

Thank you!

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"All rings of power are equal,
But some rings of power are more equal than others."
                                 (George Orwell's The Lord of the Rings)

  54. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-20T21:02:43Z

    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

  55. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-20T21:06:59Z

    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)

  56. Re: support for MERGE

    Japin Li <japinli@hotmail.com> — 2022-01-21T06:47:25Z

    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.

  57. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-21T15:37:53Z

    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)

  58. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-27T15:15:27Z

    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)

  59. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-28T20:27:37Z

    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/

  60. Re: support for MERGE

    Andres Freund <andres@anarazel.de> — 2022-01-28T22:19:33Z

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

  61. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2022-01-28T22:32:37Z

    On Fri, Jan 28, 2022 at 2:19 PM Andres Freund <andres@anarazel.de> wrote:

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

Hi,
I think you meant nodeModifyTable.c.
And I agree with your comment (on not making nodeModifyTable.c bigger).

Cheers


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

  62. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-28T23:19:48Z

    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

  63. Re: support for MERGE

    Erik Rijkers <er@xs4all.nl> — 2022-01-28T23:33:21Z

    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

  64. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-01-29T21:49:37Z

    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)

  65. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-02-11T18:21:43Z

    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/

  66. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-02-11T18:51:50Z

    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

  67. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-02-11T20:25:49Z

    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"

  68. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-02-11T22:43:23Z

    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 commits are 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

  69. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-02-27T17:24:57Z

    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)

  70. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-02-27T17:35:13Z

    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)

  71. Re: support for MERGE

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-27T17:42:56Z

    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

  72. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2022-02-27T18:36:08Z

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

> 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)
>

Hi,

+   else if (node->operation == CMD_MERGE)
+   {
+       /* EXPLAIN ANALYZE display of actual outcome for each tuple
proposed */

I know the comment was copied. But it seems `proposed` should be
`processed`.

For ExecMergeNotMatched():

+   foreach(l, actionStates)
+   {
...
+       break;
+   }

If there is only one state in the List, maybe add an assertion for the
length of the actionStates List.

+   ModifyTableContext sc;

It seems the variable name can be more intuitive. How about naming it mtc
(or context, as what later code uses) ?

Cheers

  73. Re: support for MERGE

    Daniel Gustafsson <daniel@yesql.se> — 2022-02-27T20:17:13Z

    > 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/

  74. Re: support for MERGE

    Julien Rouhaud <rjuju123@gmail.com> — 2022-02-28T07:26:07Z

    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?

  75. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-07T19:59:09Z

    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.

  76. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-07T20:04:01Z

    On Mon, Mar 7, 2022, at 4:59 PM, Álvaro Herrera wrote:
> 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.

In 0002, I've opted to have two separate structs.  One is the ModifyTableContext, as before, but I've removed 'tupleid' and 'oldtuple' (the specification of the tuple to delete/update) because it makes ExecCrossPartitionUpdate cleaner if we pass them separately.  The second struct is UpdateContext, which is used inside ExecUpdate as output data from its subroutines.  This is also for the benefit of cross-partition updates.

I'm pretty happy with how this turned out; even without considering MERGE, it seems to me that ExecUpdate benefits from being shorter.

  77. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2022-03-07T22:53:15Z

    On Mon, Mar 7, 2022 at 12:04 PM Álvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> On Mon, Mar 7, 2022, at 4:59 PM, Álvaro Herrera wrote:
>
> 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.
>
>
> In 0002, I've opted to have two separate structs.  One is the
> ModifyTableContext, as before, but I've removed 'tupleid' and 'oldtuple'
> (the specification of the tuple to delete/update) because it makes
> ExecCrossPartitionUpdate cleaner if we pass them separately.  The second
> struct is UpdateContext, which is used inside ExecUpdate as output data
> from its subroutines.  This is also for the benefit of cross-partition
> updates.
>
> I'm pretty happy with how this turned out; even without considering MERGE,
> it seems to me that ExecUpdate benefits from being shorter.
>
Hi,
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 ?

+            * XXX Should this explain why MERGE has the same logic as
UPDATE?

I think explanation should be given.

Cheers

  78. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-09T17:38:03Z

    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/

  79. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-09T17:44:42Z

    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)

  80. Re: support for MERGE

    Zhihong Yu <zyu@yugabyte.com> — 2022-03-10T00:51:52Z

    On Wed, Mar 9, 2022 at 9:38 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> 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/


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) ?

+       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().

+       if (!ExecBRUpdateTriggers(context->estate, context->epqstate,
+                                 resultRelInfo, tupleid, oldtuple, slot))
+           /* some trigger made the update a no-op; let caller know */
+           return false;

Similar comment as above (the comment is good and should be kept).

Cheers

  81. Re: support for MERGE

    Simon Riggs <simon.riggs@enterprisedb.com> — 2022-03-10T15:48:54Z

    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/

  82. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-12T03:53:47Z

    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"

  83. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-12T15:36:39Z

    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)

  84. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-03-12T16:08:15Z

    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);

  85. Re: support for MERGE

    Amit Langote <amitlangote09@gmail.com> — 2022-03-14T14:36:07Z

    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

  86. Re: support for MERGE

    Japin Li <japinli@hotmail.com> — 2022-03-14T15:16:11Z

    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.

  87. Re: support for MERGE

    Amit Langote <amitlangote09@gmail.com> — 2022-03-15T09:40:05Z

    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

  88. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-16T19:18:48Z

    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/

  89. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-16T20:36:02Z

    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)

  90. Re: support for MERGE

    Japin Li <japinli@hotmail.com> — 2022-03-17T00:14:07Z

    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.

  91. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-17T11:31:16Z

    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/

  92. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-17T18:36:48Z

    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.

  93. Re: support for MERGE

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-18T11:05:03Z

    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.

  94. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-19T10:48:35Z

    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)

  95. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-20T18:40:06Z

    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)

  96. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-25T15:36:59Z

    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/

  97. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-28T11:00:33Z

    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/

  98. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-28T15:06:14Z

    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)

  99. Should use MERGE use BulkInsertState ?

    Justin Pryzby <pryzby@telsasoft.com> — 2022-05-11T12:51:48Z

    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/

  100. Re: Should use MERGE use BulkInsertState ?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-05-11T15:23:27Z

    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/

  101. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-05-11T16:33:50Z

    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.

  102. Re: Should use MERGE use BulkInsertState ?

    Justin Pryzby <pryzby@telsasoft.com> — 2022-05-11T16:48:25Z

    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

  103. Re: support for MERGE

    Laurenz Albe <laurenz.albe@cybertec.at> — 2022-05-12T06:57:45Z

    On Wed, 2022-05-11 at 11:33 -0500, Justin Pryzby wrote:
> 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.

+1 on one of the latter versions, I don't care which one.

Yours,
Laurenz Albe

  104. Re: Should use MERGE use BulkInsertState ?

    Robert Haas <robertmhaas@gmail.com> — 2022-05-13T16:20:43Z

    On Wed, May 11, 2022 at 12:48 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> 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.

Right. It's not just about how fast individual operations go: dirtying
all of shared_buffers is faster *for you* but you've imposed a
distributed cost on everyone else on the system, which isn't great.

That said, I'm not really sure what the proper handling is in this
specific case. I guess we should try our best to have it be symmetric
with other similar cases, but questions have been raised repeatedly
about how well the ring buffer stuff actually works.

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

  105. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-05-18T12:30:05Z

    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);
+			}
 		}
 	}

  106. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-05-18T16:36:01Z

    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/

  107. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-05-18T16:42:38Z

    On 2022-May-11, 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.
> 
> 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.

Reading the paragraph, I think it may be better to do it the other way
around: first mention that concurrency is described in the MVCC page,
then explain that INSERT ON CONFLICT also exists.  What do you think of
the attached?

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

  108. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-05-18T16:57:15Z

    On Wed, May 18, 2022 at 06:42:38PM +0200, Alvaro Herrera wrote:
> On 2022-May-11, 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.
> > 
> > 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.
> 
> Reading the paragraph, I think it may be better to do it the other way
> around: first mention that concurrency is described in the MVCC page,
> then explain that INSERT ON CONFLICT also exists.  What do you think of
> the attached?

Hmm, it seems odd to put "see also" first.

My original complaint is that 1) merge.sgml doesn't include the detailed
information; but 3) mentions it vaguely without linking to it: "There are a
variety of differences and restrictions between the two statement types and
they are not interchangeable."

I prefer my original, but the most important thing is to include the link at
*somewhere*.

-- 
Justin

  109. Re: support for MERGE

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-18T16:58:03Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I chose the other way :-)

I don't think the committed patch will behave nicely in edge cases:

+           if (es->format == EXPLAIN_FORMAT_TEXT && total > 0)
+           {
+               ExplainIndentText(es);
+               appendStringInfoString(es->str, "Tuples:");
+               if (insert_path > 0)
+                   appendStringInfo(es->str, " inserted=%.0f", insert_path);
+               if (update_path > 0)
+                   appendStringInfo(es->str, " updated=%.0f", update_path);
+               if (delete_path > 0)
+                   appendStringInfo(es->str, " deleted=%.0f", delete_path);
+               if (skipped_path > 0)
+                   appendStringInfo(es->str, " skipped=%.0f", skipped_path);
+               appendStringInfoChar(es->str, '\n');
+           }
+           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);
+           }

If it's text format and total is 0, I'd wish it to print nothing,
not to revert to the verbose format.

			regards, tom lane

  110. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-05-18T19:23:07Z

    On 2022-May-18, Tom Lane wrote:

> I don't think the committed patch will behave nicely in edge cases:
[...]
> If it's text format and total is 0, I'd wish it to print nothing,
> not to revert to the verbose format.

Oops, true.  Fixed.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Before you were born your parents weren't as boring as they are now. They
got that way paying your bills, cleaning up your room and listening to you
tell them how idealistic you are."  -- Charles J. Sykes' advice to teenagers

  111. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-05-31T20:06:17Z

    On Wed, May 18, 2022 at 11:57:15AM -0500, Justin Pryzby wrote:
> I prefer my original, but the most important thing is to include the link at
> *somewhere*.

Any other opinions ?

  112. Re: support for MERGE

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-06-01T10:56:55Z

    On 31.05.22 22:06, Justin Pryzby wrote:
> On Wed, May 18, 2022 at 11:57:15AM -0500, Justin Pryzby wrote:
>> I prefer my original, but the most important thing is to include the link at
>> *somewhere*.
> 
> Any other opinions ?

Álvaro's patch seems ok to me.  What are you concerned about?  Do you 
have an alternative suggestion?  It's a bit hard to figure out the 
specifics of what everyone is thinking.

  113. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-06-01T11:03:17Z

    On Wed, Jun 01, 2022 at 12:56:55PM +0200, Peter Eisentraut wrote:
> On 31.05.22 22:06, Justin Pryzby wrote:
> > On Wed, May 18, 2022 at 11:57:15AM -0500, Justin Pryzby wrote:
> > > I prefer my original, but the most important thing is to include the link at
> > > *somewhere*.
> > 
> > Any other opinions ?
> 
> Álvaro's patch seems ok to me.  What are you concerned about?  Do you have
> an alternative suggestion?  It's a bit hard to figure out the specifics of
> what everyone is thinking.

My original suggestion was here
https://www.postgresql.org/message-id/20220511163350.GL19626%40telsasoft.com

I prefer that way, with "See also" after the text that requires more
information.  But the most important thing is to include the link at all.

-- 
Justin

  114. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-06-01T15:28:10Z

    On 2022-Jun-01, Justin Pryzby wrote:

> I prefer that way, with "See also" after the text that requires more
> information.  But the most important thing is to include the link at all.

But it's not a "see also".  It's a link to the primary source of
concurrency information for MERGE.  The text that follows is not talking
about concurrency, it just indicates that you can do something different
but related by using a different command.

Re-reading the modified paragraph, I propose "see X for a thorough
explanation on the behavior of MERGE under concurrency".  However, in
the proposed patch the link goes to Chapter 13 "Concurrency Control",
and the explanation that we intend to link to is hidden in subsection
13.2.1 "Read Committed Isolation level".  So it appears that we do not
have any explanation on how MERGE behaves in other isolation levels.
That can't be good ...

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)

  115. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-06-14T09:27:06Z

    On Wed, Jun 1, 2022, at 5:28 PM, Alvaro Herrera wrote:
> Re-reading the modified paragraph, I propose "see X for a thorough
> explanation on the behavior of MERGE under concurrency".  However, in
> the proposed patch the link goes to Chapter 13 "Concurrency Control",
> and the explanation that we intend to link to is hidden in subsection
> 13.2.1 "Read Committed Isolation level".  So it appears that we do not
> have any explanation on how MERGE behaves in other isolation levels.
> That can't be good ...

OK, here's slightly updated wording for this, after reading the MVCC docs again. I didn't change the link to point specifically to read-committed, but I worded the reference to suggest that the referenced explanation is within each isolation level's subsection. In reality, there's not much to say about each specific command in REPEATABLE READ and SERIALIZABLE, but in the former I added MERGE to the list of commands.

I checked the pages for UPDATE and DELETE, to see if they had any explanation of concurrency, to use that as precedent. I couldn't find anything.

  116. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-06-15T15:36:47Z

    Pushed this.  I noticed that the paragraph that described MERGE in the
read-committed subsection had been put in the middle of some other paras
that describe interactions with other transactions, so I moved it up so
that it appears together with all the other paras that describe the
behavior of specific commands, which is where I believe it belongs.

Thanks!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)

  117. MERGE and parsing with prepared statements

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-14T16:26:18Z

    We've used INSERT ON CONFLICT for a few years (with partitions as the target).
That's also combined with prepared statements, for bulk loading.

I was looking to see if we should use MERGE (probably not, but looking anyway).
And came across this behavior.  I'm not sure if it's any issue.

CREATE TABLE CustomerAccount (CustomerId int, Balance float);

PREPARE p AS
MERGE INTO CustomerAccount CA
USING (SELECT $1 AS CustomerId, $2 AS TransactionValue) AS T
ON CA.CustomerId = T.CustomerId
WHEN NOT MATCHED THEN
  INSERT (CustomerId, Balance)
  VALUES (T.CustomerId, T.TransactionValue)
WHEN MATCHED THEN
  UPDATE SET Balance = Balance + TransactionValue;

ERROR:  operator does not exist: integer = text
LINE 3: ON CA.CustomerId = T.CustomerId

        postgres: pryzbyj postgres [local] PREPARE(+0x2337be) [0x56108322e7be]
        postgres: pryzbyj postgres [local] PREPARE(oper+0x198) [0x56108322f1fb]
        postgres: pryzbyj postgres [local] PREPARE(make_op+0x7e) [0x56108322f55a]
        postgres: pryzbyj postgres [local] PREPARE(+0x228f2b) [0x561083223f2b]
        postgres: pryzbyj postgres [local] PREPARE(+0x227aa9) [0x561083222aa9]
        postgres: pryzbyj postgres [local] PREPARE(transformExpr+0x1c) [0x5610832227f9]
        postgres: pryzbyj postgres [local] PREPARE(transformMergeStmt+0x339) [0x56108322d988]
        postgres: pryzbyj postgres [local] PREPARE(transformStmt+0x70) [0x5610831f4071]
        postgres: pryzbyj postgres [local] PREPARE(+0x1fa350) [0x5610831f5350]
        postgres: pryzbyj postgres [local] PREPARE(transformTopLevelStmt+0x11) [0x5610831f5385]
        postgres: pryzbyj postgres [local] PREPARE(parse_analyze_varparams+0x5b) [0x5610831f54f4]
        postgres: pryzbyj postgres [local] PREPARE(pg_analyze_and_rewrite_varparams+0x38) [0x5610834bcdfe]
        postgres: pryzbyj postgres [local] PREPARE(PrepareQuery+0xcc) [0x561083292155]
        postgres: pryzbyj postgres [local] PREPARE(standard_ProcessUtility+0x4ea) [0x5610834c31a0]

Why is $1 construed to be of type text ?

  118. Re: MERGE and parsing with prepared statements

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2022-07-15T09:25:35Z

    On Thu, 14 Jul 2022, 18:26 Justin Pryzby, <pryzby@telsasoft.com> wrote:
>
> We've used INSERT ON CONFLICT for a few years (with partitions as the target).
> That's also combined with prepared statements, for bulk loading.
>
> I was looking to see if we should use MERGE (probably not, but looking anyway).
> And came across this behavior.  I'm not sure if it's any issue.
>
> CREATE TABLE CustomerAccount (CustomerId int, Balance float);
>
> PREPARE p AS
> MERGE INTO CustomerAccount CA
> USING (SELECT $1 AS CustomerId, $2 AS TransactionValue) AS T
> ON CA.CustomerId = T.CustomerId
> WHEN NOT MATCHED THEN
>   INSERT (CustomerId, Balance)
>   VALUES (T.CustomerId, T.TransactionValue)
> WHEN MATCHED THEN
>   UPDATE SET Balance = Balance + TransactionValue;
>
> ERROR:  operator does not exist: integer = text
> LINE 3: ON CA.CustomerId = T.CustomerId
>
> Why is $1 construed to be of type text ?

The select statement that generates the row type of T `(select $1 CID,
$2 TxV) AS T` does not put type bounds on the input parameters, so it
remains `unknown` for the scope of that subselect. Once stored into
the row type, the type of that column defaults to text, as a row type
should not have 'unknown'-typed columns.

You'll see the same issue with other subselects that select input
parameters without casts, such as `select a from (select $1 a) A where
A.a = 1;`. It's a pre-existing issue that has popped up earlier, and I
think it's not something we've planned to fix in backbranches.

Kind regards,

Matthias van de Meent

  119. Re: MERGE and parsing with prepared statements

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-15T17:58:54Z

    On Fri, Jul 15, 2022 at 11:25:35AM +0200, Matthias van de Meent wrote:
> On Thu, 14 Jul 2022, 18:26 Justin Pryzby, <pryzby@telsasoft.com> wrote:
> >
> > Why is $1 construed to be of type text ?
> 
> The select statement that generates the row type of T `(select $1 CID,
> $2 TxV) AS T` does not put type bounds on the input parameters, so it
> remains `unknown` for the scope of that subselect. Once stored into
> the row type, the type of that column defaults to text, as a row type
> should not have 'unknown'-typed columns.

Thanks for looking into it.

I see now that the same thing can happen with "ON CONFLICT" if used with a
subselect.

PREPARE p AS INSERT INTO t SELECT a FROM (SELECT $1 AS a)a
     ON CONFLICT (i) DO UPDATE SET i=excluded.i;
ERROR:  column "i" is of type integer but expression is of type text

It seems a bit odd that it's impossible to use merge with prepared statements
without specifically casting the source types (which I did now to continue my
experiment).

-- 
Justin

  120. Re: MERGE and parsing with prepared statements

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-07-15T18:40:19Z

    On 2022-Jul-15, Justin Pryzby wrote:

> On Fri, Jul 15, 2022 at 11:25:35AM +0200, Matthias van de Meent wrote:

> Thanks for looking into it.

Definitely!  Thanks, Matthias.

> I see now that the same thing can happen with "ON CONFLICT" if used with a
> subselect.
> 
> PREPARE p AS INSERT INTO t SELECT a FROM (SELECT $1 AS a)a
>      ON CONFLICT (i) DO UPDATE SET i=excluded.i;
> ERROR:  column "i" is of type integer but expression is of type text

Right, I didn't think that MERGE was doing anything peculiar in this
respect.

> It seems a bit odd that it's impossible to use merge with prepared statements
> without specifically casting the source types (which I did now to continue my
> experiment).

I have no comments on this.  Maybe it can be improved, but I don't know
how.

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

  121. Re: MERGE and parsing with prepared statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-15T19:14:58Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Jul-15, Justin Pryzby wrote:
>> I see now that the same thing can happen with "ON CONFLICT" if used with a
>> subselect.
>> 
>> PREPARE p AS INSERT INTO t SELECT a FROM (SELECT $1 AS a)a
>> ON CONFLICT (i) DO UPDATE SET i=excluded.i;
>> ERROR:  column "i" is of type integer but expression is of type text

> Right, I didn't think that MERGE was doing anything peculiar in this
> respect.

Yeah.  The current theory about this is that if we haven't assigned a
type to an unknown-type parameter (or literal) that is an output
column of a sub-SELECT, we will as a rule force it to text.
That MERGE USING clause is a sub-SELECT, so that rule applies.

There is a hoary old exception to that rule, which is that if you
write INSERT INTO tab SELECT ..., $1, ...
we will figure out the type of the column of "tab" that $1 is going
into, and force $1 to that type instead of text.  It looks like this
also works in INSERT ... VALUES.  You could make a case that MERGE
should be equally smart, but it's not clear to me that the info is
available sufficiently close by to make it reasonable to do that.
It looks like the MERGE syntax has a couple of levels of indirection,
which'd probably be enough to put the kibosh on that idea -- in
particular, AFAICS there might not be a unique target column
corresponding to a given data_source column.

			regards, tom lane

  122. Re: MERGE and parsing with prepared statements

    David G. Johnston <david.g.johnston@gmail.com> — 2022-07-15T19:17:51Z

    On Fri, Jul 15, 2022 at 11:40 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> On 2022-Jul-15, Justin Pryzby wrote:
>
> > It seems a bit odd that it's impossible to use merge with prepared
> statements
> > without specifically casting the source types (which I did now to
> continue my
> > experiment).
>
> I have no comments on this.  Maybe it can be improved, but I don't know
> how.
>
>
Not tested, but the example prepare command fails to make use of the
optional data types specification.  Using that should remove the need to
cast the parameter placeholder within the query itself.

That said, in theory the INSERT specification of the MERGE could be used to
either resolve unknowns or even forcibly convert the data types of the
relation produced by the USING clause to match the actual types required
for the INSERT (since that will happen at insert time anyway).  This would
make the UPDATE behavior slightly different than a top-level UPDATE command
though, since that does not have the same context information.

David J.

  123. Re: MERGE and parsing with prepared statements

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-15T19:40:14Z

    On Fri, Jul 15, 2022 at 12:17:51PM -0700, David G. Johnston wrote:
> On Fri, Jul 15, 2022 at 11:40 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2022-Jul-15, Justin Pryzby wrote:
> >
> > > It seems a bit odd that it's impossible to use merge with prepared statements
> > > without specifically casting the source types (which I did now to continue my
> > > experiment).
> >
> > I have no comments on this.  Maybe it can be improved, but I don't know
> > how.
>
> Not tested, but the example prepare command fails to make use of the
> optional data types specification.  Using that should remove the need to
> cast the parameter placeholder within the query itself.

What optional data type specification ?

> That said, in theory the INSERT specification of the MERGE could be used to
> either resolve unknowns or even forcibly convert the data types of the
> relation produced by the USING clause to match the actual types required
> for the INSERT (since that will happen at insert time anyway).

Yeah.  I hadn't looked before, but just noticed this:

https://www.postgresql.org/docs/devel/sql-merge.html
| If the expression for any column is not of the correct data type, automatic type conversion will be attempted.

That appears to be copied from the INSERT page.
What does that mean, if not that data types will be resolved as needed ?

Note that if I add casts to the "ON" condition, MERGE complains about the
INSERT VALUES.

PREPARE p AS
MERGE INTO CustomerAccount CA
USING (SELECT $1 AS CustomerId, $2 AS TransactionValue) AS T
ON CA.CustomerId = T.CustomerId::int
WHEN NOT MATCHED THEN
  INSERT (CustomerId, Balance)
  VALUES (T.CustomerId, T.TransactionValue)
WHEN MATCHED THEN
  UPDATE SET Balance = Balance + TransactionValue;

ERROR:  column "customerid" is of type integer but expression is of type text
LINE 7:   VALUES (T.CustomerId, T.TransactionValue)

        postgres: pryzbyj postgres [local] PREPARE(transformAssignedExpr+0x3b6) [0x5605f9699e8e]
        postgres: pryzbyj postgres [local] PREPARE(transformInsertRow+0x2ce) [0x5605f9653a47]
        postgres: pryzbyj postgres [local] PREPARE(transformMergeStmt+0x7ec) [0x5605f968fe3b]
        postgres: pryzbyj postgres [local] PREPARE(transformStmt+0x70) [0x5605f9656071]
        postgres: pryzbyj postgres [local] PREPARE(+0x1fa350) [0x5605f9657350]
        postgres: pryzbyj postgres [local] PREPARE(transformTopLevelStmt+0x11) [0x5605f9657385]
        postgres: pryzbyj postgres [local] PREPARE(parse_analyze_varparams+0x5b) [0x5605f96574f4]
        postgres: pryzbyj postgres [local] PREPARE(pg_analyze_and_rewrite_varparams+0x38) [0x5605f991edfe]
        postgres: pryzbyj postgres [local] PREPARE(PrepareQuery+0xcc) [0x5605f96f4155]
        postgres: pryzbyj postgres [local] PREPARE(standard_ProcessUtility+0x4ea) [0x5605f99251a0]
        postgres: pryzbyj postgres [local] PREPARE(ProcessUtility+0xdb) [0x5605f992587e]

-- 
Justin

  124. Re: MERGE and parsing with prepared statements

    David G. Johnston <david.g.johnston@gmail.com> — 2022-07-15T19:53:59Z

    On Fri, Jul 15, 2022 at 12:40 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

> On Fri, Jul 15, 2022 at 12:17:51PM -0700, David G. Johnston wrote:
> > On Fri, Jul 15, 2022 at 11:40 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> > > On 2022-Jul-15, Justin Pryzby wrote:
> > >
> > > > It seems a bit odd that it's impossible to use merge with prepared
> statements
> > > > without specifically casting the source types (which I did now to
> continue my
> > > > experiment).
> > >
> > > I have no comments on this.  Maybe it can be improved, but I don't know
> > > how.
> >
> > Not tested, but the example prepare command fails to make use of the
> > optional data types specification.  Using that should remove the need to
> > cast the parameter placeholder within the query itself.
>
> What optional data type specification ?
>

The one documented here:

https://www.postgresql.org/docs/current/sql-prepare.html

PREPARE name [ ( data_type [, ...] ) ] AS statement

David J.

  125. Re: MERGE and parsing with prepared statements

    David G. Johnston <david.g.johnston@gmail.com> — 2022-07-15T19:59:34Z

    On Fri, Jul 15, 2022 at 12:40 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

>
> That appears to be copied from the INSERT page.
> What does that mean, if not that data types will be resolved as needed ?
>

Yep, and the system needs to resolve the type at a point where there is no
contextual information and so it chooses text.


> Note that if I add casts to the "ON" condition, MERGE complains about the
> INSERT VALUES.
>
> PREPARE p AS
> MERGE INTO CustomerAccount CA
> USING (SELECT $1 AS CustomerId, $2 AS TransactionValue) AS T
> ON CA.CustomerId = T.CustomerId::int
> WHEN NOT MATCHED THEN
>   INSERT (CustomerId, Balance)
>   VALUES (T.CustomerId, T.TransactionValue)
> WHEN MATCHED THEN
>   UPDATE SET Balance = Balance + TransactionValue;
>
> ERROR:  column "customerid" is of type integer but expression is of type
> text
> LINE 7:   VALUES (T.CustomerId, T.TransactionValue)
>
>
Noted.  Not surprised.  That error was always present, it's just that the
join happens first.  Since your fix narrowly targeted the join this error
remained to be discovered.

David J.

  126. Re: MERGE and parsing with prepared statements

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-15T20:43:41Z

    On Fri, Jul 15, 2022 at 12:59:34PM -0700, David G. Johnston wrote:
> On Fri, Jul 15, 2022 at 12:40 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> 
> >
> | If the expression for any column is not of the correct data type, automatic type conversion will be attempted.
> > That appears to be copied from the INSERT page.
> > What does that mean, if not that data types will be resolved as needed ?
>
> Yep, and the system needs to resolve the type at a point where there is no
> contextual information and so it chooses text.

I don't know if that's a viable interpretation.  The parser "resolved the type"
by assuming a default, which caused an error before finishing parsing.

In the case of INSERT, "conversion will be attempted" means it looks for a cast
from the source type to the target type, and its "automatic type conversion"
will fail if (for example) you try to insert a timestamp into an int.

In the case of MERGE, that same sentence evidently means that it assumes a
default source type (rather than looking for a cast) and then fails if it
doesn't exactly match the target type.

Should that sentence be removed from MERGE ?

-- 
Justin

  127. Re: MERGE and parsing with prepared statements

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-18T13:20:00Z

    On Fri, Jul 15, 2022 at 03:43:41PM -0500, Justin Pryzby wrote:
> Should that sentence be removed from MERGE ?

Also, I think these examples should be more similar.

doc/src/sgml/ref/merge.sgml

> <programlisting>
> MERGE INTO CustomerAccount CA
> USING RecentTransactions T
> ON T.CustomerId = CA.CustomerId
> WHEN MATCHED THEN
>   UPDATE SET Balance = Balance + TransactionValue
> WHEN NOT MATCHED THEN
>   INSERT (CustomerId, Balance)
>   VALUES (T.CustomerId, T.TransactionValue);
> </programlisting>
>   </para>
> 
>   <para>
>    Notice that this would be exactly equivalent to the following
>    statement because the <literal>MATCHED</literal> result does not change
>    during execution.
> 
> <programlisting>
> MERGE INTO CustomerAccount CA
> USING (Select CustomerId, TransactionValue From RecentTransactions) AS T
> ON CA.CustomerId = T.CustomerId
> WHEN NOT MATCHED THEN
>   INSERT (CustomerId, Balance)
>   VALUES (T.CustomerId, T.TransactionValue)
> WHEN MATCHED THEN
>   UPDATE SET Balance = Balance + TransactionValue;
> </programlisting>
>   </para>

The "ON" lines can be the same.
The "MATCHED" can be in the same order.

-- 
Justin

  128. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-01T14:52:57Z

    On Tue, Jun 14, 2022 at 11:27:06AM +0200, Álvaro Herrera wrote:
> @@ -448,9 +448,9 @@ COMMIT;
>      and execute the first one that succeeds.
>      If <command>MERGE</command> attempts an <command>INSERT</command>
>      and a unique index is present and a duplicate row is concurrently
> -    inserted, then a uniqueness violation is raised.
> -    <command>MERGE</command> does not attempt to avoid the
> -    error by executing an <command>UPDATE</command>.
> +    inserted, then a uniqueness violation error is raised;
> +    <command>MERGE</command> does not attempt to avoid such
> +    errors by evaluating <literal>MATCHED</literal> conditions.

This was a portion of a chang that was committed as ffffeebf2.

But I don't understand why this changed from "does not attempt to avoid the
error by executing an <command>UPDATE</command>." to "...by evaluating
<literal>MATCHED</literal> conditions."

Maybe it means to say "..by re-starting evaluation of match conditions".

Sorry to re-raise this 6 weeks later..

-- 
Justin

  129. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-08-01T15:30:30Z

    On 2022-Aug-01, Justin Pryzby wrote:

> On Tue, Jun 14, 2022 at 11:27:06AM +0200, Álvaro Herrera wrote:
> > @@ -448,9 +448,9 @@ COMMIT;
> >      and execute the first one that succeeds.
> >      If <command>MERGE</command> attempts an <command>INSERT</command>
> >      and a unique index is present and a duplicate row is concurrently
> > -    inserted, then a uniqueness violation is raised.
> > -    <command>MERGE</command> does not attempt to avoid the
> > -    error by executing an <command>UPDATE</command>.
> > +    inserted, then a uniqueness violation error is raised;
> > +    <command>MERGE</command> does not attempt to avoid such
> > +    errors by evaluating <literal>MATCHED</literal> conditions.
> 
> This was a portion of a chang that was committed as ffffeebf2.
> 
> But I don't understand why this changed from "does not attempt to avoid the
> error by executing an <command>UPDATE</command>." to "...by evaluating
> <literal>MATCHED</literal> conditions."
> 
> Maybe it means to say "..by re-starting evaluation of match conditions".

Yeah, my thought there is that it may also be possible that the action
that would run if the conditions are re-run is a DELETE or a WHEN
MATCHED THEN DO NOTHING; so saying "by executing an UPDATE" it leaves
out those possibilities.  IOW if we're evaluating NOT MATCHED INSERT and
we find a duplicate, we do not go back to MATCHED.  We have this comment
in ExecMerge:

     * ExecMergeMatched takes care of following the update chain and
     * re-finding the qualifying WHEN MATCHED action, as long as the updated
     * target tuple still satisfies the join quals, i.e., it remains a WHEN
     * MATCHED case. If the tuple gets deleted or the join quals fail, it
     * returns and we try ExecMergeNotMatched. Given that ExecMergeMatched
     * always make progress by following the update chain and we never switch
     * from ExecMergeNotMatched to ExecMergeMatched, there is no risk of a
     * livelock.

(Another change there is the period to semicolon.  I did that to make it
clear that the last phrase applies to only that part and not phrases
earlier in the same paragraph.)

Your proposed rewording might be a clearer way to express the same idea.

Any other opinions?

> Sorry to re-raise this 6 weeks later..

No worries.  As the Zen of Python says, now is better than never.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"All rings of power are equal,
But some rings of power are more equal than others."
                                 (George Orwell's The Lord of the Rings)

  130. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-08-09T09:48:23Z

    On 2022-Aug-01, Álvaro Herrera wrote:

> > >      If <command>MERGE</command> attempts an <command>INSERT</command>
> > >      and a unique index is present and a duplicate row is concurrently
> > > +    inserted, then a uniqueness violation error is raised;
> > > +    <command>MERGE</command> does not attempt to avoid such
> > > +    errors by evaluating <literal>MATCHED</literal> conditions.
> > 
> > This was a portion of a chang that was committed as ffffeebf2.
> > 
> > But I don't understand why this changed from "does not attempt to avoid the
> > error by executing an <command>UPDATE</command>." to "...by evaluating
> > <literal>MATCHED</literal> conditions."
> > 
> > Maybe it means to say "..by re-starting evaluation of match conditions".
> 
> Yeah, my thought there is that it may also be possible that the action
> that would run if the conditions are re-run is a DELETE or a WHEN
> MATCHED THEN DO NOTHING; so saying "by executing an UPDATE" it leaves
> out those possibilities.  IOW if we're evaluating NOT MATCHED INSERT and
> we find a duplicate, we do not go back to MATCHED.

So I propose to leave it as

           If <command>MERGE</command> attempts an <command>INSERT</command>
           and a unique index is present and a duplicate row is concurrently
           inserted, then a uniqueness violation error is raised;
           <command>MERGE</command> does not attempt to avoid such
           errors by restarting evaluation of <literal>MATCHED</literal>
	   conditions.

(Is "re-starting" better than "restarting"?)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Voy a acabar con todos los humanos / con los humanos yo acabaré
voy a acabar con todos (bis) / con todos los humanos acabaré ¡acabaré! (Bender)

  131. Re: support for MERGE

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-09T12:26:22Z

    On Tue, Aug 09, 2022 at 11:48:23AM +0200, Álvaro Herrera wrote:
> On 2022-Aug-01, Álvaro Herrera wrote:
> 
> > > >      If <command>MERGE</command> attempts an <command>INSERT</command>
> > > >      and a unique index is present and a duplicate row is concurrently
> > > > +    inserted, then a uniqueness violation error is raised;
> > > > +    <command>MERGE</command> does not attempt to avoid such
> > > > +    errors by evaluating <literal>MATCHED</literal> conditions.
> > > 
> > > This was a portion of a chang that was committed as ffffeebf2.
> > > 
> > > But I don't understand why this changed from "does not attempt to avoid the
> > > error by executing an <command>UPDATE</command>." to "...by evaluating
> > > <literal>MATCHED</literal> conditions."
> > > 
> > > Maybe it means to say "..by re-starting evaluation of match conditions".
> > 
> > Yeah, my thought there is that it may also be possible that the action
> > that would run if the conditions are re-run is a DELETE or a WHEN
> > MATCHED THEN DO NOTHING; so saying "by executing an UPDATE" it leaves
> > out those possibilities.  IOW if we're evaluating NOT MATCHED INSERT and
> > we find a duplicate, we do not go back to MATCHED.
> 
> So I propose to leave it as
> 
>            If <command>MERGE</command> attempts an <command>INSERT</command>
>            and a unique index is present and a duplicate row is concurrently
>            inserted, then a uniqueness violation error is raised;
>            <command>MERGE</command> does not attempt to avoid such
>            errors by restarting evaluation of <literal>MATCHED</literal>
> 	   conditions.

I think by "leave it as" you mean "change it to".
(Meaning, without referencing UPDATE).

> (Is "re-starting" better than "restarting"?)

"re-starting" doesn't currently existing in the docs, so I guess not.
You could also say "starting from scratch the evaluation of MATCHED conditions".

Note that I proposed two other changes in the other thread ("MERGE and parsing
with prepared statements").

  - remove the sentence with "automatic type conversion will be attempted";
  - make examples more similar to emphasize their differences;

-- 
Justin

  132. Re: MERGE and parsing with prepared statements

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-08-12T11:20:22Z

    On 2022-Jul-15, Justin Pryzby wrote:

> Should that sentence be removed from MERGE ?

Removed

On 2022-Jul-18, Justin Pryzby wrote:

> On Fri, Jul 15, 2022 at 03:43:41PM -0500, Justin Pryzby wrote:
> > Should that sentence be removed from MERGE ?
> 
> Also, I think these examples should be more similar.

Agreed, done.

On 2022-Aug-09, Justin Pryzby wrote:

> On Tue, Aug 09, 2022 at 11:48:23AM +0200, Álvaro Herrera wrote:

> > So I propose to leave it as
> > 
> >            If <command>MERGE</command> attempts an <command>INSERT</command>
> >            and a unique index is present and a duplicate row is concurrently
> >            inserted, then a uniqueness violation error is raised;
> >            <command>MERGE</command> does not attempt to avoid such
> >            errors by restarting evaluation of <literal>MATCHED</literal>
> > 	   conditions.
> 
> I think by "leave it as" you mean "change it to".
> (Meaning, without referencing UPDATE).

Yes.  I suppose we could add a parenthical comment, given that it's
likely the most popular option?  Feel free to suggest something
specific.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them."  (Larry Wall)

  133. Re: MERGE and parsing with prepared statements

    Simon Riggs <simon.riggs@enterprisedb.com> — 2022-08-12T11:46:55Z

    On Fri, 12 Aug 2022 at 12:20, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> On 2022-Jul-18, Justin Pryzby wrote:
>
> > On Fri, Jul 15, 2022 at 03:43:41PM -0500, Justin Pryzby wrote:
> > > Should that sentence be removed from MERGE ?
> >
> > Also, I think these examples should be more similar.
>
> Agreed, done.

Sorry, but I disagree with this chunk in the latest commit,
specifically, changing the MATCHED from after to before the NOT
MATCHED clause.

The whole point of the second example was to demonstrate that the
order of the MATCHED/NOT MATCHED clauses made no difference.

By changing the examples so they are the same, the sentence at line
573 now makes no sense.

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

  134. Re: MERGE and parsing with prepared statements

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-08-12T11:53:25Z

    On 2022-Aug-12, Simon Riggs wrote:

> Sorry, but I disagree with this chunk in the latest commit,
> specifically, changing the MATCHED from after to before the NOT
> MATCHED clause.
> 
> The whole point of the second example was to demonstrate that the
> order of the MATCHED/NOT MATCHED clauses made no difference.
> 
> By changing the examples so they are the same, the sentence at line
> 573 now makes no sense.

Hmm, I thought the point of the example was to show that you can replace
the table in the USING clause with a query that retrieves the column;
but you're right, we lost the thing there.  Maybe it was too subtle to
the point that I failed to understand it.  Perhaps we can put it back
the way it was and explain these two differences (change of data source
*and* clause ordering) more explicitly.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

  135. Re: MERGE and parsing with prepared statements

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-19T13:30:16Z

    On Fri, Aug 12, 2022 at 01:53:25PM +0200, Alvaro Herrera wrote:
> On 2022-Aug-12, Simon Riggs wrote:
> 
> > Sorry, but I disagree with this chunk in the latest commit,
> > specifically, changing the MATCHED from after to before the NOT
> > MATCHED clause.

3d895bc84 also moved a semicolon into the middle of the sql statement.

> > The whole point of the second example was to demonstrate that the
> > order of the MATCHED/NOT MATCHED clauses made no difference.
> > 
> > By changing the examples so they are the same, the sentence at line
> > 573 now makes no sense.
> 
> Hmm, I thought the point of the example was to show that you can replace
> the table in the USING clause with a query that retrieves the column;
> but you're right, we lost the thing there.  Maybe it was too subtle to
> the point that I failed to understand it.  Perhaps we can put it back
> the way it was and explain these two differences (change of data source
> *and* clause ordering) more explicitly.

Evidently I misunderstood it, too.

-- 
Justin

  136. Re: MERGE and parsing with prepared statements

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-09-09T11:58:39Z

    On 2022-Aug-12, Simon Riggs wrote:

> Sorry, but I disagree with this chunk in the latest commit,
> specifically, changing the MATCHED from after to before the NOT
> MATCHED clause.
> 
> The whole point of the second example was to demonstrate that the
> order of the MATCHED/NOT MATCHED clauses made no difference.
> 
> By changing the examples so they are the same, the sentence at line
> 573 now makes no sense.

Not sure how to fix this.  We could add put the WHEN clauses in the
order they were, and add another phrase to that paragraph to explain
more explicitly that the order is not relevant (while at the same time
explaining that if you have multiple WHEN MATCHED clauses, the relative
order of those *is* relevant).

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"At least to kernel hackers, who really are human, despite occasional
rumors to the contrary" (LWN.net)