public inbox for [email protected]  

help / color / mirror / Atom feed

From: Amit Langote <[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: Justin Pryzby <[email protected]>
Cc: Japin Li <[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:36:07 +0900
Message-ID: <CA+HiwqFSYmiRVhUE0_WwHfBSD+1du6QCXZ_O9RTTKfypcOYraA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>

On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera <[email protected]> wrote:
> FYI I intend to get the ModifyTable split patch (0001+0003) pushed
> hopefully on Tuesday or Wednesday next week, unless something really
> ugly is found on it.

I looked at 0001 and found a few things that could perhaps be improved.

+   /* Slot containing tuple obtained from subplan, for RETURNING */
+   TupleTableSlot *planSlot;

I think planSlot is also used by an FDW to look up any FDW-specific
junk attributes that the FDW's scan method would have injected into
the slot, so maybe no need to specifically mention only RETURNING here
as what cares about it.

+   /*
+    * The tuple produced by EvalPlanQual to retry from, if a cross-partition
+    * UPDATE requires it
+    */
+   TupleTableSlot *retrySlot;

Maybe a bit long name, though how about calling this updateRetrySlot
to make it apparent that what is to be retried with the slot is the
whole UPDATE operation, not some sub-operation (like say the
cross-partition portion)?

+   /*
+    * The tuple projected by the INSERT's RETURNING clause, when doing a
+    * cross-partition UPDATE
+    */
+   TupleTableSlot *projectedFromInsert;

I'd have gone with cpUpdateReturningSlot here, because that is what it
looks to me to be.  The fact that it is ExecInsert() that computes the
RETURNING targetlist projection seems like an implementation detail.

I know you said you don't like having "Slot" in the name, but then we
also have retrySlot. :)

+/*
+ * Context struct containing output data specific to UPDATE operations.
+ */
+typedef struct UpdateContext
+{
+   bool        updateIndexes;  /* index update required? */
+   bool        crossPartUpdate;    /* was it a cross-partition update? */
+} UpdateContext;

I wonder if it isn't more readable to just have these two be the
output arguments of ExecUpdateAct()?

+redo_act:
+       /* XXX reinit ->crossPartUpdate here? */

Why not just make ExecUpdateAct() always set the flag, not only when a
cross-partition update does occur?

Will look some more in the morning tomorrow.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

view thread (4+ 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: <CA+HiwqFSYmiRVhUE0_WwHfBSD+1du6QCXZ_O9RTTKfypcOYraA@mail.gmail.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