On 26 March 2018 at 15:39, Pavan Deolasee <pavan.deola...@gmail.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