Re: [HACKERS] MERGE SQL Statement for PG11

On Wed, Mar 28, 2018 at 8:28 AM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:

> On Tue, Mar 27, 2018 at 2:28 AM, Pavan Deolasee
> <pavan(dot)deolasee(at)gmail(dot)com> wrote:
> > (Version 26)
>
> I have some feedback on this version:
>
> * ExecMergeMatched() needs to determine tuple lock mode for
> EvalPlanQual() in a way that's based on how everything else works;
> it's not okay to just use LockTupleExclusive in all cases. That will
> often lead to lock escalation, which can cause unprincipled deadlocks.
> You need to pass back the relevant info from routines like
> heap_update(), which means more state needs to come back to
> ExecMergeMatched() from routines like ExecUpdate().
>

You're right. I am thinking what would be a good way to pass that
information back. Should we add a new out parameter to ExecUpdate() or a
new member to HeapUpdateFailureData? It seems only ExecUpdate() would need
the change, so may be it's fine to change the API but HeapUpdateFailureData
doesn't look bad either since it deals with failure cases and we are indeed
dealing with ExecUpdate() failure. Any preference?

>
> * Doesn't ExecUpdateLockMode(), which is called from places like
> ExecBRUpdateTriggers(), also need to be taught about
> GetEPQRangeTableIndex() (i.e. the ri_mergeTargetRTI/ri_RangeTableIndex
> divide)? You should audit everything like that carefully. Maybe
> GetEPQRangeTableIndex() is not the best choke-point to do this kind of
> thing. Not that I have a clearly better idea.
>
> * Looks like there is a similar problem in
> ExecPartitionCheckEmitError(). I don't really understand how that
> works, so I might be wrong here.
>
> * More or less the same issue seems to exist within ExecConstraints(),
> including where GetInsertedColumns() is used.
>

They all look fine to me. Remember that we always resolve column references
in WHEN targetlist from the target relation and hence things like
updatedCols/insertedCols are get set on the target RTE. All of these
routines read from the target RTE as far as I can see. But I will check in
more detail.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services