Re: [HACKERS] MERGE SQL Statement for PG11 - Mailing list pgsql-hackers

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().

* 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.

* Compiler warning:

fwrapv -fexcess-precision=standard -Og -g3 -fno-omit-frame-pointer
-I../../../src/include
-I/code/postgresql/root/build/../source/src/include
-D_FORTIFY_SOURCE=2 -D TRUST_STRXFRM -D LOCK_DEBUG -D WAL_DEBUG -D
BTREE_BUILD_STATS -D MULTIXACT_DEBUG -D SELECTIVITY_DEBUG -D HJDEBUG
-D_GNU_SOURCE -I/usr/include/libxml2   -c -o nodeModifyTable.o
/code/postgresql/root/build/../source/src/backend/executor/nodeModifyTable.c
/code/postgresql/root/build/../source/src/backend/executor/nodeModifyTable.c:
In function ‘ExecInsert’:
/code/postgresql/root/build/../source/src/backend/executor/nodeModifyTable.c:412:4:
warning: ‘wco_kind’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
    ExecWithCheckOptions(wco_kind, resultRelInfo, slot, estate);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

* The BufferIsValid() checks to decide if we need to ReleaseBuffer()
within ExecMergeMatched() are unnecessary -- a buffer pin must be held
throughout. This looks like it's leftover from before the
ExecMergeNotMatched()/ExecMergeMatched() split was made.

* There should be ResetExprContext() calls for your new
MergeActionState projections. That's what we see for the RETURNING +
ON CONFLICT projections within nodeModifyTable.c, which the new
projections are very similar to, and clearly modeled on.

--
Peter Geoghegan