On 27 March 2018 at 10:28, Pavan Deolasee <pavan.deola...@gmail.com> wrote:
>> 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() > > AFAICS this is fine because we invoke that code only when > HeapTupleSatisfiesUpdate returns HeapTupleSelfUpdated i.e. for the case when > the tuple is updated by our transaction after the scan is started. > HeapTupleSatisfiesUpdate already checks for command id before returning > HeapTupleSelfUpdated. Cool. >> 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? > > I didn't get this point. Can you please explain? The code confused me at that point. More docs pls. >> 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? > > > Can that happen? I mean shouldn't there always be a source relation? It > could be a subquery or a function scan or just a plain relation, but > something, right? Yes, OK. So ordering is target, source, join. >> 10. Comment needs updating for changes in code below it - "In MERGE >> when and condition, no system column is allowed" >> > > Yeah, that's kinda half-true since the code below supports TABLEOID and OID > system columns. I am thinking about this in a larger context though. Peter > has expressed desire to support system columns in WHEN targetlist and quals. > I gave it a try and it seems if we remove that error block, all system > columns are supported readily. But only from the target side. There is a > problem if we try to refer a system column from the source side since the > mergrSourceTargetList only includes user columns and so set_plan_refs() > complains about a system column. > > I am not sure what's the best way to handle this. May be we can add system > columns to the mergrSourceTargetList. I haven't yet found a neat way to do > that. I was saying the comment needs changing, not the code. Cool, thanks -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services