From 28c6a5aa7a3f1018738581489f7b03517850bf89 Mon Sep 17 00:00:00 2001 From: Pavan Deolasee Date: Wed, 28 Mar 2018 11:44:48 +0530 Subject: [PATCH v27 5/5] Fixes per review comments Use the correct lockmode while running EvalPlanQual Fix some typos matviews can't use MERGE, just like they can't use UPDATE/INSERTs Fix a compiler warning buffer can be released without checking BufferIsValid() Couple of new test cases to check ERROR conditions and some minor cleanup Call ResetExprContext() once per tuple processed --- src/backend/access/heap/heapam.c | 25 ++++++------- src/backend/executor/nodeMerge.c | 51 ++++++++++++++++----------- src/backend/executor/nodeModifyTable.c | 13 ++++--- src/backend/parser/parse_merge.c | 9 +++-- src/include/access/heapam.h | 12 ++++++- src/test/regress/expected/merge.out | 64 ++++++++++++++++++++++++++++++++-- src/test/regress/sql/merge.sql | 58 ++++++++++++++++++++++++++++++ 7 files changed, 184 insertions(+), 48 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index decc3d37c3..f96567f5d5 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3508,7 +3508,7 @@ simple_heap_delete(Relation relation, ItemPointer tid) HTSU_Result heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, CommandId cid, Snapshot crosscheck, bool wait, - HeapUpdateFailureData *hufd, LockTupleMode *lockmode) + HeapUpdateFailureData *hufd) { HTSU_Result result; TransactionId xid = GetCurrentTransactionId(); @@ -3548,8 +3548,10 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, infomask2_old_tuple, infomask_new_tuple, infomask2_new_tuple; + LockTupleMode lockmode; Assert(ItemPointerIsValid(otid)); + Assert(hufd != NULL); /* * Forbid this during a parallel operation, lest it allocate a combocid. @@ -3665,7 +3667,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, */ if (!bms_overlap(modified_attrs, key_attrs)) { - *lockmode = LockTupleNoKeyExclusive; + lockmode = hufd->lockmode = LockTupleNoKeyExclusive; mxact_status = MultiXactStatusNoKeyUpdate; key_intact = true; @@ -3682,7 +3684,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, } else { - *lockmode = LockTupleExclusive; + lockmode = hufd->lockmode = LockTupleExclusive; mxact_status = MultiXactStatusUpdate; key_intact = false; } @@ -3760,12 +3762,12 @@ l2: int remain; if (DoesMultiXactIdConflict((MultiXactId) xwait, infomask, - *lockmode)) + lockmode)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); /* acquire tuple lock, if necessary */ - heap_acquire_tuplock(relation, &(oldtup.t_self), *lockmode, + heap_acquire_tuplock(relation, &(oldtup.t_self), lockmode, LockWaitBlock, &have_tuple_lock); /* wait for multixact */ @@ -3849,7 +3851,7 @@ l2: * lock. */ LockBuffer(buffer, BUFFER_LOCK_UNLOCK); - heap_acquire_tuplock(relation, &(oldtup.t_self), *lockmode, + heap_acquire_tuplock(relation, &(oldtup.t_self), lockmode, LockWaitBlock, &have_tuple_lock); XactLockTableWait(xwait, relation, &oldtup.t_self, XLTW_Update); @@ -3897,7 +3899,7 @@ l2: hufd->cmax = InvalidCommandId; UnlockReleaseBuffer(buffer); if (have_tuple_lock) - UnlockTupleTuplock(relation, &(oldtup.t_self), *lockmode); + UnlockTupleTuplock(relation, &(oldtup.t_self), lockmode); if (vmbuffer != InvalidBuffer) ReleaseBuffer(vmbuffer); bms_free(hot_attrs); @@ -3935,7 +3937,7 @@ l2: compute_new_xmax_infomask(HeapTupleHeaderGetRawXmax(oldtup.t_data), oldtup.t_data->t_infomask, oldtup.t_data->t_infomask2, - xid, *lockmode, true, + xid, lockmode, true, &xmax_old_tuple, &infomask_old_tuple, &infomask2_old_tuple); @@ -4052,7 +4054,7 @@ l2: compute_new_xmax_infomask(HeapTupleHeaderGetRawXmax(oldtup.t_data), oldtup.t_data->t_infomask, oldtup.t_data->t_infomask2, - xid, *lockmode, false, + xid, lockmode, false, &xmax_lock_old_tuple, &infomask_lock_old_tuple, &infomask2_lock_old_tuple); @@ -4364,7 +4366,7 @@ l2: * Release the lmgr tuple lock, if we had it. */ if (have_tuple_lock) - UnlockTupleTuplock(relation, &(oldtup.t_self), *lockmode); + UnlockTupleTuplock(relation, &(oldtup.t_self), lockmode); pgstat_count_heap_update(relation, use_hot_update); @@ -4588,12 +4590,11 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup) { HTSU_Result result; HeapUpdateFailureData hufd; - LockTupleMode lockmode; result = heap_update(relation, otid, tup, GetCurrentCommandId(true), InvalidSnapshot, true /* wait for commit */ , - &hufd, &lockmode); + &hufd); switch (result) { case HeapTupleSelfUpdated: diff --git a/src/backend/executor/nodeMerge.c b/src/backend/executor/nodeMerge.c index ee41ed2eb2..67f1bfbb16 100644 --- a/src/backend/executor/nodeMerge.c +++ b/src/backend/executor/nodeMerge.c @@ -139,7 +139,6 @@ ExecMergeMatched(ModifyTableState *mtstate, EState *estate, lmerge_matched:; slot = saved_slot; - buffer = InvalidBuffer; /* * UPDATE/DELETE is only invoked for matched rows. And we must have found @@ -263,7 +262,7 @@ lmerge_matched:; ereport(ERROR, (errcode(ERRCODE_CARDINALITY_VIOLATION), errmsg("MERGE command cannot affect row a second time"), - errhint("Ensure that not more than one source rows match any one target row"))); + errhint("Ensure that not more than one source row match any one target row"))); /* This shouldn't happen */ elog(ERROR, "attempted to update or delete invisible tuple"); break; @@ -284,7 +283,7 @@ lmerge_matched:; * tuple. If EvalPlanQual() does not return a tuple (can * that happen?), then again we switch to NOT MATCHED * action. If it does return a tuple and the join qual is - * still satified, then we just need to recheck the + * still satisfied, then we just need to recheck the * MATCHED actions, starting from the top, and execute the * first qualifying action. */ @@ -335,8 +334,7 @@ lmerge_matched:; * newer tuple found in the update chain. */ *tupleid = hufd.ctid; - if (BufferIsValid(buffer)) - ReleaseBuffer(buffer); + ReleaseBuffer(buffer); goto lmerge_matched; } } @@ -348,8 +346,7 @@ lmerge_matched:; */ *tupleid = hufd.ctid; estate->es_result_relation_info = saved_resultRelInfo; - if (BufferIsValid(buffer)) - ReleaseBuffer(buffer); + ReleaseBuffer(buffer); return false; default: @@ -365,14 +362,13 @@ lmerge_matched:; /* * We've activated one of the WHEN clauses, so we don't search - * further. This is required behaviour, not an optimisation. + * further. This is required behaviour, not an optimization. */ estate->es_result_relation_info = saved_resultRelInfo; break; } - if (BufferIsValid(buffer)) - ReleaseBuffer(buffer); + ReleaseBuffer(buffer); /* * Successfully executed an action or no qualifying action was found. @@ -403,7 +399,7 @@ ExecMergeNotMatched(ModifyTableState *mtstate, EState *estate, resultRelInfo = mtstate->resultRelInfo; /* - * For INSERT actions, root relation's merge action is OK since the the + * For INSERT actions, root relation's merge action is OK since the * INSERT's targetlist and the WHEN conditions can only refer to the * source relation and hence it does not matter which result relation we * work with. @@ -484,6 +480,7 @@ void ExecMerge(ModifyTableState *mtstate, EState *estate, TupleTableSlot *slot, JunkFilter *junkfilter, ResultRelInfo *resultRelInfo) { + ExprContext *econtext = mtstate->ps.ps_ExprContext; ItemPointer tupleid; ItemPointerData tuple_ctid; bool matched = false; @@ -493,9 +490,14 @@ ExecMerge(ModifyTableState *mtstate, EState *estate, TupleTableSlot *slot, relkind = resultRelInfo->ri_RelationDesc->rd_rel->relkind; Assert(relkind == RELKIND_RELATION || - relkind == RELKIND_MATVIEW || relkind == RELKIND_PARTITIONED_TABLE); + /* + * Reset per-tuple memory context to free any expression evaluation + * storage allocated in the previous cycle. + */ + ResetExprContext(econtext); + /* * We run a JOIN between the target relation and the source relation to * find a set of candidate source rows that has matching row in the target @@ -523,12 +525,12 @@ ExecMerge(ModifyTableState *mtstate, EState *estate, TupleTableSlot *slot, /* * If we are dealing with a WHEN MATCHED case, we look at the given WHEN - * MATCHED actions in an order and execute the first action which also - * satisfies the additional WHEN MATCHED AND quals. If an action without - * any additional quals is found, that action is executed. + * MATCHED actions in an order and execute the first action for which + * the additional WHEN MATCHED AND quals pass. If an action without + * quals is found, that action is executed. * * Similarly, if we are dealing with WHEN NOT MATCHED case, we look at the - * given WHEN NOT MATCHED actions in an ordr and execute the first + * given WHEN NOT MATCHED actions in an order and execute the first * qualifying action. * * Things get interesting in case of concurrent update/delete of the @@ -538,9 +540,9 @@ ExecMerge(ModifyTableState *mtstate, EState *estate, TupleTableSlot *slot, * A concurrent update for example can: * * 1. modify the target tuple so that it no longer satisfies the - * additional quals attached to the current WHEN MATCHED action OR 2. - * modify the target tuple so that the join quals no longer pass and hence - * the source tuple no longer has a match. + * additional quals attached to the current WHEN MATCHED action OR + * 2. modify the target tuple so that the join quals no longer pass and + * hence the source tuple no longer has a match. * * In the first case, we are still dealing with a WHEN MATCHED case, but * we should recheck the list of WHEN MATCHED actions and choose the first @@ -559,7 +561,14 @@ ExecMerge(ModifyTableState *mtstate, EState *estate, TupleTableSlot *slot, * from ExecMergeNotMatched to ExecMergeMatched, there is no risk of a * livelock. */ - if (!matched || - !ExecMergeMatched(mtstate, estate, slot, junkfilter, tupleid)) + if (matched) + matched = ExecMergeMatched(mtstate, estate, slot, junkfilter, tupleid); + + /* + * Either we were dealing with a NOT MATCHED tuple or ExecMergeNotMatched() + * returned "false", indicating the previously MATCHED tuple is no longer a + * matching tuple. + */ + if (!matched) ExecMergeNotMatched(mtstate, estate, slot); } diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 08f812fc6d..5798eb12a5 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -398,11 +398,11 @@ ExecInsert(ModifyTableState *mtstate, */ if (mtstate->operation == CMD_UPDATE) wco_kind = WCO_RLS_UPDATE_CHECK; - else if (mtstate->operation == CMD_INSERT) - wco_kind = WCO_RLS_INSERT_CHECK; else if (mtstate->operation == CMD_MERGE) wco_kind = (actionState->commandType == CMD_UPDATE) ? WCO_RLS_UPDATE_CHECK : WCO_RLS_INSERT_CHECK; + else + wco_kind = WCO_RLS_INSERT_CHECK; /* * ExecWithCheckOptions() will skip any WCOs which are not of the kind @@ -1083,7 +1083,6 @@ ExecUpdate(ModifyTableState *mtstate, } else { - LockTupleMode lockmode; bool partition_constraint_failed; /* @@ -1204,7 +1203,7 @@ lreplace:; * retrieve the tuple conversion map for this resultRelInfo. * * If we're running MERGE then resultRelInfo is per-partition - * resultRelInfo as initialised in ExecInitPartitionInfo(). Note + * resultRelInfo as initialized in ExecInitPartitionInfo(). Note * that we don't expand inheritance for the resultRelation in case * of MERGE and hence there is just one subplan. Whereas for * regular UPDATE, resultRelInfo is one of the per-subplan @@ -1285,7 +1284,7 @@ lreplace:; estate->es_output_cid, estate->es_crosscheck_snapshot, true /* wait for commit */ , - &hufd, &lockmode); + &hufd); /* * Copy the necessary information, if the caller has asked for it. We @@ -1357,7 +1356,7 @@ lreplace:; epqstate, resultRelationDesc, GetEPQRangeTableIndex(resultRelInfo), - lockmode, + hufd.lockmode, &hufd.ctid, hufd.xmax); if (!TupIsNull(epqslot)) @@ -2684,7 +2683,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) mtstate->mt_partition_tuple_routing ? NULL : relationDesc); - /* initialise slot for merge actions */ + /* initialize slot for merge actions */ Assert(mtstate->mt_mergeproj == NULL); mtstate->mt_mergeproj = ExecInitExtraTupleSlot(mtstate->ps.state, diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c index 6df63439bb..25416a321c 100644 --- a/src/backend/parser/parse_merge.c +++ b/src/backend/parser/parse_merge.c @@ -83,7 +83,7 @@ transformMergeJoinClause(ParseState *pstate, Node *merge, * We created an internal join between the target and the source relation * to carry out the MERGE actions. Normally such an unaliased join hides * the joining relations, unless the column references are qualified. - * Also, any unqualified column refernces are resolved to the Join RTE, if + * Also, any unqualified column references are resolved to the Join RTE, if * there is a matching entry in the targetlist. But the way MERGE * execution is later setup, we expect all column references to resolve to * either the source or the target relation. Hence we must not add the @@ -351,7 +351,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt) * Simplify the MERGE query as much as possible * * These seem like things that could go into Optimizer, but they are - * semantic simplications rather than optimizations, per se. + * semantic simplifications rather than optimizations, per se. * * If there are no INSERT actions we won't be using the non-matching * candidate rows for anything, so no need for an outer join. We do still @@ -426,7 +426,6 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt) * XXX MERGE is unsupported in various cases */ if (!(pstate->p_target_relation->rd_rel->relkind == RELKIND_RELATION || - pstate->p_target_relation->rd_rel->relkind == RELKIND_MATVIEW || pstate->p_target_relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -463,7 +462,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt) /* * Set namespace for the specific action. This must be done before - * analysing the WHEN quals and the action targetlisst. + * analyzing the WHEN quals and the action targetlisst. */ setNamespaceForMergeAction(pstate, action); @@ -471,7 +470,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt) * Transform the when condition. * * Note that these quals are NOT added to the join quals; instead they - * are evaluated sepaartely during execution to decide which of the + * are evaluated separately during execution to decide which of the * WHEN MATCHED or WHEN NOT MATCHED actions to execute. */ action->qual = transformWhereClause(pstate, action->condition, diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 100174138d..608f50b061 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -53,17 +53,26 @@ typedef enum LockTupleMode * When heap_update, heap_delete, or heap_lock_tuple fail because the target * tuple is already outdated, they fill in this struct to provide information * to the caller about what happened. + * + * result is the result of HeapTupleSatisfiesUpdate, leading to the failure. + * It's set to HeapTupleMayBeUpdated when there is no failure. + * * ctid is the target's ctid link: it is the same as the target's TID if the * target was deleted, or the location of the replacement tuple if the target * was updated. + * * xmax is the outdating transaction's XID. If the caller wants to visit the * replacement tuple, it must check that this matches before believing the * replacement is really a match. + * * cmax is the outdating command's CID, but only when the failure code is * HeapTupleSelfUpdated (i.e., something in the current transaction outdated * the tuple); otherwise cmax is zero. (We make this restriction because * HeapTupleHeaderGetCmax doesn't work for tuples outdated in other * transactions.) + * + * lockmode is only relevant for callers of heap_update() and is the mode which + * the caller should use in case it needs to lock the updated tuple. */ typedef struct HeapUpdateFailureData { @@ -71,6 +80,7 @@ typedef struct HeapUpdateFailureData ItemPointerData ctid; TransactionId xmax; CommandId cmax; + LockTupleMode lockmode; } HeapUpdateFailureData; @@ -163,7 +173,7 @@ extern void heap_abort_speculative(Relation relation, HeapTuple tuple); extern HTSU_Result heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, CommandId cid, Snapshot crosscheck, bool wait, - HeapUpdateFailureData *hufd, LockTupleMode *lockmode); + HeapUpdateFailureData *hufd); extern HTSU_Result heap_lock_tuple(Relation relation, HeapTuple tuple, CommandId cid, LockTupleMode mode, LockWaitPolicy wait_policy, bool follow_update, diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out index 90f3177743..7546ed84f1 100644 --- a/src/test/regress/expected/merge.out +++ b/src/test/regress/expected/merge.out @@ -84,6 +84,24 @@ WHEN NOT MATCHED THEN ERROR: syntax error at or near "INTO" LINE 5: INSERT INTO target DEFAULT VALUES ^ +-- Multiple VALUES clause +MERGE INTO target t +USING source AS s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT VALUES (1,1), (2,2); +ERROR: Multiple VALUES clauses not allowed in MERGE INSERT statement +; +-- SELECT query for INSERT +MERGE INTO target t +USING source AS s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT SELECT (1, 1); +ERROR: syntax error at or near "SELECT" +LINE 5: INSERT SELECT (1, 1); + ^ +; -- NOT MATCHED/UPDATE MERGE INTO target t USING source AS s @@ -104,6 +122,48 @@ WHEN MATCHED THEN ERROR: syntax error at or near "target" LINE 5: UPDATE target SET balance = 0 ^ +-- unsupported relation types +-- view +CREATE VIEW tv AS SELECT * FROM target; +MERGE INTO tv t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; +ERROR: MERGE is not supported for this relation type +DROP VIEW tv; +-- materialized view +CREATE MATERIALIZED VIEW mv AS SELECT * FROM target; +MERGE INTO mv t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; +ERROR: MERGE is not supported for this relation type +DROP MATERIALIZED VIEW mv; +-- inherited table +CREATE TABLE inhp (tid int, balance int); +CREATE TABLE child1() INHERITS (inhp); +CREATE TABLE child2() INHERITS (child1); +MERGE INTO inhp t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; +ERROR: MERGE is not supported for relations with inheritance +MERGE INTO child1 t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; +ERROR: MERGE is not supported for relations with inheritance +-- this should be ok +MERGE INTO child2 t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; +DROP TABLE inhp, child1, child2; -- permissions MERGE INTO target USING source2 @@ -382,7 +442,7 @@ WHEN MATCHED THEN UPDATE SET balance = 0 ; ERROR: MERGE command cannot affect row a second time -HINT: Ensure that not more than one source rows match any one target row +HINT: Ensure that not more than one source row match any one target row ROLLBACK; BEGIN; MERGE INTO target t @@ -392,7 +452,7 @@ WHEN MATCHED THEN DELETE ; ERROR: MERGE command cannot affect row a second time -HINT: Ensure that not more than one source rows match any one target row +HINT: Ensure that not more than one source row match any one target row ROLLBACK; -- correct source data DELETE FROM source WHERE sid = 2; diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql index cd6144bb5f..db2f0acb86 100644 --- a/src/test/regress/sql/merge.sql +++ b/src/test/regress/sql/merge.sql @@ -63,6 +63,20 @@ ON t.tid = s.sid WHEN NOT MATCHED THEN INSERT INTO target DEFAULT VALUES ; +-- Multiple VALUES clause +MERGE INTO target t +USING source AS s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT VALUES (1,1), (2,2); +; +-- SELECT query for INSERT +MERGE INTO target t +USING source AS s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT SELECT (1, 1); +; -- NOT MATCHED/UPDATE MERGE INTO target t USING source AS s @@ -78,6 +92,50 @@ WHEN MATCHED THEN UPDATE target SET balance = 0 ; +-- unsupported relation types +-- view +CREATE VIEW tv AS SELECT * FROM target; +MERGE INTO tv t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; +DROP VIEW tv; + +-- materialized view +CREATE MATERIALIZED VIEW mv AS SELECT * FROM target; +MERGE INTO mv t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; +DROP MATERIALIZED VIEW mv; + +-- inherited table +CREATE TABLE inhp (tid int, balance int); +CREATE TABLE child1() INHERITS (inhp); +CREATE TABLE child2() INHERITS (child1); + +MERGE INTO inhp t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; + +MERGE INTO child1 t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; + +-- this should be ok +MERGE INTO child2 t +USING source s +ON t.tid = s.sid +WHEN NOT MATCHED THEN + INSERT DEFAULT VALUES; +DROP TABLE inhp, child1, child2; + -- permissions MERGE INTO target -- 2.14.3 (Apple Git-98)