Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Mar 26, 2018 at 9:36 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

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


>
> 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.
>

I've now added a separate counter to count all three actions and we also
report "Tuples skipped" which could be either because there was no action
to handle that source tuple or quals did not match. Added regression tests
specific to EXPLAIN ANALYZE.


>
> 3. sp. depedning trigger.sgml
>

Fixed.


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

Fixed.


>
> 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?


>
> 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?


>
> 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
>

Fixed.


>
> 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")
>

Agree, removed.


>
> 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.
>
>
Added a short para to merge.sgml


> 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.



> 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
>

Ok, fixed.


>
> 12. Missed out merge.sgml from v25 patch.
>

Ouch, added. Also generating a new patch which includes merge.sgml and
sending other improvements as add-ons.


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

Ok. Added a short para in create_policy.sgml


Thanks,
Pavan

-- 
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services