On Wed, Mar 28, 2018 at 8:28 AM, Peter Geoghegan <pg@bowt.ie> wrote:
On Tue, Mar 27, 2018 at 2:28 AM, Pavan Deolasee <pavan.deolasee@gmail.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.
