Re: [HACKERS] MERGE SQL Statement for PG11

On Thu, Mar 8, 2018 at 4:54 AM, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> wrote:
> Thanks for the feedback. I've greatly refactored the code based on your
> comments and I too like the resultant code. What we have now have
> essentially is: two functions:

I really like how ExecMerge() now mostly just consists of this code
(plus a lot of explanatory comments):

> + if (!matched ||
> + !ExecMergeMatched(mtstate, estate, slot, junkfilter, tupleid))
> + ExecMergeNotMatched(mtstate, estate, slot);

This is far easier to follow. In general, I'm now a lot less worried
about what was previously the #1 concern -- READ COMMITTED conflict
handling (EvalPlanQual() stuff). My #1 concern has become RLS, and
perhaps only because I haven't studied it in enough detail. It seems
like getting this patch committable is now a matter of verifying
details. Of course, there are a lot of details to verify. :-)

> I've also added a bunch of comments, but it might still not be enough. Feel
> free to suggest improvements there.

I think that the ExecMerge() comments are very good, although they
could perhaps use some light copy-editing. Also, I noticed some
specific issues with comments in ExecMerge() and friends, as well as a
few code issues. Those are:

* MERGE code needs to do cardinality violations like ON CONFLICT.
Specifically, you need the TransactionIdIsCurrentTransactionId()
check, and a second error fallback "attempted to lock invisible tuple"
error (though this should say "attempted to update or delete invisible
tuple" instead). The extra check is redundant, but better safe than
sorry. I like defensive errors like this because it makes the code
easier to read -- I don't get distracted by their absence.

* The last item on cardinality violations also implies this new merge
comment should be changed:

> + /*
> + * The target tuple was already updated or deleted by the
> + * current command, or by a later command in the current
> + * transaction.
> + */

It should be changed because it can't have been a later command in
this xact. We handled that case when we called ExecUpdate or
ExecDelete() (plus we now have the extra defensive "can't happen"
elog() error).

* This related comment shouldn't even talk about update/historic
behavior, now that it isn't in ExecUpdate() -- just MERGE:

> + /*
> + * The former case is possible in a join UPDATE where
> + * multiple tuples join to the same target tuple. This is
> + * pretty questionable, but Postgres has always allowed
> + * it: we just execute the first update action and ignore
> + * additional update attempts. SQLStandard disallows this
> + * for MERGE, so allow the caller to select how to handle
> + * this.
> + */

* This wording should be tweaked:

> + * If the current tuple is that last tuple in the update
> + * chain, then we know that the tuple was concurrently
> + * deleted. We just switch to NOT MATCHED case and let the
> + * caller retry the NOT MATCHED actions.

This should say something like "caller can move on to NOT MATCHED
actions". They can never go back from there, of course, which I want
us to be clear on.

* This check of whether whenqual is set is unnecessary, and doesn't
match MATCHED code, or the accompanying comments:

> + /*
> + * Test condition, if any
> + *
> + * In the absence of a condition we perform the action unconditionally
> + * (no need to check separately since ExecQual() will return true if
> + * there are no conditions to evaluate).
> + */
> + if (action->whenqual && !ExecQual(action->whenqual, econtext))
> + continue;

* I think that this ExecMerge() assertion is not helpful, since you go
on to dereference the pointer in all cases anyway:

> + Assert(junkfilter != NULL);

* Executor README changes, particularly about projecting twice, really
should be ExecMerge() comments. Maybe just get rid of these?

* Why are we using CMD_NOTHING at all? That constant has something to
do with user-visible rules, and there is no need to reuse it (make a
new CMD_* if you have to). More importantly, why do we even have the
corresponding DO NOTHING stuff in the grammar? Why would users want
that?

For quite a while, I thought that patch must have been support for ON
CONFLICT DO NOTHING within MERGE INSERTs (the docs don't even say what
DO NOTHING is). But that's not what it is at all. It seems like this
is a way of having an action that terminates early, so you don't have
to go on to evaluate other action quals. I can't see much point,
though. More importantly, supporting this necessitates code like the
following RLS code within ExecMergeMatched():

> + if ((action->commandType == CMD_UPDATE ||
> + action->commandType == CMD_DELETE) &&
> + resultRelInfo->ri_WithCheckOptions)
> + {
> + ExecWithCheckOptions(action->commandType == CMD_UPDATE ?
> + WCO_RLS_MERGE_UPDATE_CHECK : WCO_RLS_MERGE_DELETE_CHECK,
> + resultRelInfo,
> + mtstate->mt_merge_existing[ud_target],
> + mtstate->ps.state);
> + }

I wonder if the CMD_NOTHING case makes this code prone to information
leak attacks. Even if it that isn't the case, ISTM that several code
blocks within ExecMergeMatched() could do without the repeated
"action->commandType" tests. Have I missed something that makes
supporting DO NOTHING seem more compelling?

* The docs say that we use a LEFT OUTER JOIN for MERGE, while the
implementation uses a RIGHT OUTER JOIN because it's convenient for
parse analysis finding the target. This difference is a bit confusing.
Why say that we use any kind of join at all, though?

The SQL standard doesn't say outer join at all. Neither do the MERGE
docs of the other database systems that I checked. Taking a position
on this seems to add nothing at all. Let's make the MERGE docs refer
to it as a join, without further elaboration.

* I don't find this comment from analyze.c very helpful:

> + * We don't have a separate plan for each action, so the when
> + * condition must be executed as a per-row check, making it very
> + * similar to a CHECK constraint and so we adopt the same semantics
> + * for that.

Why explain it that way at all? There are two rels, unlike a check constraint.

* The first time I read this comment, it made me laugh:

> + /*
> + * First rule of MERGE club is we don't talk about rules
> + */

The joke has become significantly less funny since then, though. I'd
just say that MERGE doesn't support rules, as it's unclear how that
could work.

* This comment seems redundant, since pstate is always allocated with palloc0():

+ * Note: we assume that the pstate's p_rtable, p_joinlist, and p_namespace
+ * lists were initialized to NIL when the pstate was created.

make_parsestate() knows about this. This is widespread, normal
practice during parse analysis.

* Is this actually needed at all?:

> + /* In MERGE when and condition, no system column is allowed */
> + if (pstate->p_expr_kind == EXPR_KIND_MERGE_WHEN_AND &&
> + attnum < InvalidAttrNumber &&
> + !(attnum == TableOidAttributeNumber || attnum == ObjectIdAttributeNumber))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
> + errmsg("system column \"%s\" reference in WHEN AND condition is invalid",
> + colname),
> + parser_errposition(pstate, location)));

We're talking about the scantuple here. It's not like excluded.*.

* Are these checks really needed?:

> +void
> +rewriteTargetListMerge(Query *parsetree, Relation target_relation)
> +{
> + Var *var = NULL;
> + const char *attrname;
> + TargetEntry *tle;
> +
> + if (target_relation->rd_rel->relkind == RELKIND_RELATION ||
> + target_relation->rd_rel->relkind == RELKIND_MATVIEW ||
> + target_relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)

* I think that you should remove this debug include:

> index 3a02307bd9..ed4e857477 100644
> --- a/src/backend/parser/parse_clause.c
> +++ b/src/backend/parser/parse_clause.c
> @@ -31,6 +31,7 @@
> #include "commands/defrem.h"
> #include "nodes/makefuncs.h"
> #include "nodes/nodeFuncs.h"
> +#include "nodes/print.h"
> #include "optimizer/tlist.h"
> #include "optimizer/var.h"
> #include "parser/analyze.h"

It may be worth considering custom dprintf code within your .gdbinit
to do stuff like this automatically, without code changes that may end
up in the patch you post to the list. Here is one that I sometimes use
for tuplesort.c:

define print_tuplesort_counts
dprintf tuplesort_sort_memtuples, "memtupsize: %d, memtupcount:
%d\n", state->memtupsize, state->memtupcount
end

This is way easier than adding custom printf style debug code, since
it doesn't require that you rebuild. It would be safe to add a similar
dprintf that called pprint() or similar when some function is reached.

* find_mergetarget_parents() and find_mergetarget_for_rel() could both
use at least a one-liner header comment.

* This analyze.c comment, which is in transformMergeStmt(), seems
pretty questionable:

> + /*
> + * Simplify the MERGE query as much as possible
> + *
> + * These seem like things that could go into Optimizer, but they are
> + * semantic simplications rather than optimizations, per se.
> + *
> + * If there are no INSERT actions we won't be using the non-matching
> + * candidate rows for anything, so no need for an outer join. We do still
> + * need an inner join for UPDATE and DELETE actions.

This is talking about an ad-hoc form of join strength reduction. Yeah,
it's a semantic simplification, but that's generally true of join
strength reduction, which mostly exists because of bad ORMs that
sprinkle OUTER on top of queries indifferently. MERGE is hardly an
ideal candidate for join strength reduction, and I would just lose
this code entirely for Postgres v11.

* This sounds really brittle, and so doesn't make sense even as an aspiration:

> + * XXX if we were really keen we could look through the actionList and
> + * pull out common conditions, if there were no terminal clauses and put
> + * them into the main query as an early row filter but that seems like an
> + * atypical case and so checking for it would be likely to just be wasted
> + * effort.
> + */

Again, I suggest removing everything on join strength reduction.

* You should say *when* this happens (what later point):

> + *
> + * Track the RTE index of the target table used in the join query. This is
> + * later used to add required junk attributes to the targetlist.
> + */

* This seems unnecessary, as we don't say anything like it for ON
CONFLICT DO UPDATE:

> + * As top-level statements INSERT, UPDATE and DELETE have a Query, whereas
> + * with MERGE the individual actions do not require separate planning,
> + * only different handling in the executor. See nodeModifyTable handling
> + * of commandType CMD_MERGE.

* What does this mean?:

> + * A sub-query can include the Target, but otherwise the sub-query cannot
> + * reference the outermost Target table at all.

* I don't see the point in this:

> + * XXX Perhaps we require Parallel Safety since that is a superset of
> + * the restriction and enforcing that makes it easier to consider
> + * running MERGE plans in parallel in future.

* This references the now-removed executor WAL check thing, so you'll
need to remove it too:

> + * SQL Standard says we should not allow anything that possibly
> + * modifies SQL-data. We enforce that with an executor check that we
> + * have not written any WAL.

* This should be reworded:

> + * Note that we don't add this to the MERGE Query's quals because
> + * that's not the logic MERGE uses.
> + */
> + action->qual = transformWhereClause(pstate, action->condition,
> + EXPR_KIND_MERGE_WHEN_AND, "WHEN");

Perhaps say "WHEN ... AND quals are evaluated separately from the
MERGE statement's join quals" instead. Or just lose it altogether.

* This comment seems inaccurate:

> + /*
> + * Process INSERT ... VALUES with a single VALUES
> + * sublist. We treat this case separately for
> + * efficiency. The sublist is just computed directly
> + * as the Query's targetlist, with no VALUES RTE. So
> + * it works just like a SELECT without any FROM.
> + */

Wouldn't it be more accurate to say that this it totally different to
what transformInsertStmt() does for a SELECT without any FROM? Also,
do you think that that's good?

>> * Tests for transition table behavior (mixed INSERTs, UPDATEs, and
>> DELETEs) within triggers.sql seems like a good idea.
>
> Ok, I will add. But not done in this version.

Note that it's implied in at least one place that we don't support
transition tables at all:

> + /*
> + * XXX if we support transition tables this would need to move
> + * earlier before ExecSetupTransitionCaptureState()
> + */
> + switch (action->commandType)
> + {

You'll want to get to this as part of that transition table effort.

Any plan to fix this/support identity columns? I see that you don't
support them here:

> + /*
> + * Handle INSERT much like in transformInsertStmt
> + *
> + * XXX currently ignore stmt->override, if present
> + */

I think that this is a blocker, unfortunately.

>> * I still think we probably need to add something to ruleutils.c, so
>> that MERGE Query structs can be deparsed -- see get_query_def().
>
> Ok. I will look at it. Not done in this version though.

I also wonder what it would take to support referencing CTEs. Other
implementations do have this. Why can't we?

I do accept that RETURNING support and user-defined rule support are
not desirable, because it isn't at all clear what that means, and
there is no precedent for those from other database systems. CTEs and
identity columns are not in that same category, though.

Phew! Thanks for your patience and perseverance. I do have more
feedback on the docs lined up, but that isn't so important right now.

--
Peter Geoghegan