Re: MERGE ... RETURNING

On Sun, 28 Jan 2024 at 23:50, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> one minor white space issue:
>
> git diff --check
> doc/src/sgml/func.sgml:22482: trailing whitespace.
> + action | clause_number | product_id | in_stock | quantity
>

Ah, well spotted! I'm not in the habit of running git diff --check.

> @@ -3838,7 +3904,7 @@ ExecModifyTable(PlanState *pstate)
> }
> slot = ExecGetUpdateNewTuple(resultRelInfo, context.planSlot,
> oldSlot);
> - context.relaction = NULL;
> + node->mt_merge_action = NULL;
>
> I wonder what's the purpose of setting node->mt_merge_action to null ?
> I add `node->mt_merge_action = NULL;` at the end of each branch in
> `switch (operation)`.
> All the tests still passed.

Good question. It was necessary to set it to NULL there, because code
under ExecUpdate() reads it, and context.relaction would otherwise be
uninitialised. Now though, mtstate->mt_merge_action is automatically
initialised to NULL when the ModifyTableState node is first built, and
only the MERGE code sets it to non-NULL, so it's no longer necessary
to set it to NULL for other types of operation, because it will never
become non-NULL unless mtstate->operation is CMD_MERGE. So we can
safely remove that line.

Having said that, it seems a bit ugly to be relying on mt_merge_action
in so many places anyway. The places that test if it's non-NULL should
more logically be testing whether mtstate->operation is CMD_MERGE.
Doing that, reduces the number of places in nodeModifyTable.c that
read mt_merge_action down to one, and that one place only reads it
after testing that mtstate->operation is CMD_MERGE, which seems neater
and safer.

Regards,
Dean