On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera <alvherre@alvh.no-ip.org> 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