Date: Fri, 28 Jan 2022 17:19:48 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@lists.postgresql.org,
	Simon Riggs <simon.riggs@enterprisedb.com>,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Zhihong Yu <zyu@yugabyte.com>,
	Daniel Westermann <dwe@dbi-services.com>,
	Amit Langote <amitlangote09@gmail.com>,
	Japin Li <japinli@hotmail.com>, Erik Rijkers <er@xs4all.nl>,
	Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: support for MERGE
Message-ID: <20220128231948.GI23027@telsasoft.com>
References: <202201202106.tbs3a6tyk453@alvherre.pgsql>
 <202201282027.jt555atu6hlh@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202201282027.jt555atu6hlh@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220128231948.GI23027%40telsasoft.com>
Precedence: bulk

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

It's probably a good idea.  

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

From commit message:

> MERGE does not yet support inheritance,

It does support it now, right ?

From merge.sgml:

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

s/an delete/a delete/

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

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

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

Looks like this is intended to be in alpha order.

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

based on ?

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

s/partitioned/has child tables/ ?

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

Is it intended to stay in alpha order (?)

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

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

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

so *if* ?

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

alphabetize (?)

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

relhassubclass=false is wrong now ?

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

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

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

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

-- 
Justin