Date: Fri, 16 Mar 2018 18:23:44 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Pavan Deolasee <pavan.deolasee@gmail.com>
Cc: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>,
	Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>,
	Pg Hackers <pgsql-hackers@postgresql.org>,
	Peter Geoghegan <pg@bowt.ie>, Andres Freund <andres@anarazel.de>
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
Message-ID: <20180316212344.e5hipioljibtx3xw@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20180316151303.rml2p5wffn3o6qy6@alvherre.pgsql>
User-Agent: NeoMutt/20170306-137-4415bd-dirty (1.8.0)
Precedence: bulk

Another thing I noticed is that the split of the ON CONFLICT slot and
its corresponding projection is pretty weird.  The projection is in
ResultRelInfo, but the slot itself is in ModifyTableState.  You can't
make the projection work without a corresponding slot initialized with
the correct descriptor, so splitting it this way doesn't make a lot of
sense to me.

(Now, TBH the split between resultRelInfo->ri_projectReturning and
ModifyTableState->ps.ps_ResultTupleSlot, which is the slot that the
returning project uses, doesn't make a lot of sense to me either; so
maybe there some reason that I'm just not seeing.  But I digress.)

So I want to propose that we move the slot to be together with the
projection node that it serves, ie. we put the slot in ResultRelInfo:

typedef struct ResultRelInfo
{
...
    /* for computing ON CONFLICT DO UPDATE SET */
    TupleTableSlot *ri_onConflictProjSlot;
    ProjectionInfo *ri_onConflictSetProj;

and with this the structure makes more sense.  So ExecInitModifyTable
does this

        /* create target slot for UPDATE SET projection */
        tupDesc = ExecTypeFromTL((List *) node->onConflictSet,
                                 relationDesc->tdhasoid);
        resultRelInfo->ri_onConflictProjSlot =
            ExecInitExtraTupleSlot(mtstate->ps.state, tupDesc);

        /* build UPDATE SET projection state */
        resultRelInfo->ri_onConflictSetProj =
            ExecBuildProjectionInfo(node->onConflictSet, econtext,
                                    resultRelInfo->ri_onConflictProjSlot,
                                    &mtstate->ps, relationDesc);

and then ExecOnConflictUpdate can simply do this:

    /* Project the new tuple version */
    ExecProject(resultRelInfo->ri_onConflictSetProj);

    /* Execute UPDATE with projection */
    *returning = ExecUpdate(mtstate, &tuple.t_self, NULL,
                            resultRelInfo->ri_onConflictProjSlot, planSlot,
                            &mtstate->mt_epqstate, mtstate->ps.state,
                            canSetTag);

Now, maybe there is some reason I'm missing for the on conflict slot for
the projection to be in ModifyTableState rather than resultRelInfo.  But
this code passes all current tests, so I don't know what that reason
would be.


Overall, the resulting code looks simpler to me than the previous
arrangements.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services