Re: support for MERGE

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