Skip to content

Commit

Permalink
[#23429] YSQL: fix INSERT ON CONFLICT TupleDesc ref leak
Browse files Browse the repository at this point in the history
Summary:
Nested INSERT ON CONFLICT causes TupleDesc reference leak warning.  The
cause is obvious: both the outer and inner INSERT ON CONFLICT use the
same estate->yb_conflict_slot:

1. outer: set estate->yb_conflict_slot
2. inner: set estate->yb_conflict_slot
3. inner: free estate->yb_conflict_slot
4. outer: free estate->yb_conflict_slot

The slot allocated in (1) is not freed.

Fix by removing yb_conflict_slot and using local variable
ybConflictSlot and passing it through functions like PG's conflictTid.
In future PG versions, resultRelInfo is local to the node, so maybe this
can be put into resultRelInfo to reduce modifications to the functions
signatures and make future merges easier.

Add test coverage using examples from issue #6782.
Jira: DB-12350

Test Plan:
On Almalinux 8:

    ./yb_build.sh fastdebug --gcc11 --java-test TestPgRegressPgMisc
    ./yb_build.sh fastdebug --gcc11 --java-test 'TestPgRegressMisc#testPgRegressMiscSerial4'

Close: #23429
Depends on D37104

Reviewers: kramanathan, aagrawal

Reviewed By: aagrawal

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D37151
  • Loading branch information
jaki committed Aug 9, 2024
1 parent f69b08f commit efd4cb7
Show file tree
Hide file tree
Showing 10 changed files with 189 additions and 38 deletions.
37 changes: 24 additions & 13 deletions src/postgres/src/backend/executor/execIndexing.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 ||
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -769,7 +772,8 @@ ExecCheckIndexConstraints(TupleTableSlot *slot,
indexInfo, &invalidItemPtr,
values, isnull, estate, false,
CEOUC_WAIT, true,
conflictTid);
conflictTid,
ybConflictSlot);
if (!satisfiesConstraint)
return false;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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 */);
}

/*
Expand Down
1 change: 0 additions & 1 deletion src/postgres/src/backend/executor/execUtils.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
31 changes: 17 additions & 14 deletions src/postgres/src/backend/executor/nodeModifyTable.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
{
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
3 changes: 2 additions & 1 deletion src/postgres/src/include/executor/executor.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 0 additions & 3 deletions src/postgres/src/include/nodes/execnodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/*
Expand Down
9 changes: 6 additions & 3 deletions src/postgres/src/test/regress/expected/yb_pg_with.out
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
Expand Down
92 changes: 92 additions & 0 deletions src/postgres/src/test/regress/expected/yb_with.out
Original file line number Diff line number Diff line change
@@ -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);
5 changes: 2 additions & 3 deletions src/postgres/src/test/regress/sql/yb_pg_with.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
45 changes: 45 additions & 0 deletions src/postgres/src/test/regress/sql/yb_with.sql
Original file line number Diff line number Diff line change
@@ -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);
Loading

0 comments on commit efd4cb7

Please sign in to comment.