Re: [HACKERS] MERGE SQL Statement for PG11

On 27 March 2018 at 10:28, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)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