Re: support for MERGE - Mailing list pgsql-hackers

On Sat, 12 Mar 2022 at 11:53, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Mar-09, Zhihong Yu wrote:
>
>> Hi,
>> For v14-0002-Split-ExecUpdate-and-ExecDelete-in-reusable-piec.patch :
>>
>> +   TupleTableSlot *insertProjectedTuple;
>>
>> With `insert` at the beginning of the variable name, someone may think it
>> is a verb. How about naming the variable projectedTupleFromInsert (or
>> something similar) ?
>
> I went with projectedFromInsert.  I don't feel a need to call it a
> "tuple" because it's already a TupleTableSlot * ...
>
>> +       if (!ExecBRDeleteTriggers(context->estate, context->epqstate,
>> +                                 resultRelInfo, tupleid, oldtuple,
>> +                                 epqreturnslot))
>> +           /* some trigger made the delete a no-op; let caller know */
>> +           return false;
>>
>> It seems you can directly return the value returned
>> from ExecBRDeleteTriggers().
>
> True, let's do that.
>
> On 2022-Mar-10, Simon Riggs wrote:
>
>> Various cases were not tested in the patch - additional patch
>> attached, but nothing surprising there.
>
> Thanks, incorporated here.
>
> This is mostly just a rebase to keep cfbot happy.

Hi, hackers

+       ar_delete_trig_tcs = mtstate->mt_transition_capture;
+       if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture &&
+               mtstate->mt_transition_capture->tcs_update_old_table)
+       {
+               ExecARUpdateTriggers(estate, resultRelInfo, tupleid, oldtuple,
+                                                        NULL, NULL, mtstate->mt_transition_capture);
+
+               /*
+                * We've already captured the NEW TABLE row, so make sure any AR
+                * DELETE trigger fired below doesn't capture it again.
+                */
+               ar_delete_trig_tcs = NULL;
+       }

Maybe we can use ar_delete_trig_tcs in if condition and ExecARUpdateTriggers() to make the code shorter.


+       /* "old" transition table for DELETE, if any */
+       Tuplestorestate *old_del_tuplestore;
+       /* "new" transition table INSERT, if any */
+       Tuplestorestate *new_ins_tuplestore;

Should we add "for" for transition INSERT comment?

+MERGE is a multiple-table, multiple-action command: it specifies a target
+table and a source relation, and can contain multiple WHEN MATCHED and
+WHEN NOT MATCHED clauses, each of which specifies one UPDATE, INSERT,
+UPDATE, or DO NOTHING actions.  The target table is modified by MERGE,
+and the source relation supplies additional data for the actions

I think the "source relation" is inaccurate.  How about using "data source" like document?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.