Re: [HACKERS] MERGE SQL Statement for PG11 - Mailing list pgsql-hackers
| From | Pavan Deolasee |
|---|---|
| Subject | Re: [HACKERS] MERGE SQL Statement for PG11 |
| Date | |
| Msg-id | CABOikdOwA4+CJO4MwzF250huNuBWejWugf-GZwrbX-k=NDc2rw@mail.gmail.com Whole thread Raw |
| In response to | Re: [HACKERS] MERGE SQL Statement for PG11 (Simon Riggs <simon@2ndquadrant.com>) |
| Responses |
Re: [HACKERS] MERGE SQL Statement for PG11
(Simon Riggs <simon@2ndquadrant.com>)
Re: [HACKERS] MERGE SQL Statement for PG11 (Peter Geoghegan <pg@bowt.ie>) |
| List | pgsql-hackers |
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. HeapTupleSatisfiesUpd
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
Pavan
--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
PostgreSQL Development, 24x7 Support, Training & Services
Attachment
pgsql-hackers by date: