Re: support for MERGE

On Fri, Nov 12, 2021 at 3:13 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

> On 2021-Nov-12, Zhihong Yu wrote:
>
> > Hi,
> >
> > + skipped_path = total - insert_path - update_path -
> delete_path;
> >
> > Should there be an assertion that skipped_path is not negative ?
>
> Hm, yeah, added.
>
> > + * We maintain separate transaction tables for UPDATE/INSERT/DELETE
> since
> > + * MERGE can run all three actions in a single statement. Note that
> UPDATE
> > + * needs both old and new transition tables
> >
> > Should the 'transaction' in the first line be transition ?
>
> Oh, of course.
>
> Uploaded fixup commits to
> https://github.com/alvherre/postgres/commits/merge-15
>
> --
> Álvaro Herrera Valdivia, Chile —
> https://www.EnterpriseDB.com/

Hi,

+ resultRelInfo->ri_notMatchedMergeAction = NIL;

ri_notMatchedMergeAction -> ri_unmatchedMergeAction

+static void ExecMergeNotMatched(ModifyTableState *mtstate,

ExecMergeNotMatched -> ExecMergeUnmatched

+ * In this case, we are still dealing with a WHEN MATCHED case.
+ * In this case, we recheck the list of WHEN MATCHED actions from

It seems the comment can be simplified to:

+ * In this case, since we are still dealing with a WHEN MATCHED case,
+ * we recheck the list of WHEN MATCHED actions from

+ * If we got no tuple, or the tuple we get
has

'get' appears in different tenses. Better use either 'get' or 'got' in both
places.

+lmerge_matched:
...
+ foreach(l, resultRelInfo->ri_matchedMergeAction)

I suggest expanding the foreach macro into the form of for loop where the
loop condition has extra boolean variable merge_matched.
Initial value for merge_matched can be true.
Inside the loop, we can adjust merge_matched's value to control whether the
for loop continues.
This would avoid using goto label.

+ if (commandType == CMD_UPDATE && tuple_updated)

Since commandType can only be update or delete, it seems tuple_updated
and tuple_deleted can be consolidated into one boolean variable
(tuple_modified).
The above point is personal preference.

+ * We've activated one of the WHEN clauses, so we don't search
+ * further. This is required behaviour, not an optimization.
+ */
+ break;

We can directly return instead of break'ing.

+ * Similar logic appears in ExecInitPartitionInfo(), so if changing
+ * anything here, do so there too.

The above implies code dedup via refactoring - can be done in a separate
patch.

To be continued ...