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