diff --git a/src/postgres/src/backend/executor/execIndexing.c b/src/postgres/src/backend/executor/execIndexing.c index 2f7f28d4a1fc..432eceac3bcc 100644 --- a/src/postgres/src/backend/executor/execIndexing.c +++ b/src/postgres/src/backend/executor/execIndexing.c @@ -131,7 +131,8 @@ static bool check_exclusion_or_unique_constraint(Relation heap, Relation index, EState *estate, bool newIndex, CEOUC_WAIT_MODE waitMode, bool errorOK, - ItemPointer conflictTid); + ItemPointer conflictTid, + TupleTableSlot **ybConflictSlot); static bool index_recheck_constraint(Relation index, Oid *constr_procs, Datum *existing_values, bool *existing_isnull, @@ -469,7 +470,8 @@ ExecInsertIndexTuplesOptimized(TupleTableSlot *slot, indexRelation, indexInfo, &(tuple->t_self), values, isnull, estate, false, - waitMode, violationOK, NULL); + waitMode, violationOK, NULL, + NULL /* ybConflictSlot */); } if ((checkUnique == UNIQUE_CHECK_PARTIAL || @@ -659,7 +661,8 @@ ExecDeleteIndexTuplesOptimized(Datum ybctid, bool ExecCheckIndexConstraints(TupleTableSlot *slot, EState *estate, ItemPointer conflictTid, - List *arbiterIndexes) + List *arbiterIndexes, + TupleTableSlot **ybConflictSlot) { ResultRelInfo *resultRelInfo; int i; @@ -769,7 +772,8 @@ ExecCheckIndexConstraints(TupleTableSlot *slot, indexInfo, &invalidItemPtr, values, isnull, estate, false, CEOUC_WAIT, true, - conflictTid); + conflictTid, + ybConflictSlot); if (!satisfiesConstraint) return false; } @@ -830,7 +834,8 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index, EState *estate, bool newIndex, CEOUC_WAIT_MODE waitMode, bool violationOK, - ItemPointer conflictTid) + ItemPointer conflictTid, + TupleTableSlot **ybConflictSlot) { Oid *constr_procs; uint16 *constr_strats; @@ -898,10 +903,6 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index, econtext = GetPerTupleExprContext(estate); save_scantuple = econtext->ecxt_scantuple; econtext->ecxt_scantuple = existing_slot; - if (estate->yb_conflict_slot != NULL) { - ExecDropSingleTupleTableSlot(estate->yb_conflict_slot); - estate->yb_conflict_slot = NULL; - } /* * May have to restart scan from this point if a potential conflict is @@ -1003,7 +1004,8 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index, { conflict = true; if (IsYBRelation(heap)) { - estate->yb_conflict_slot = existing_slot; + Assert(!*ybConflictSlot); + *ybConflictSlot = existing_slot; } if (conflictTid) *conflictTid = tup->t_self; @@ -1048,9 +1050,17 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index, */ econtext->ecxt_scantuple = save_scantuple; - if (estate->yb_conflict_slot == NULL) { + /* + * YB: ordinarily, PG frees existing slot here. But for YB, we need it for + * the DO UPDATE part (PG only needs conflictTid which is not palloc'd). + * If ybConflictSlot is filled, we found a conflict and need to extend the + * memory lifetime till the DO UPDATE part is finished. The memory will be + * freed after that at the end of ExecInsert. + * TODO(jason): this is not necessary for DO NOTHING, so it could be freed + * here as a minor optimization in that case. + */ + if (!*ybConflictSlot) ExecDropSingleTupleTableSlot(existing_slot); - } return !conflict; } @@ -1070,7 +1080,8 @@ check_exclusion_constraint(Relation heap, Relation index, (void) check_exclusion_or_unique_constraint(heap, index, indexInfo, tupleid, values, isnull, estate, newIndex, - CEOUC_WAIT, false, NULL); + CEOUC_WAIT, false, NULL, + NULL /* ybConflictSlot */); } /* diff --git a/src/postgres/src/backend/executor/execUtils.c b/src/postgres/src/backend/executor/execUtils.c index 35ae2ebe89ec..df14713a7e12 100644 --- a/src/postgres/src/backend/executor/execUtils.c +++ b/src/postgres/src/backend/executor/execUtils.c @@ -175,7 +175,6 @@ CreateExecutorState(void) */ estate->yb_es_is_single_row_modify_txn = false; estate->yb_es_is_fk_check_disabled = false; - estate->yb_conflict_slot = NULL; estate->yb_es_in_txn_limit_ht_for_reads = 0; estate->yb_exec_params.limit_count = 0; diff --git a/src/postgres/src/backend/executor/nodeModifyTable.c b/src/postgres/src/backend/executor/nodeModifyTable.c index 7004b4962ced..d4a4b0b0a912 100644 --- a/src/postgres/src/backend/executor/nodeModifyTable.c +++ b/src/postgres/src/backend/executor/nodeModifyTable.c @@ -92,7 +92,8 @@ static bool ExecOnConflictUpdate(ModifyTableState *mtstate, TupleTableSlot *excludedSlot, EState *estate, bool canSetTag, - TupleTableSlot **returning); + TupleTableSlot **returning, + TupleTableSlot *ybConflictSlot); static TupleTableSlot *ExecPrepareTupleRouting(ModifyTableState *mtstate, EState *estate, PartitionTupleRouting *proute, @@ -337,11 +338,7 @@ ExecInsert(ModifyTableState *mtstate, ModifyTable *node = (ModifyTable *) mtstate->ps.plan; OnConflictAction onconflict = node->onConflictAction; - /* - * The attribute "yb_conflict_slot" is only used within ExecInsert. - * Initialize its value to NULL. - */ - estate->yb_conflict_slot = NULL; + TupleTableSlot *ybConflictSlot = NULL; /* * get the heap tuple out of the tuple table slot, making sure we have a @@ -513,7 +510,7 @@ ExecInsert(ModifyTableState *mtstate, vlock: specConflict = false; if (!ExecCheckIndexConstraints(slot, estate, &conflictTid, - arbiterIndexes)) + arbiterIndexes, &ybConflictSlot)) { /* committed conflict tuple found */ if (onconflict == ONCONFLICT_UPDATE) @@ -528,14 +525,18 @@ ExecInsert(ModifyTableState *mtstate, if (ExecOnConflictUpdate(mtstate, resultRelInfo, &conflictTid, planSlot, slot, - estate, canSetTag, &returning)) + estate, canSetTag, &returning, + ybConflictSlot)) { InstrCountTuples2(&mtstate->ps, 1); result = returning; goto conflict_resolved; } else + { + Assert(!IsYBRelation(resultRelationDesc)); goto vlock; + } } else { @@ -772,10 +773,10 @@ ExecInsert(ModifyTableState *mtstate, } conflict_resolved: - if (estate->yb_conflict_slot != NULL) { - ExecDropSingleTupleTableSlot(estate->yb_conflict_slot); - estate->yb_conflict_slot = NULL; - } + /* YB: UPDATE-or-not is done: the conflict slot is no longer needed */ + if (ybConflictSlot) + ExecDropSingleTupleTableSlot(ybConflictSlot); + return result; } @@ -1834,7 +1835,8 @@ ExecOnConflictUpdate(ModifyTableState *mtstate, TupleTableSlot *excludedSlot, EState *estate, bool canSetTag, - TupleTableSlot **returning) + TupleTableSlot **returning, + TupleTableSlot *ybConflictSlot) { ExprContext *econtext = mtstate->ps.ps_ExprContext; Relation relation = resultRelInfo->ri_RelationDesc; @@ -1966,7 +1968,8 @@ ExecOnConflictUpdate(ModifyTableState *mtstate, if (IsYBRelation(relation)) { - oldtuple = ExecMaterializeSlot(estate->yb_conflict_slot); + Assert(ybConflictSlot); + oldtuple = ExecMaterializeSlot(ybConflictSlot); ExecStoreBufferHeapTuple(oldtuple, mtstate->mt_existing, buffer); planSlot->tts_tuple->t_ybctid = oldtuple->t_ybctid; } diff --git a/src/postgres/src/include/executor/executor.h b/src/postgres/src/include/executor/executor.h index 8627b97e8543..2997c3c5afad 100644 --- a/src/postgres/src/include/executor/executor.h +++ b/src/postgres/src/include/executor/executor.h @@ -596,7 +596,8 @@ extern void ExecDeleteIndexTuples(Datum ybctid, HeapTuple tuple, EState *estate) extern void ExecDeleteIndexTuplesOptimized(Datum ybctid, HeapTuple tuple, EState *estate); extern bool ExecCheckIndexConstraints(TupleTableSlot *slot, EState *estate, - ItemPointer conflictTid, List *arbiterIndexes); + ItemPointer conflictTid, List *arbiterIndexes, + TupleTableSlot **ybConflictSlot); extern void check_exclusion_constraint(Relation heap, Relation index, IndexInfo *indexInfo, ItemPointer tupleid, diff --git a/src/postgres/src/include/nodes/execnodes.h b/src/postgres/src/include/nodes/execnodes.h index 6a747db6f96f..73cf0d542981 100644 --- a/src/postgres/src/include/nodes/execnodes.h +++ b/src/postgres/src/include/nodes/execnodes.h @@ -593,9 +593,6 @@ typedef struct EState bool yb_es_is_single_row_modify_txn; /* Is this query a single-row modify * and the only stmt in this txn. */ bool yb_es_is_fk_check_disabled; /* Is FK check disabled? */ - TupleTableSlot *yb_conflict_slot; /* If a conflict is to be resolved when inserting data, - * we cache the conflict tuple here when processing and - * then free the slot after the conflict is resolved. */ YBCPgExecParameters yb_exec_params; /* diff --git a/src/postgres/src/test/regress/expected/yb_pg_with.out b/src/postgres/src/test/regress/expected/yb_pg_with.out index ffc73412d39e..f32fe492811d 100644 --- a/src/postgres/src/test/regress/expected/yb_pg_with.out +++ b/src/postgres/src/test/regress/expected/yb_pg_with.out @@ -1899,7 +1899,6 @@ ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 'a' L WITH aa AS (SELECT 1 a, 2 b) INSERT INTO withz VALUES(1, (SELECT b || ' insert' FROM aa WHERE a = 1 )) ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 1 LIMIT 1); -/* Disable the below test until #6782 is fixed -- Update a row more than once, in different parts of a wCTE. That is -- an allowed, presumably very rare, edge case, but since it was -- broken in the past, having a test seems worthwhile. @@ -1911,8 +1910,12 @@ upsert_cte AS ( RETURNING k, v) INSERT INTO withz VALUES(2, 'Red') ON CONFLICT (k) DO UPDATE SET (k, v) = (SELECT k, v FROM upsert_cte WHERE upsert_cte.k = withz.k) -RETURNING k, v; -*/ +RETURNING k, v; -- YB: output is different from PG (see #6782) + k | v +---+------- + 2 | Green +(1 row) + DROP TABLE withz; -- check that run to completion happens in proper ordering TRUNCATE TABLE y; diff --git a/src/postgres/src/test/regress/expected/yb_with.out b/src/postgres/src/test/regress/expected/yb_with.out new file mode 100644 index 000000000000..16a629953239 --- /dev/null +++ b/src/postgres/src/test/regress/expected/yb_with.out @@ -0,0 +1,92 @@ +/* Test A */ +drop table if exists a; +NOTICE: table "a" does not exist, skipping +drop table if exists b; +NOTICE: table "b" does not exist, skipping +create table a (i int unique); +create table b (i int unique); +insert into a values (1); +insert into b values (2); +EXPLAIN (costs off) +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +) insert into b values (2) on conflict on constraint b_i_key do update set i = (select 20 from w); + QUERY PLAN +--------------------------------------------- + Insert on b + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: b_i_key + CTE w + -> Insert on a + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: a_i_key + -> Result + InitPlan 2 (returns $2) + -> CTE Scan on w + -> Result +(11 rows) + +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +) insert into b values (2) on conflict on constraint b_i_key do update set i = (select 20 from w); +/* Test B */ +drop table if exists a; +create table a (i int unique); +insert into a values (1), (2); +EXPLAIN (costs off) +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +) insert into a values (2) on conflict on constraint a_i_key do update set i = (select 20 from w); + QUERY PLAN +--------------------------------------------- + Insert on a + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: a_i_key + CTE w + -> Insert on a a_1 + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: a_i_key + -> Result + InitPlan 2 (returns $2) + -> CTE Scan on w + -> Result +(11 rows) + +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +) insert into a values (2) on conflict on constraint a_i_key do update set i = (select 20 from w); +/* Test C */ +drop table if exists a; +create table a (i int unique); +insert into a values (1), (2), (3); +EXPLAIN (costs off) +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +), x(i) as ( + insert into a values (2) on conflict on constraint a_i_key do update set i = 20 returning i +) insert into a values (3) on conflict on constraint a_i_key do update set i = (select 30 from w); + QUERY PLAN +--------------------------------------------- + Insert on a + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: a_i_key + CTE w + -> Insert on a a_1 + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: a_i_key + -> Result + CTE x + -> Insert on a a_2 + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: a_i_key + -> Result + InitPlan 3 (returns $4) + -> CTE Scan on w + -> Result +(16 rows) + +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +), x(i) as ( + insert into a values (2) on conflict on constraint a_i_key do update set i = 20 returning i +) insert into a values (3) on conflict on constraint a_i_key do update set i = (select 30 from w); diff --git a/src/postgres/src/test/regress/sql/yb_pg_with.sql b/src/postgres/src/test/regress/sql/yb_pg_with.sql index 8405fdd9cdc6..7f42335ee076 100644 --- a/src/postgres/src/test/regress/sql/yb_pg_with.sql +++ b/src/postgres/src/test/regress/sql/yb_pg_with.sql @@ -869,7 +869,6 @@ WITH aa AS (SELECT 1 a, 2 b) INSERT INTO withz VALUES(1, (SELECT b || ' insert' FROM aa WHERE a = 1 )) ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 1 LIMIT 1); -/* Disable the below test until #6782 is fixed -- Update a row more than once, in different parts of a wCTE. That is -- an allowed, presumably very rare, edge case, but since it was -- broken in the past, having a test seems worthwhile. @@ -881,8 +880,8 @@ upsert_cte AS ( RETURNING k, v) INSERT INTO withz VALUES(2, 'Red') ON CONFLICT (k) DO UPDATE SET (k, v) = (SELECT k, v FROM upsert_cte WHERE upsert_cte.k = withz.k) -RETURNING k, v; -*/ +RETURNING k, v; -- YB: output is different from PG (see #6782) + DROP TABLE withz; -- check that run to completion happens in proper ordering diff --git a/src/postgres/src/test/regress/sql/yb_with.sql b/src/postgres/src/test/regress/sql/yb_with.sql new file mode 100644 index 000000000000..b0dac4e368b1 --- /dev/null +++ b/src/postgres/src/test/regress/sql/yb_with.sql @@ -0,0 +1,45 @@ +/* Test A */ +drop table if exists a; +drop table if exists b; +create table a (i int unique); +create table b (i int unique); +insert into a values (1); +insert into b values (2); + +EXPLAIN (costs off) +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +) insert into b values (2) on conflict on constraint b_i_key do update set i = (select 20 from w); +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +) insert into b values (2) on conflict on constraint b_i_key do update set i = (select 20 from w); + +/* Test B */ +drop table if exists a; +create table a (i int unique); +insert into a values (1), (2); + +EXPLAIN (costs off) +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +) insert into a values (2) on conflict on constraint a_i_key do update set i = (select 20 from w); +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +) insert into a values (2) on conflict on constraint a_i_key do update set i = (select 20 from w); + +/* Test C */ +drop table if exists a; +create table a (i int unique); +insert into a values (1), (2), (3); + +EXPLAIN (costs off) +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +), x(i) as ( + insert into a values (2) on conflict on constraint a_i_key do update set i = 20 returning i +) insert into a values (3) on conflict on constraint a_i_key do update set i = (select 30 from w); +with w(i) as ( + insert into a values (1) on conflict on constraint a_i_key do update set i = 10 returning i +), x(i) as ( + insert into a values (2) on conflict on constraint a_i_key do update set i = 20 returning i +) insert into a values (3) on conflict on constraint a_i_key do update set i = (select 30 from w); diff --git a/src/postgres/src/test/regress/yb_misc_serial4_schedule b/src/postgres/src/test/regress/yb_misc_serial4_schedule index 01c4b70eb8fe..b300680da738 100644 --- a/src/postgres/src/test/regress/yb_misc_serial4_schedule +++ b/src/postgres/src/test/regress/yb_misc_serial4_schedule @@ -6,3 +6,4 @@ test: yb_explicit_row_lock_planning test: yb_explicit_row_lock_batching test: yb_pg_fast_default +test: yb_with