Re: [HACKERS] MERGE SQL Statement for PG11

On 26 March 2018 at 15:39, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> wrote:

> reviewer

1. In ExecMergeMatched() we have a line of code that does this...

if (TransactionIdIsCurrentTransactionId(hufd.xmax))
then errcode(ERRCODE_CARDINALITY_VIOLATION)

I notice this is correct, but too strong. It should be possible to run
a sequence of MERGE commands inside a transaction, repeatedly updating
the same set of rows, as is possible with UPDATE.

We need to check whether the xid is the current subxid and the cid is
the current commandid, rather than using
TransactionIdIsCurrentTransactionId()

On further analysis, I note that ON CONFLICT suffers from this problem
as well, looks like I just refactored it from there.

2. EXPLAIN ANALYZE looks unchanged from some time back. The math is
only correct as long as there are zero rows that do not cause an
INS/UPD/DEL.
We don't test for that. I think this is a regression from an earlier
report of the same bug, or perhaps I didn't fix it at the time.

3. sp. depedning trigger.sgml

4. trigger.sgml replace "specific actions specified" with "events
specified in actions"
to avoid the double use of "specific"

5. I take it the special code for TransformMerge target relations is
replaced by "right_rte"? Seems fragile to leave it like that. Can we
add an Assert()? Do we care?

6. I didn't understand "Assume that the top-level join RTE is at the
end. The source relation
+ * is just before that."
What is there is no source relation?

7. The formatting of the SQL statement in transformMergeStmt that
begins "Construct a query of the form" is borked, so the SQL layout is
unclear, just needs pretty print

8. I didn't document why I thought this was true "XXX if we have a
constant subquery, we can also skip join", but one of the explain
analyze outputs shows this is already true - where we provide a
constant query and it skips the join. So perhaps we can remove the
comment. (Search for "Seq Scan on target t_1")

9. I think we need to mention that MERGE won't work with rules or
inheritance (only partitioning) in the main doc page. The current
text says that rules are ignored, which would be true if we didn't
specifically throw ERROR feature not supported.

10. Comment needs updating for changes in code below it - "In MERGE
when and condition, no system column is allowed"

11. In comment "Since the plan re-evaluated by EvalPlanQual uses the
second RTE", suggest using "join RTE" to make it more explicit which
RTE we are discussing

12. Missed out merge.sgml from v25 patch.

13. For triggers we say "No separate triggers are defined for
<command>MERGE</command>"
we should also state the same caveat for POLICY events

That's all I can see so far.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services