Re: [HACKERS] MERGE SQL Statement for PG11

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

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

Fixed by adding a new member to HeapUpdateFailureData. That seemed more
natural, but we can change if others disagree.

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

As I said, I do not see a problem with this. The insertedCols/updatedCols
etc are tracked in the target table's RTE and hence we should continue to
use ri_RangeTableIndex for such purposes. The ri_mergeTargetRTI is only
useful to running EvalPlanQual correctly.

>
> * 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);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>

Fixed.

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

Fixed.

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

Right. I thought about what would the right place to call ResetExprContext()
and chose to do that in ExecMerge(). I am actually a bit surprised
that ExecProcessReturning() and ExecOnConflictUpdate() call ResetExprContext().
Aren't they both using mtstate->ps.ps_ExprContext? If so, why is it safe to
reset the expression context before we are completely done with the
previous tuple? Clearly, the current code is working fine, so may be there
is no bug, but it looks curious.

On Tue, Mar 27, 2018 at 4:16 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

>
> More comments on v26
>
> * Change errmsg “Ensure that not more than one source rows match any
> one target row”
> should be
> “Ensure that not more than one source row matches any one target row”
>

Fixed.

>
> * I think we need an example in the docs showing a constant source
> query, so people can understand how to use MERGE for OLTP as well as
> large ELT
>
> * Long comment in ExecMerge() needs rewording, formatting and spell check
>

Tried. Please check and suggest improvements, if any.

> I suggest not referring to an "order" since that concept doesn't exist
> anywhere else
>

You mean when we say that the actions are evaluated "in an order"?

>
> * Need tests for coverage of these ERROR messages
> Named security policy violation
>

Surprisingly none exists even for regular UPDATE/INSERT/DELETE. I will see
what is needed to trigger that error message.

> SELECT not allowed in MERGE INSERT...
> Multiple VALUES clauses not...
> MERGE is not supported for this...
> MERGE is not supported for relations with inheritance
> MERGE is not supported for relations with rules

Added a test for each of those. v27 attached, though review changes are in
the add-on 0005 patch.

Apart from that

- ran the patch through spell-checker and fixed a few typos
- reorganised the code in 0004 a bit.
- rebased on current master

Thanks,
Pavan

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