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
