Re: [HACKERS] MERGE SQL Statement for PG11 - Mailing list pgsql-hackers

Sure, ExecUpdate() and ExecDelete() *do* require a little special
handling for MERGE, but ExecMerge() should itself call EvalPlanQual()
for the join quals, rather than getting ExecUpdate()/ExecDelete() to
do it. All that ExecUpdate()/ExecDelete() need to tell ExecMerge() is
"you need to do your HeapTupleUpdated stuff". This not only clarifies
the new EPQ design -- it also lets you do that special case rti EPQ
stuff in the right place (BTW, I'm not a huge fan of that detail in
general, but haven't thought about it enough to say more). The new EPQ
code within ExecUpdate() and ExecDelete() is already identical, so why
not do it that way?

I especially like this organization because ExecOnConflictUpdate()
already calls ExecUpdate() without expecting it to do EPQ handling at
all, which is kind of similar to what I suggest -- in both cases, the
caller "takes care of all of the details of the scan". And, by
returning a HTSU_Result value to ExecMerge() from
ExecUpdate()/ExecDelete(), you could also do the new cardinality
violation stuff from within ExecMerge() in exactly one place -- no
need to invent new error_on_SelfUpdate arguments.

I would also think about organizing ExecMerge() handling so that
CMD_INSERT handling became WHEN NOT MATCHED handling. It's
unnecessarily confusing to have that included in the same switch
statement as CMD_UPDATE and CMD_DELETE, since that organization fails
to convey that once we reach WHEN NOT MATCHED, there is no going back.
Actually, I suppose the new EPQ organization described in the last few
paragraphs already implies that you'd do this CMD_INSERT stuff, since
the high level goal is breaking ExecMerge() into WHEN MATCHED and WHEN
NOT MATCHED sections (doing "commonality and variability analysis" for
CMD_UPDATE and CMD_DELETE only serves that high level goal).

Other feedback:

* Is a new ExecMaterializeSlot() call needed for the EPQ slot? Again,
doing your own EvalPlanQual() within ExecMerge() would perhaps have
avoided this.

* Do we really need UpdateStmt raw parser node pointers in structs
that appear in execnodes.h? What's that all about?

* Tests for transition table behavior (mixed INSERTs, UPDATEs, and
DELETEs) within triggers.sql seems like a good idea.

* Is this comment really accurate?:

> +               /*
> +                * If EvalPlanQual did not return a tuple, it means we
> +                * have seen a concurrent delete, or a concurrent update
> +                * where the row has moved to another partition.
> +                *
> +                * Set matched = false and loop back if there exists a NOT
> +                * MATCHED action. Otherwise, we have nothing to do for this
> +                * tuple and we can continue to the next tuple.
> +                */

Won't we loop back when a concurrent update occurs that makes the new
candidate tuple not satisfy the join qual anymore? What does this have
to do with partitioning?

* Why does MergeJoin get referenced here?:

> + * Also, since the internal MergeJoin node can hide the source and target
> + * relations, we must explicitly make the respective relation as visible so
> + * that columns can be referenced unqualified from these relations.

* This patch could use a pg_indent.

* We already heard about this code from Tomas, who found it unnecessary:

> +           /*
> +            * SQL Standard says that WHEN AND conditions must not
> +            * write to the database, so check we haven't written
> +            * any WAL during the test. Very sensible that is, since
> +            * we can end up evaluating some tests multiple times if
> +            * we have concurrent activity and complex WHEN clauses.
> +            *
> +            * XXX If we had some clear form of functional labelling
> +            * we could use that, if we trusted it.
> +            */
> +           if (startWAL < GetXactWALBytes())
> +               ereport(ERROR,
> +                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                        errmsg("cannot write to database within WHEN AND condition")));

This needs to go. Apart from the fact that GetXactWALBytes() is buggy
(it returns int64 for the unsigned type XLogRecPtr), the whole idea
just seems unnecessary. I don't see why this is any different to using
a volatile function in a regular UPDATE.

* I still think we probably need to add something to ruleutils.c, so
that MERGE Query structs can be deparsed -- see get_query_def().