public inbox for [email protected]  

help / color / mirror / Atom feed

From: Japin Li <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Zhihong Yu <[email protected]>
Cc: Simon Riggs <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Daniel Westermann <[email protected]>
Cc: Amit Langote <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Jaime Casanova <[email protected]>
Cc: Andres Freund <[email protected]>
Subject: Re: support for MERGE
Date: Mon, 14 Mar 2022 23:16:11 +0800
Message-ID: <MEYP282MB166983FFEC67F51A72432617B60F9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <[email protected]>
References: <CANbhV-HgNB805QSikNTVSeFOV_jpfGokQpbboQjHntzo9auXRw@mail.gmail.com>
	<CALNJ-vTr=gR+jQNUAn68ozFf1eCpCc2599oo=bkEW_Rr5xo9WA@mail.gmail.com>
	<[email protected]>


On Sat, 12 Mar 2022 at 11:53, Alvaro Herrera <[email protected]> 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.

view thread (7+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: support for MERGE
  In-Reply-To: <MEYP282MB166983FFEC67F51A72432617B60F9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox