Re: support for MERGE

[Justin Pryzby]

On Tue, Aug 09, 2022 at 11:48:23AM +0200, Álvaro Herrera wrote:
> On 2022-Aug-01, Álvaro Herrera wrote:
> 
> > > >  If MERGE attempts an INSERT
> > > >  and a unique index is present and a duplicate row is concurrently
> > > > +inserted, then a uniqueness violation error is raised;
> > > > +MERGE does not attempt to avoid such
> > > > +errors by evaluating MATCHED conditions.
> > > 
> > > This was a portion of a chang that was committed as eebf2.
> > > 
> > > But I don't understand why this changed from "does not attempt to avoid 
> > > the
> > > error by executing an UPDATE." to "...by evaluating
> > > MATCHED 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 MERGE attempts an INSERT
>and a unique index is present and a duplicate row is concurrently
>inserted, then a uniqueness violation error is raised;
>MERGE does not attempt to avoid such
>errors by restarting evaluation of MATCHED
>  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

Re: support for MERGE

[Álvaro Herrera]

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

> > >  If MERGE attempts an INSERT
> > >  and a unique index is present and a duplicate row is concurrently
> > > +inserted, then a uniqueness violation error is raised;
> > > +MERGE does not attempt to avoid such
> > > +errors by evaluating MATCHED conditions.
> > 
> > This was a portion of a chang that was committed as eebf2.
> > 
> > But I don't understand why this changed from "does not attempt to avoid the
> > error by executing an UPDATE." to "...by evaluating
> > MATCHED 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 MERGE attempts an INSERT
   and a unique index is present and a duplicate row is concurrently
   inserted, then a uniqueness violation error is raised;
   MERGE does not attempt to avoid such
   errors by restarting evaluation of MATCHED
   conditions.

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

-- 
Álvaro HerreraBreisgau, 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)

Re: support for MERGE

[Álvaro Herrera]

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 MERGE attempts an INSERT
> >  and a unique index is present and a duplicate row is concurrently
> > -inserted, then a uniqueness violation is raised.
> > -MERGE does not attempt to avoid the
> > -error by executing an UPDATE.
> > +inserted, then a uniqueness violation error is raised;
> > +MERGE does not attempt to avoid such
> > +errors by evaluating MATCHED conditions.
> 
> This was a portion of a chang that was committed as eebf2.
> 
> But I don't understand why this changed from "does not attempt to avoid the
> error by executing an UPDATE." to "...by evaluating
> MATCHED 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)

Re: support for MERGE

[Justin Pryzby]

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 MERGE attempts an INSERT
>  and a unique index is present and a duplicate row is concurrently
> -inserted, then a uniqueness violation is raised.
> -MERGE does not attempt to avoid the
> -error by executing an UPDATE.
> +inserted, then a uniqueness violation error is raised;
> +MERGE does not attempt to avoid such
> +errors by evaluating MATCHED conditions.

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

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

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

Sorry to re-raise this 6 weeks later..

-- 
Justin

Re: support for MERGE

[Álvaro Herrera]

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)

Re: support for MERGE

[Álvaro Herrera]

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.From 582f8b932cc515485bfdb84479f0600f91bbf9a7 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 18 May 2022 18:41:04 +0200
Subject: [PATCH v4] Link to MVCC docs in MERGE docs

---
 doc/src/sgml/mvcc.sgml  | 15 ---
 doc/src/sgml/ref/merge.sgml |  4 
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 341fea524a..2f8d2b17ef 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -425,13 +425,13 @@ COMMIT;

 MERGE allows the user to specify various
 combinations of INSERT, UPDATE
-or DELETE subcommands. A MERGE
+and DELETE subcommands. A MERGE
 command with both INSERT and UPDATE
 subcommands looks similar to INSERT with an
 ON CONFLICT DO UPDATE clause but does not
 guarantee that either INSERT or
 UPDATE will occur.
-If MERGE attempts an UPDATE or
+If MERGE attempts an UPDATE or
 DELETE and the row is concurrently updated but
 the join condition still passes for the current target and the
 current source tuple, then MERGE will behave
@@ -448,9 +448,9 @@ COMMIT;
 and execute the first one that succeeds.
 If MERGE attempts an INSERT
 and a unique index is present and a duplicate row is concurrently
-inserted, then a uniqueness violation is raised.
-MERGE does not attempt to avoid the
-error by executing an UPDATE.
+inserted, then a uniqueness violation error is raised;
+MERGE does not attempt to avoid such
+errors by evaluating MATCHED conditions.

 

@@ -516,8 +516,9 @@ COMMIT;

 

-UPDATE, DELETE, SELECT
-FOR UPDATE, and SELECT FOR SHARE commands
+UPDATE, DELETE,
+MERGE, SELECT FOR UPDATE,
+and SELECT FOR SHARE commands
 behave the same as SELECT
 in terms of searching for target rows: they will only find target rows
 that were committed as of the transaction start time.  However, such a
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index f68aa09736..f0745d01c7 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -539,6 +539,10 @@ MERGE total_count
   
 
   
+   When MERGE is run concurrently with other commands
+   that modify the target table, the usual transaction isolation rules
+   apply; see  for an explanation
+   on the behavior at each isolation level.
You may also wish to consider using INSERT ... ON CONFLICT
as an alternative statement which offers the ability to run an
UPDATE if a concurrent INSERT
-- 
2.30.2

Re: support for MERGE

[Alvaro Herrera]

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 HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)
>From b41c2a1725af0ba3513219b8ea7eceb32e93f9e0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 18 May 2022 18:41:04 +0200
Subject: [PATCH v3] Link to MVCC docs in MERGE docs

---
 doc/src/sgml/mvcc.sgml  | 10 +-
 doc/src/sgml/ref/merge.sgml |  2 ++
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 341fea524a..1d4d5a62f9 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -425,13 +425,13 @@ COMMIT;

 MERGE allows the user to specify various
 combinations of INSERT, UPDATE
-or DELETE subcommands. A MERGE
+and DELETE subcommands. A MERGE
 command with both INSERT and UPDATE
 subcommands looks similar to INSERT with an
 ON CONFLICT DO UPDATE clause but does not
 guarantee that either INSERT or
 UPDATE will occur.
-If MERGE attempts an UPDATE or
+If MERGE attempts an UPDATE or
 DELETE and the row is concurrently updated but
 the join condition still passes for the current target and the
 current source tuple, then MERGE will behave
@@ -448,9 +448,9 @@ COMMIT;
 and execute the first one that succeeds.
 If MERGE attempts an INSERT
 and a unique index is present and a duplicate row is concurrently
-inserted, then a uniqueness violation is raised.
-MERGE does not attempt to avoid the
-error by executing an UPDATE.
+inserted, then a uniqueness violation error is raised;
+MERGE does not attempt to avoid such
+errors by evaluating MATCHED conditions.

 

diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index f68aa09736..271076bfd5 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -539,6 +539,8 @@ MERGE total_count
   
 
   
+   See  for a thorough explanation on the behavior of
+   MERGE under concurrency.
You may also wish to consider using INSERT ... ON CONFLICT
as an alternative statement which offers the ability to run an
UPDATE if a concurrent INSERT
-- 
2.30.2

Re: support for MERGE

[Justin Pryzby]

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

Re: support for MERGE

[Peter Eisentraut]

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.

Re: support for MERGE

[Justin Pryzby]

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 ?

Re: support for MERGE

[Alvaro Herrera]

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

Re: support for MERGE

[Tom Lane]

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

Re: support for MERGE

[Justin Pryzby]

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  > class="parameter">total_count
> > UPDATE if a concurrent INSERT
> > occurs.  There are a variety of differences and restrictions between
> > the two statement types and they are not interchangeable.
> > +   See  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

Re: support for MERGE

[Alvaro Herrera]

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  class="parameter">total_count
> UPDATE if a concurrent INSERT
> occurs.  There are a variety of differences and restrictions between
> the two statement types and they are not interchangeable.
> +   See  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 HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
>From 5bad2c5a4bcd2bf04751f980f947c1050aeafd03 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 18 May 2022 18:41:04 +0200
Subject: [PATCH v2] Link to MVCC docs in MERGE docs.

---
 doc/src/sgml/mvcc.sgml  | 2 +-
 doc/src/sgml/ref/merge.sgml | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 341fea524a..4446e1c484 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -425,7 +425,7 @@ COMMIT;

 MERGE allows the user to specify various
 combinations of INSERT, UPDATE
-or DELETE subcommands. A MERGE
+and DELETE subcommands. A MERGE
 command with both INSERT and UPDATE
 subcommands looks similar to INSERT with an
 ON CONFLICT DO UPDATE clause but does not
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index f68aa09736..6b94c863b5 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -539,6 +539,8 @@ MERGE total_count
   
 
   
+   See  for more details on the behavior of
+   MERGE under concurrency.
You may also wish to consider using INSERT ... ON CONFLICT
as an alternative statement which offers the ability to run an
UPDATE if a concurrent INSERT
-- 
2.30.2

Re: support for MERGE

[Alvaro Herrera]

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/

Re: support for MERGE

[Justin Pryzby]

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

Re: support for MERGE

[Laurenz Albe]

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

Re: support for MERGE

[Justin Pryzby]

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;

 MERGE allows the user to specify various
 combinations of INSERT, UPDATE
-or DELETE subcommands. A MERGE
+and DELETE subcommands. A MERGE
 command with both INSERT and UPDATE
 subcommands looks similar to INSERT with an
 ON CONFLICT DO UPDATE 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 total_count
UPDATE if a concurrent INSERT
occurs.  There are a variety of differences and restrictions between
the two statement types and they are not interchangeable.
+   See  for more information.
   
  
 

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.

Re: support for MERGE

[Ranier Vilela]

Em ter., 12 de abr. de 2022 às 10:47, Alvaro Herrera <
alvhe...@alvh.no-ip.org> escreveu:

> On 2022-Apr-02, Ranier Vilela wrote:
>
> > Em sáb., 2 de abr. de 2022 às 12:01, Alvaro Herrera <
> alvhe...@alvh.no-ip.org>
> > escreveu:
>
> > IMHO, actually there are bug here.
> > ExecGetChildToRootMap is clear, is possible returning NULL.
> > To discover if the map is NULL, ExecGetChildToRootMap needs to process
> > "ResultRelInfo *leaf_part_rri".
> > So, the argument "if the map is NULL, this function should not be
> called",
> > is contradictory.
>
> I was not explicit enough.  I meant "if no map is needed to adjust
> columns, then this function should not be called".  The caller already
> knows if it's needed or not; it doesn't depend on literally testing
> 'map'.  If somebody mis-calls this function, it would have crashed, yes;
> but that's a caller bug, not this function's.
>
Thanks for the explanation.


>
> A few days ago, the community Coverity also complained about this, so I
> added an Assert that the map is not null, which should silence it.
>
Thanks for hardening this.


>
> > If the right fix is to return the original list, here is the patch
> attached.
>
> ... for a buggy caller (one that calls it when unnecessary), then yes
> this would be the correct code -- except that now the caller doesn't
> know if the returned list needs to be freed or not.  So it seems better
> to avoid accumulating pointless calls to this function by just not
> coping with them.
>
 Sure, it is always better to avoid doing work, unless strictly necessary.

regards,
Ranier Vilela

Re: support for MERGE

[Alvaro Herrera]

On 2022-Apr-02, Ranier Vilela wrote:

> Em sáb., 2 de abr. de 2022 às 12:01, Alvaro Herrera 
> escreveu:

> IMHO, actually there are bug here.
> ExecGetChildToRootMap is clear, is possible returning NULL.
> To discover if the map is NULL, ExecGetChildToRootMap needs to process
> "ResultRelInfo *leaf_part_rri".
> So, the argument "if the map is NULL, this function should not be called",
> is contradictory.

I was not explicit enough.  I meant "if no map is needed to adjust
columns, then this function should not be called".  The caller already
knows if it's needed or not; it doesn't depend on literally testing
'map'.  If somebody mis-calls this function, it would have crashed, yes;
but that's a caller bug, not this function's.

A few days ago, the community Coverity also complained about this, so I
added an Assert that the map is not null, which should silence it.

> If the right fix is to return the original list, here is the patch attached.

... for a buggy caller (one that calls it when unnecessary), then yes
this would be the correct code -- except that now the caller doesn't
know if the returned list needs to be freed or not.  So it seems better
to avoid accumulating pointless calls to this function by just not
coping with them.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I suspect most samba developers are already technically insane...
Of course, since many of them are Australians, you can't tell." (L. Torvalds)

Re: support for MERGE

[Ranier Vilela]

Em sáb., 2 de abr. de 2022 às 12:01, Alvaro Herrera 
escreveu:

> On 2022-Mar-31, Daniel Gustafsson wrote:
>
> > > On 31 Mar 2022, at 19:38, Ranier Vilela  wrote:
> >
> > > I think that there is an oversight at 7103ebb
> > > There is no chance of Assert preventing this bug.
> >
> > This seems reasonable from brief reading of the code, NULL is a
> legitimate
> > value for the map and that should yield an empty list AFAICT.
>
> There's no bug here and this is actually intentional: if the map is
> NULL, this function should not be called.
>
IMHO, actually there are bug here.
ExecGetChildToRootMap is clear, is possible returning NULL.
To discover if the map is NULL, ExecGetChildToRootMap needs to process
"ResultRelInfo *leaf_part_rri".
So, the argument "if the map is NULL, this function should not be called",
is contradictory.

Actually, with Assert at function adjust_partition_colnos_using_map,
will never be checked, because it crashed before, both
production and debug modes.


> In the code before this commit, there was an assert that this variable
> was not null:
>
>   static List *
>   adjust_partition_colnos(List *colnos, ResultRelInfo *leaf_part_rri)
>   {
> -   List   *new_colnos = NIL;
> TupleConversionMap *map = ExecGetChildToRootMap(leaf_part_rri);
> !   AttrMap*attrMap;
> ListCell   *lc;
>
> !   Assert(map != NULL);/* else we shouldn't be here */
> !   attrMap = map->attrMap;
>
> foreach(lc, colnos)
> {
>
>
> We could add an Assert that map is not null in the new function, but
> really there's no point: if the map is null, we'll crash just fine in
> the following line.
>
> I would argue that we should *remove* the Assert() that I left in
> adjust_partition_colnos_with_map.
>
> Even if we wanted to make the function handle the case of a NULL map,
> then the right fix is not to return NIL, but rather we should return the
> original list.
>
If the right fix is to return the original list, here is the patch attached.

regards
Ranier Vilela


v2-0001-avoid-dereference-null-map.patch
Description: Binary data

Re: support for MERGE

[Andres Freund]

Hi,

On 2022-04-02 17:02:01 +0200, Alvaro Herrera wrote:
> There's no bug here and this is actually intentional: if the map is
> NULL, this function should not be called.

This made me, again, wonder if we should add a pg_nonnull attibute to c.h. The
compiler can probably figure it out in this case, but there's plenty cases it
can't, because the function definition is in a different translation unit. And
IMO it helps humans too.

Regards,

Andres

Re: support for MERGE

[Alvaro Herrera]

On 2022-Mar-31, Daniel Gustafsson wrote:

> > On 31 Mar 2022, at 19:38, Ranier Vilela  wrote:
> 
> > I think that there is an oversight at 7103ebb
> > There is no chance of Assert preventing this bug.
> 
> This seems reasonable from brief reading of the code, NULL is a legitimate
> value for the map and that should yield an empty list AFAICT.

There's no bug here and this is actually intentional: if the map is
NULL, this function should not be called.

In the code before this commit, there was an assert that this variable
was not null:

  static List *
  adjust_partition_colnos(List *colnos, ResultRelInfo *leaf_part_rri)
  {
-   List   *new_colnos = NIL;
TupleConversionMap *map = ExecGetChildToRootMap(leaf_part_rri);
!   AttrMap*attrMap;
ListCell   *lc;
  
!   Assert(map != NULL);/* else we shouldn't be here */
!   attrMap = map->attrMap;
  
foreach(lc, colnos)
{


We could add an Assert that map is not null in the new function, but
really there's no point: if the map is null, we'll crash just fine in
the following line.

I would argue that we should *remove* the Assert() that I left in
adjust_partition_colnos_with_map.

Even if we wanted to make the function handle the case of a NULL map,
then the right fix is not to return NIL, but rather we should return the
original list.

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

Re: support for MERGE

[Daniel Gustafsson]

> On 31 Mar 2022, at 19:38, Ranier Vilela  wrote:

> I think that there is an oversight at 7103ebb
> There is no chance of Assert preventing this bug.

This seems reasonable from brief reading of the code, NULL is a legitimate
value for the map and that should yield an empty list AFAICT.

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

support for MERGE

[Ranier Vilela]

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

I think that there is an oversight at 7103ebb

There is no chance of Assert preventing this bug.

regards,
Ranier Vilela


0001-avoid-dereference-null-map.patch
Description: Binary data

Re: support for MERGE

[Alvaro Herrera]

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)

Re: support for MERGE

[Alvaro Herrera]

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)

Re: support for MERGE

[Peter Eisentraut]

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.From 18ad74bfc2b6d1a758eaab9d2332532f6abb074d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 18 Mar 2022 11:44:16 +0100
Subject: [PATCH 1/2] fixup! MERGE SQL Command following SQL:2016

Add back serial commas.
---
 doc/src/sgml/libpq.sgml | 2 +-
 doc/src/sgml/mvcc.sgml  | 2 +-
 doc/src/sgml/ref/create_policy.sgml | 2 +-
 doc/src/sgml/ref/insert.sgml| 2 +-
 doc/src/sgml/ref/merge.sgml | 6 +++---
 doc/src/sgml/trigger.sgml   | 2 +-
 src/include/nodes/execnodes.h   | 2 +-
 src/include/nodes/pathnodes.h   | 2 +-
 src/include/nodes/plannodes.h   | 2 +-
 9 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 5fcfb29a55..70233aa872 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -4128,7 +4128,7 @@ Retrieving Other Result Information
MERGE, MOVE, 
FETCH,
or COPY statement, or an EXECUTE 
of a
prepared query that contains an INSERT,
-   UPDATE, DELETE
+   UPDATE, DELETE,
or MERGE statement.
If the command that generated the PGresult was 
anything
else,  returns an empty string. The 
caller
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 62451d297b..9b3061c03d 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -955,7 +955,7 @@ Table-Level Lock Modes
 
 
  The commands UPDATE,
- DELETE, INSERT and
+ DELETE, INSERT, and
  MERGE
  acquire this lock mode on the target table (in addition to
  ACCESS SHARE locks on any other referenced
diff --git a/doc/src/sgml/ref/create_policy.sgml 
b/doc/src/sgml/ref/create_policy.sgml
index f45882814d..c8ecc06347 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -619,7 +619,7 @@ Notes
   
No separate policy exists for MERGE. Instead, the 
policies
defined for SELECT, INSERT,
-   UPDATE and DELETE are applied
+   UPDATE, and DELETE are applied
while executing MERGE, depending on the actions that are
performed.
   
diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index ad61d757af..41cafc8455 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -592,7 +592,7 @@ Notes
 
   
You may also wish to consider using MERGE, since that
-   allows mixing INSERT, UPDATE and
+   allows mixing INSERT, UPDATE, and
DELETE within a single statement.
See .
   
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index 73778666e9..3a1823ceed 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -118,8 +118,8 @@ Description
   
MERGE is not supported if the
target_table_name is a
-   materialized view, foreign table or if it has any
-   RULEs defined on it.
+   materialized view, foreign table, or if it has any
+   rules defined on it.
   
  
 
@@ -153,7 +153,7 @@ Parameters
 source_table_name
 
  
-  The name (optionally schema-qualified) of the source table, view or
+  The name (optionally schema-qualified) of the source table, view, or
   transition table.
  
 
diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 2662be243f..04e702a795 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -195,7 +195,7 @@ Overview of Trigger Behavior

 No separate triggers are defined for MERGE. Instead,
 statement-level or row-level UPDATE,
-DELETE and INSERT triggers are fired
+DELETE, and INSERT triggers are fired
 depending on (for statement-level triggers) what actions are specified in
 the MERGE query and (for row-level triggers) what
 actions 

Re: support for MERGE

[Alvaro Herrera]

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.

Re: support for MERGE

[Japin Li]

On Thu, 17 Mar 2022 at 03:18, Alvaro Herrera  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.

Re: support for MERGE

[Amit Langote]

On Mon, Mar 14, 2022 at 11:36 PM Amit Langote  wrote:
> On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera  
> 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
> +{
> +   boolupdateIndexes;  /* index update required? */
> +   boolcrossPartUpdate;/* 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

Re: support for MERGE

[Japin Li]

On Sat, 12 Mar 2022 at 11:53, Alvaro Herrera  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.

Re: support for MERGE

[Amit Langote]

On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera  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
+{
+   boolupdateIndexes;  /* index update required? */
+   boolcrossPartUpdate;/* 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

Re: support for MERGE

[Justin Pryzby]

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;
> + boolmatched;/* 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;
> + boolmatched;/* 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 

Re: support for MERGE

[Alvaro Herrera]

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)

Re: support for MERGE

[Simon Riggs]

On Sun, 27 Feb 2022 at 17:35, Alvaro Herrera  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, 
> > > ,
> > > + 
> > > mtstate->canSetTag);
> > > +   InstrCountFiltered1(>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 Riggshttp://www.EnterpriseDB.com/


new_insert_tests_for_merge.patch
Description: Binary data

Re: support for MERGE

[Zhihong Yu]

On Wed, Mar 9, 2022 at 9:38 AM Alvaro Herrera 
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

Re: support for MERGE

[Álvaro Herrera]

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)

Re: support for MERGE

[Zhihong Yu]

On Mon, Mar 7, 2022 at 12:04 PM Álvaro Herrera 
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

Re: support for MERGE

[Álvaro Herrera]

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.

Re: support for MERGE

[Julien Rouhaud]

On Sun, Feb 27, 2022 at 09:17:13PM +0100, Daniel Gustafsson wrote:
> > On 27 Feb 2022, at 18:42, Tom Lane  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?

Re: support for MERGE

[Daniel Gustafsson]

> On 27 Feb 2022, at 18:42, Tom Lane  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/

Re: support for MERGE

[Zhihong Yu]

On Sun, Feb 27, 2022 at 9:25 AM Alvaro Herrera 
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

Re: support for MERGE

[Tom Lane]

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

Re: support for MERGE

[Alvaro Herrera]

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, 
> > ,
> > + 
> > mtstate->canSetTag);
> > +   InstrCountFiltered1(>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)

Re: support for MERGE

[Justin Pryzby]

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

Re: support for MERGE

[Alvaro Herrera]

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"

Re: support for MERGE

[Justin Pryzby]

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

Re: support for MERGE

[Alvaro Herrera]

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/

Re: support for MERGE

[Erik Rijkers]

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--- src/bin/psql/tab-complete.c
+++ src/bin/psql/tab-complete.c
@@ -1714,6 +1736,12 @@ psql_completion(const char *text, int start, int end)
 	/* ALTER PUBLICATION  ADD */
 	else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD"))
 		COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE");
+	else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD|SET", "TABLE") ||
+			 (HeadMatches("ALTER", "PUBLICATION", MatchAny, "ADD|SET", "TABLE") &&
+			  ends_with(prev_wd, ',')))
+		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);
+	else if (HeadMatches("ALTER", "PUBLICATION", MatchAny, "ADD|SET", "TABLE"))
+		COMPLETE_WITH(",");
 	/* ALTER PUBLICATION  DROP */
 	else if (Matches("ALTER", "PUBLICATION", MatchAny, "DROP"))
 		COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE");

Re: support for MERGE

[Justin Pryzby]

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

Looks like this is intended to be in alpha order.

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

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

Re: support for MERGE

[Zhihong Yu]

On Fri, Jan 28, 2022 at 2:19 PM Andres Freund  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/2018040523.gar3j26gsk32g...@alap3.anarazel.de
> [2]
> https://www.postgresql.org/message-id/20180403021800.b5nsgiclzanob...@alap3.anarazel.de
>
>
>

Re: support for MERGE

[Andres Freund]

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/2018040523.gar3j26gsk32g...@alap3.anarazel.de
[2] 
https://www.postgresql.org/message-id/20180403021800.b5nsgiclzanob...@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;
> + boolmatched = false;
> + Datum   datum;
> + boolisNull;
> +
> + /*
> +  * 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,
> +  );

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


> +  * A concurrent update can:
> +  *
>

Re: support for MERGE

[Alvaro Herrera]

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

Re: support for MERGE

[Japin Li]

On Fri, 21 Jan 2022 at 05:06, Alvaro Herrera  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.

Re: support for MERGE

[Alvaro Herrera]

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.8010...@sigaev.ru

Re: support for MERGE

[Alvaro Herrera]

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)

Re: support for MERGE

[Peter Eisentraut]

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.

Re: support for MERGE

[Justin Pryzby]

Resending some language fixes to the public documentation.
>From 28b8532976ddb3e8b617ca007fae6b4822b36527 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sat, 13 Nov 2021 12:11:46 -0600
Subject: [PATCH] f!typos

---
 doc/src/sgml/mvcc.sgml  |  8 ++---
 doc/src/sgml/ref/create_policy.sgml |  5 +--
 doc/src/sgml/ref/insert.sgml|  2 +-
 doc/src/sgml/ref/merge.sgml | 48 ++---
 doc/src/sgml/trigger.sgml   |  7 +++--
 src/test/regress/expected/merge.out |  4 +--
 src/test/regress/sql/merge.sql  |  4 +--
 7 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index a1ae8423414..61a10c28120 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -441,16 +441,16 @@ COMMIT;
 can specify several actions and they can be conditional, the
 conditions for each action are re-evaluated on the updated version of
 the row, starting from the first action, even if the action that had
-originally matched was later in the list of actions.
+originally matched appears later in the list of actions.
 On the other hand, if the row is concurrently updated or deleted so
 that the join condition fails, then MERGE will
-evaluate the conditions NOT MATCHED actions next,
+evaluate the condition's NOT MATCHED actions next,
 and execute the first one that succeeds.
 If MERGE attempts an INSERT
 and a unique index is present and a duplicate row is concurrently
-inserted then a uniqueness violation is raised.
+inserted, then a uniqueness violation is raised.
 MERGE does not attempt to avoid the
-ERROR by attempting an UPDATE.
+ERROR by executing an UPDATE.

 

diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
index 3db3908b429..db312681f78 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -96,10 +96,11 @@ CREATE POLICY name ON 
 
   
-   No separate policy exists for MERGE. Instead policies
+   No separate policy exists for MERGE. Instead, the policies
defined for SELECT, INSERT,
UPDATE and DELETE are applied
-   while executing MERGE, depending on the actions that are activated.
+   while executing MERGE, depending on the actions that are
+   performed.
   
  
 
diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index 477de2689b6..ad61d757af5 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -592,7 +592,7 @@ INSERT oid count
You may also wish to consider using MERGE, since that
-   allows mixed INSERT, UPDATE and
+   allows mixing INSERT, UPDATE and
DELETE within a single statement.
See .
   
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index 7700d9b9bb1..149df9f0ad9 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -73,11 +73,11 @@ DELETE
from data_source to
target_table_name
producing zero or more candidate change rows.  For each candidate change
-   row the status of MATCHED or NOT MATCHED
+   row, the status of MATCHED or NOT MATCHED
is set just once, after which WHEN clauses are evaluated
-   in the order specified.  The first clause to match each candidate change
-   row is executed.  No more than one WHEN clause is
-   executed for any candidate change row.  
+   in the order specified.  For each candidate change row, the first clause to
+   evaluate as true executed.  No more than one WHEN clause
+   is executed for any candidate change row.  
   
 
   
@@ -85,14 +85,14 @@ DELETE
regular UPDATE, INSERT, or
DELETE commands of the same names. The syntax of
those commands is different, notably that there is no WHERE
-   clause and no tablename is specified.  All actions refer to the
+   clause and no table name is specified.  All actions refer to the
target_table_name,
though modifications to other tables may be made using triggers.
   
 
   
-   When DO NOTHING action is specified, the source row is
-   skipped. Since actions are evaluated in the given order, DO
+   When DO NOTHING is specified, the source row is
+   skipped. Since actions are evaluated in their specified order, DO
NOTHING can be handy to skip non-interesting source rows before
more fine-grained handling.
   
@@ -178,7 +178,7 @@ DELETE
 
  
   A substitute name for the data source. When an alias is
-  provided, it completely hides whether table or query was specified.
+  provided, it completely hides the actual name of the table or query.
  
 

@@ -202,7 +202,7 @@ DELETE
rows should appear in join_condition.
join_condition subexpressions that
only reference target_table_name
-   columns can only affect which action is taken, often in surprising ways.
+   columns can affect which action is taken, often in surprising ways.
   
  
 
@@ 

Re: support for MERGE

[Erik Rijkers]

Op 13-01-2022 om 13:43 schreef Alvaro Herrera:

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

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



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


I read though the MERGE-docs; some typos:

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

'that clause clause'
'that clause'

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

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

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



Erik Rijkers

Re: support for MERGE

[Erik Rijkers]

Op 13-01-2022 om 13:43 schreef Alvaro Herrera:

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

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



[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

Re: support for MERGE

[Zhihong Yu]

On Thu, Jan 13, 2022 at 4:43 AM Alvaro Herrera 
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

Re: support for MERGE

[Andrew Dunstan]

On 1/5/22 02:53, Simon Riggs wrote:
> On Wed, 22 Dec 2021 at 11:35, Simon Riggs  
> wrote:
>> On Mon, 15 Nov 2021 at 22:45, Alvaro Herrera  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 


cheers


andrew


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

Re: support for MERGE

[Jaime Casanova]

On Wed, Dec 22, 2021 at 11:35:56AM +, Simon Riggs wrote:
> On Mon, 15 Nov 2021 at 22:45, Alvaro Herrera  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


merge1.sql
Description: application/sql
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
set = {__val = {4194304, 140726414547632, 2, 6, 5309120, 
94249822928768, 4611686018427388799, 140446649031334, 0, 281470681751456, 0, 0, 
0, 0, 0, 0}}
pid = 
tid = 
ret = 
#1  0x7fbc4867c535 in __GI_abort () at abort.c:79
save_stage = 1
act = {__sigaction_handler = {sa_handler = 0x0, sa_sigaction = 0x0}, 
sa_mask = {__val = {0, 0, 0, 0, 0, 140446646788085, 2, 4062868317402242624, 
7017002064575424051, 94249829619760, 
  7003715557358071819, 674, 12862997737215894016, 140726414547872, 
140726414547888, 140726414548720}}, sa_flags = 22, sa_restorer = 0x7ffd6bf31bb0}
sigs = {__val = {32, 0 }}
#2  0x55b83f88c8b5 in ExceptionalCondition 
(conditionName=conditionName@entry=0x55b83f9d61c8 "resultnum >= 0 && resultnum 
< resultslot->tts_tupleDescriptor->natts", 
errorType=errorType@entry=0x55b83f8e700b "FailedAssertion", 
fileName=fileName@entry=0x55b83f9d5830 "execExprInterp.c", 
lineNumber=lineNumber@entry=674) at assert.c:69
No locals.
#3  0x55b83f58ea5f in ExecInterpExpr (state=0x55b840b0ac18, 
econtext=0x55b840b0aa10, isnull=0x7ffd6bf31fff) at execExprInterp.c:674
resultnum = 
op = 0x55b840b0adb0
resultslot = 0x55b840b0ab40
innerslot = 0x55b840b08660
outerslot = 0x0
scanslot = 0x0
dispatch_table = {0x55b83f58e5c4 , 0x55b83f58e5e4 
, 0x55b83f58e616 , 0x55b83f58e648 
, 0x55b83f58e67a , 
  0x55b83f58e6db , 0x55b83f58e73c 
, 0x55b83f58e79d , 0x55b83f58e7bc 
, 0x55b83f58e7db , 
  0x55b83f58e7fa , 0x55b83f58e815 
, 0x55b83f58e8b7 , 0x55b83f58e959 
, 0x55b83f58e9fb , 
  0x55b83f58ea5f , 0x55b83f58eaeb 
, 0x55b83f58eb0c , 0x55b83f58eb39 
, 0x55b83f58eb92 , 
  0x55b83f58ebad , 0x55b83f58ebc8 
, 0x55b83f58ebcf , 0x55b83f58ec16 
, 0x55b83f58ec4c , 
  0x55b83f58ec53 , 0x55b83f58ec9a 
, 0x55b83f58ecd0 , 0x55b83f58ecf5 
, 0x55b83f58ed43 , 
  0x55b83f58ed64 , 0x55b83f58ed9a 
, 0x55b83f58edd0 , 0x55b83f58ee10 
, 0x55b83f58ee3f , 
  0x55b83f58ee6e , 0x55b83f58ee89 
, 0x55b83f58eea4 , 0x55b83f58eecb 
, 0x55b83f58ef0d , 
  0x55b83f58ef4f , 0x55b83f58ef76 
, 0x55b83f58ef91 , 0x55b83f58efac 
, 0x55b83f58efc5 , 
  0x55b83f58f053 , 0x55b83f58f08b 
, 0x55b83f58f1c9 , 0x55b83f58f24a 
, 0x55b83f58f2ba , 
  0x55b83f58f323 , 0x55b83f58f33a 
, 0x55b83f58f345 , 0x55b83f58f35c 
, 0x55b83f58f373 , 
  0x55b83f58f38e , 0x55b83f58f3a5 
, 0x55b83f58f467 , 0x55b83f58f511 
, 0x55b83f58f528 , 
  0x55b83f58f540 , 0x55b83f58f558 
, 0x55b83f58f570 , 0x55b83f58f5a8 
, 0x55b83f58f5aa , 
  0x55b83f58f5aa , 0x55b83f58f00c 
, 0x55b83f58f604 , 0x55b83f58f618 
, 0x55b83f58f5c0 , 
  0x55b83f58f5d8 , 0x55b83f58f5ec 
, 0x55b83f58f62c , 0x55b83f58f643 
, 0x55b83f58f69d , 
  0x55b83f58f6b4 , 0x55b83f58f715 
, 0x55b83f58f730 , 0x55b83f58f75b 
, 0x55b83f58f7d9 , 
  0x55b83f58f829 , 0x55b83f58f874 
, 0x55b83f58f8e4 , 0x55b83f58f9f6 
, 0x55b83f58faef , 
  0x55b83f58fbe1 , 0x55b83f58fd30 
, 0x55b83f58fe61 , 0x55b83f58ff85 
, 0x55b83f58ffa0 , 
  0x55b83f58ffbb }
#4  0x55b83f58ae18 in ExecInterpExprStillValid (state=0x55b840b0ac18, 
econtext=0x55b840b0aa10, isNull=0x7ffd6bf31fff) at execExprInterp.c:1824
No locals.
#5  0x55b83f597f7b in ExecEvalExprSwitchContext (isNull=0x7ffd6bf31fff, 
econtext=0x55b840b0aa10, state=0x55b840b0ac18) at 
../../../src/include/executor/executor.h:339
retDatum = 
oldContext = 0x55b840ad7b40
retDatum = 
oldContext = 
#6  ExecProject (projInfo=0x55b840b0ac10) at 
../../../src/include/executor/executor.h:373
econtext = 0x55b840b0aa10
state = 0x55b840b0ac18
slot = 0x55b840b0ab40
isnull = false
econtext = 
state = 
slot = 
isnull = 

Re: support for MERGE

[Simon Riggs]

On Wed, 22 Dec 2021 at 11:35, Simon Riggs  wrote:
>
> On Mon, 15 Nov 2021 at 22:45, Alvaro Herrera  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 Riggshttp://www.EnterpriseDB.com/


merge_fix_docs_into.v1.patch
Description: Binary data

Re: support for MERGE

[Álvaro Herrera]

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"

Re: support for MERGE

[Amit Langote]

Hi Álvaro,

On Tue, Nov 16, 2021 at 6:01 AM Álvaro Herrera  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,
> peaktempint,
> 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,
> peaktempint,
> 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);
>
> a

Re: support for MERGE

[Alvaro Herrera]

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)

Re: support for MERGE

[Álvaro Herrera]

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

Re: support for MERGE

[Amit Langote]

On Sun, Nov 14, 2021 at 12:23 AM Álvaro Herrera  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

Re: support for MERGE

[Justin Pryzby]

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
>From 119c6c7342ff9410424e94d60dc11b3a5e9d98c9 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sat, 13 Nov 2021 15:47:12 -0600
Subject: [PATCH] f!merge code comments

---
 src/backend/commands/trigger.c   |  8 +++
 src/backend/executor/execMerge.c | 24 +--
 src/backend/optimizer/plan/planner.c |  2 +-
 src/backend/optimizer/plan/setrefs.c |  2 +-
 src/backend/parser/parse_merge.c | 36 +++-
 src/backend/rewrite/rowsecurity.c|  2 +-
 src/bin/psql/tab-complete.c  |  2 +-
 7 files changed, 39 insertions(+), 37 deletions(-)

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index ea5c276cca..e708e4d185 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3694,9 +3694,9 @@ struct AfterTriggersTableData
 	AfterTriggerEventList after_trig_events;	/* if so, saved list pointer */
 
 	/*
-	 * We maintain separate transaction tables for UPDATE/INSERT/DELETE since
+	 * 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
+	 * needs both old and new transition tables whereas INSERT needs only new,
 	 * and DELETE needs only old.
 	 */
 
@@ -5650,8 +5650,8 @@ AfterTriggerGetTransitionTable(int event,
 	bool		insert_new_table = transition_capture->tcs_insert_new_table;
 
 	/*
-	 * For INSERT events NEW should be non-NULL, for DELETE events OLD should
-	 * be non-NULL, whereas for UPDATE events normally both OLD and NEW are
+	 * For INSERT events, NEW should be non-NULL, for DELETE events, OLD should
+	 * be non-NULL, whereas for UPDATE events, normally both OLD and NEW are
 	 * non-NULL.  But for UPDATE events fired for capturing transition tuples
 	 * during UPDATE partition-key row movement, OLD is NULL when the event is
 	 * for a row being inserted, whereas NEW is NULL when the event is for a
diff --git a/src/backend/executor/execMerge.c b/src/backend/executor/execMerge.c
index a9bd103f41..aabe10d14c 100644
--- a/src/backend/executor/execMerge.c
+++ b/src/backend/executor/execMerge.c
@@ -66,9 +66,9 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
 
 	/*
 	 * We run a JOIN between the target relation and the source relation to
-	 * find a set of candidate source rows that has matching row in the target
-	 * table and a set of candidate source rows that does not have matching
-	 * row in the target table. If the join returns us a tuple with target
+	 * find a set of candidate source rows that have a matching row in the target
+	 * table and a set of candidate source rows that does not have a matching
+	 * row in the target table. If the join returns a tuple with the target
 	 * relation's tid 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 ctid column
@@ -122,7 +122,7 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
 	 * XXX Hmmm, what if the updated tuple would now match one that was
 	 * considered NOT MATCHED so far?
 	 *
-	 * A concurrent delete, changes a WHEN MATCHED case to WHEN NOT MATCHED.
+	 * A concurrent delete changes a WHEN MATCHED case to WHEN NOT MATCHED.
 	 *
 	 * ExecMergeMatched takes care of following the update chain and
 	 * re-finding the qualifying WHEN MATCHED action, as long as the updated
@@ -140,7 +140,7 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
 	/*
 	 * Either we were dealing with a NOT MATCHED tuple or
 	 * ExecMergeNotMatched() returned "false", indicating the previously
-	 * MATCHED tuple is no longer a matching tuple.
+	 * MATCHED tuple no longer matches.
 	 */
 	if (!matched)
 		ExecMergeNotMatched(mtstate, resultRelInfo, estate, slot);
@@ -223,7 +223,7 @@ lmerge_matched:
 		/*
 		 * Test condition, if any
 		 *
-		 * In the absence of a condition we perform the action unconditionally
+		 * In the absence of any condition, we perform the action unconditionally
 		 * (no need to check separately since ExecQual() will return true if
 		 * there are no conditions to evaluate).
 		 */
@@ -231,14 +231,14 @@ lmerge_matched:
 			continue;
 
 		/*
-		 * Check if the existing target tuple meet the USING checks of
+		 * Check if the existing target tuple meets the USING checks of
 		 * UPDATE/DELETE RLS policies. If those checks fail, we throw an
 		 * error.
 		 *
 		 * The WITH CHECK quals 

Re: support for MERGE

[Justin Pryzby]

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
>From 2f3d2465a93ee6207b2afdab8787cf2eaa2c0bb4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sat, 13 Nov 2021 12:11:46 -0600
Subject: [PATCH] f!typos

---
 doc/src/sgml/mvcc.sgml  |  8 ++---
 doc/src/sgml/ref/create_policy.sgml |  5 +--
 doc/src/sgml/ref/insert.sgml|  2 +-
 doc/src/sgml/ref/merge.sgml | 50 ++---
 doc/src/sgml/trigger.sgml   |  7 ++--
 src/test/regress/expected/merge.out |  4 +--
 src/test/regress/sql/merge.sql  |  4 +--
 7 files changed, 41 insertions(+), 39 deletions(-)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index a1ae842341..7dd0a3811f 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -441,16 +441,16 @@ COMMIT;
 can specify several actions and they can be conditional, the
 conditions for each action are re-evaluated on the updated version of
 the row, starting from the first action, even if the action that had
-originally matched was later in the list of actions.
+originally matched appears later in the list of actions.
 On the other hand, if the row is concurrently updated or deleted so
 that the join condition fails, then MERGE will
-evaluate the conditions NOT MATCHED actions next,
+evaluate the condition's NOT MATCHED actions next,
 and execute the first one that succeeds.
 If MERGE attempts an INSERT
 and a unique index is present and a duplicate row is concurrently
-inserted then a uniqueness violation is raised.
+inserted, then a uniqueness violation is raised.
 MERGE does not attempt to avoid the
-ERROR by attempting an UPDATE.
+ERROR by executign an UPDATE.

 

diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
index 3db3908b42..db312681f7 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -96,10 +96,11 @@ CREATE POLICY name ON 
 
   
-   No separate policy exists for MERGE. Instead policies
+   No separate policy exists for MERGE. Instead, the policies
defined for SELECT, INSERT,
UPDATE and DELETE are applied
-   while executing MERGE, depending on the actions that are activated.
+   while executing MERGE, depending on the actions that are
+   performed.
   
  
 
diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index 477de2689b..ad61d757af 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -592,7 +592,7 @@ INSERT oid count
You may also wish to consider using MERGE, since that
-   allows mixed INSERT, UPDATE and
+   allows mixing INSERT, UPDATE and
DELETE within a single statement.
See .
   
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index 7e1de4..f7d29da9d9 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -73,10 +73,10 @@ DELETE
from data_source to
target_table_name
producing zero or more candidate change rows.  For each candidate change
-   row the status of MATCHED or NOT MATCHED
+   row, the status of MATCHED or NOT MATCHED
is set just once, after which WHEN clauses are evaluated
-   in the order specified.  The first clause to match each candidate change
-   row is executed.  No more than one WHEN clause is
+   in the order specified.  For each candidate change row, the first clause to
+   the row is executed.  No more than one WHEN clause is
executed for any candidate change row.  
   
 
@@ -85,14 +85,14 @@ DELETE
regular UPDATE, INSERT, or
DELETE commands of the same names. The syntax of
those commands is different, notably that there is no WHERE
-   clause and no tablename is specified.  All actions refer to the
+   clause and no table name is specified.  All actions refer to the
target_table_name,
though modifications to other tables may be made using triggers.
   
 
   
-   When DO NOTHING action is specified, the source row is
-   skipped. Since actions are evaluated in the given order, DO
+   When DO NOTHING is specified, the source row is
+   skipped. Since actions are evaluated in their specified order, DO
NOTHING can be handy to skip non-interesting source rows before
more fine-grained handling.
   
@@ -179,7 +179,7 @@ DELETE
 
  
   A substitute name for the data source. When an alias is
-  provided, it completely hides whether table or query was specified.
+  provided, it completely hides whatever table or query was specified.
  
 

@@ -203,7 +203,7 @@ DELETE
rows should appear in 

Re: support for MERGE

[Zhihong Yu]

On Sat, Nov 13, 2021 at 7:41 AM Alvaro Herrera 
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

Re: support for MERGE

[Alvaro Herrera]

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)

Re: support for MERGE

[Alvaro Herrera]

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/

Re: support for MERGE

[Álvaro Herrera]

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/

Re: support for MERGE

[Zhihong Yu]

On Fri, Nov 12, 2021 at 9:58 AM 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.
>
> 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

Re: support for MERGE

[Zhihong Yu]

On Fri, Nov 12, 2021 at 3:13 PM Alvaro Herrera 
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 ...

Re: support for MERGE

[Alvaro Herrera]

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/

Re: support for MERGE

[Zhihong Yu]

On Fri, Nov 12, 2021 at 9:58 AM 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.
>
> 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

Re: support for MERGE

[Alvaro Herrera]

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/

Re: support for MERGE

[Tomas Vondra]

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

Re: support for MERGE

[Daniel Gustafsson]

> On 1 Sep 2021, at 14:25, Alvaro Herrera  wrote:
> 
> On 2021-Sep-01, Daniel Gustafsson wrote:
> 
>>> On 21 Mar 2021, at 14:57, Alvaro Herrera  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/

Re: support for MERGE

[Alvaro Herrera]

On 2021-Sep-01, Daniel Gustafsson wrote:

> > On 21 Mar 2021, at 14:57, Alvaro Herrera  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/

Re: support for MERGE

[Daniel Gustafsson]

> On 21 Mar 2021, at 14:57, Alvaro Herrera  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/

Re: support for MERGE

[Magnus Hagander]

On Sun, Mar 21, 2021 at 2:57 PM Alvaro Herrera  wrote:
>
> On 2021-Mar-21, Magnus Hagander wrote:
>
> > On Fri, Mar 19, 2021 at 6:03 PM Bruce Momjian  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/

Re: support for MERGE

[Alvaro Herrera]

On 2021-Mar-21, Magnus Hagander wrote:

> On Fri, Mar 19, 2021 at 6:03 PM Bruce Momjian  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

Re: support for MERGE

[Magnus Hagander]

On Fri, Mar 19, 2021 at 6:03 PM Bruce Momjian  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/

Re: support for MERGE

[Bruce Momjian]

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  https://momjian.us
  EDB  https://enterprisedb.com

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

Re: support for MERGE

[David Steele]

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
da...@pgmasters.net

Re: support for MERGE

[Alvaro Herrera]

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

Re: support for MERGE

[David Steele]

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  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...@mail.gmail.com
https://postgr.es/m/7168.1547584...@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
da...@pgmasters.net

Re: support for MERGE

[Alvaro Herrera]

On 2021-Jan-18, Robert Haas wrote:

> On Thu, Dec 31, 2020 at 8:48 AM 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).
> 
> 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...@mail.gmail.com
https://postgr.es/m/7168.1547584...@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

Re: support for MERGE

[Robert Haas]

On Thu, Dec 31, 2020 at 8:48 AM 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).

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

Re: support for MERGE

[Alvaro Herrera]

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

Re: support for MERGE

[Alvaro Herrera]

On 2021-Jan-13, Simon Riggs wrote:

> On Sun, Jan 10, 2021 at 1:44 AM Tomas Vondra
>  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)

Re: support for MERGE

[Tomas Vondra]

On 1/13/21 11:20 AM, Simon Riggs wrote:

On Sun, Jan 10, 2021 at 1: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?


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

Re: support for MERGE

[Simon Riggs]

On Sun, Jan 10, 2021 at 1: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?

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 Riggshttp://www.EnterpriseDB.com/

Re: support for MERGE

[Vik Fearing]

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

Re: support for MERGE

[Tomas Vondra]

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  or
maybe , depending on the context. I think the new docs are a
bit confused which of those it should be, but I'd say  should
be used for SQL commands and  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
>From a332b1c279e847ec533ff34ffc35303672cd Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Sat, 9 Jan 2021 03:17:56 +0100
Subject: [PATCH 1/2] review

---
 doc/src/sgml/mvcc.sgml | 20 +++-
 doc/src/sgml/ref/create_policy.sgml|  6 +++---
 doc/src/sgml/ref/merge.sgml| 12 +---
 src/backend/commands/explain.c |  2 +-
 src/backend/executor/execMerge.c   |  5 -
 src/backend/executor/nodeModifyTable.c |  3 ++-
 src/backend/optimizer/prep/preptlist.c |  6 ++
 src/backend/parser/parse_agg.c |  1 +
 src/backend/replication/walsender.c|  4 ++--
 src/backend/rewrite/rewriteHandler.c   |  4 
 src/backend/utils/adt/varlena.c|  3 +++
 src/include/nodes/parsenodes.h |  7 ---
 12 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 9ec8474185..02c9f3fdea 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -429,22 +429,24 @@ COMMIT;
 with both INSERT and UPDATE
 subcommands looks similar to INSERT with an
 ON CONFLICT DO UPDATE clause but does not guarantee
-that either INSERT and UPDATE will 
occur.
+that either INSERT or UPDATE will 
occur.
 
-If MERGE attempts an UPDATE or DELETE and the row is concurrently updated
+   /* XXX This is a very long and hard to understand sentence :-( */
+   /* XXX Do we want to mention EvalPlanQual here? There's no explanation 
what it does in this file, so maybe elaborate what it does or leave that detail 
for a README? */
+If MERGE attempts an UPDATE or 
DELETE and the row is concurrently updated
 but the join condition still passes for the curre