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>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Cc: Daniel Westermann <dwe@dbi-services.com>
Cc: Amit Langote <amitlangote09@gmail.com>
Cc: Japin Li <japinli@hotmail.com>
Cc: Erik Rijkers <er@xs4all.nl>
Cc: Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: support for MERGE
Date: Fri, 28 Jan 2022 17:19:48 -0600
Message-ID: <20220128231948.GI23027@telsasoft.com> (raw)
In-Reply-To: <202201282027.jt555atu6hlh@alvherre.pgsql>
References: <202201202106.tbs3a6tyk453@alvherre.pgsql>
<202201282027.jt555atu6hlh@alvherre.pgsql>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, alvherre@alvh.no-ip.org, simon.riggs@enterprisedb.com, tomas.vondra@enterprisedb.com, zyu@yugabyte.com, dwe@dbi-services.com, amitlangote09@gmail.com, japinli@hotmail.com, er@xs4all.nl, jcasanov@systemguards.com.ec
Subject: Re: support for MERGE
In-Reply-To: <20220128231948.GI23027@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox