Skip to content

Commit

Permalink
[pg15] fix: ON CONFLICT DO UPDATE related fixes
Browse files Browse the repository at this point in the history
Summary:
Fixes the following issues related to ON CONFLICT DO UPDATE clause:

- Issue 1: The commands:
```
create temporary table mytmp (id int primary key, name text, count int);
insert into mytmp values (1, 'foo', 0);
insert into mytmp values (1, 'foo') on conflict ON CONSTRAINT mytmp_pkey do update set id = mytmp.id+1;
```
throw
```
TRAP: FailedAssertion("ItemPointerIsValid(otid)", File: "../../../../../../../src/postgres/src/backend/access/heap/heapam.c", Line: 3216, PID: 44939)
0   postgres                            0x000000010112977c ExceptionalCondition + 280
1   postgres                            0x000000010077e868 heap_update + 316
2   postgres                            0x0000000100794c4c heapam_tuple_update + 168
3   postgres                            0x0000000100bb3580 table_tuple_update + 140
4   postgres                            0x0000000100bb2648 ExecUpdateAct + 620
5   postgres                            0x0000000100baee94 ExecUpdate + 744
6   postgres                            0x0000000100bb4a88 ExecOnConflictUpdate + 2680
```
This is because the merge commit D27766 incorrectly set `ybTid` to NULL  in `ExecOnConflictUpdate` for temporary relations. Bring back the old behavior for temporary relations.

- Issue 2: The above fix exposed another issue: insertion with ON CONFLICT clause into a temp table that has BEFORE ROW UPDATE triggers set fails at assertion `Assert(HeapTupleIsValid(fdw_trigtuple) ^ ItemPointerIsValid(tupleid))` in `ExecBRUpdateTriggers`. This is because in the pre-existing execution flow of function `ExecOnConflictUpdate`, `ybOldTuple ` is set for temporary relations. With the above-mentioned change, `ybTid` field is also set for such relations. But the assertion expects only one of them to be set. To fix the issue, do not set `ybOldTuple ` for temporary relations (the same behaviour as upstream PG). This issue should exist in the master branch as well, but I cannot verify it because inserting into a temp table with ON CONFLICT fails with `invalid memory alloc request size` there (GH #19909).

Also, do the following refactors:

- refactor 1: In function `ExecOnConflictUpdate`, replace `IsYBBackedRelation` with `IsYBRelation`. This does not have any run time implication because `IsYBBackedRelation` (in addition to when `IsYBRelation` is true) is true for views on non-temporary relations. For an insertion with ON CONFLICT clause on such a view, `ExecOnConflictUpdate` will be called for the underlying base relation. This change just makes the YB relation check consistent throughout the function.

- refactor 2: Add 'yb' prefix to some YB-specific variables.

Test Plan: Jenkins: rebase: pg15, test regex: org.yb.pgsql.TestPgRegress.*

Reviewers: jason, tnayak

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D30062
  • Loading branch information
arpang committed Nov 14, 2023
1 parent 58016ef commit a87fee2
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 12 deletions.
2 changes: 1 addition & 1 deletion src/postgres/src/backend/executor/execIndexing.c
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,7 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
if (violationOK)
{
conflict = true;
if (IsYugaByteEnabled()) {
if (IsYBRelation(heap)) {
estate->yb_conflict_slot = existing_slot;
}
if (conflictTid)
Expand Down
22 changes: 11 additions & 11 deletions src/postgres/src/backend/executor/nodeModifyTable.c
Original file line number Diff line number Diff line change
Expand Up @@ -2988,8 +2988,8 @@ ExecOnConflictUpdate(ModifyTableContext *context,
ExprContext *econtext = mtstate->ps.ps_ExprContext;
Relation relation = resultRelInfo->ri_RelationDesc;
ExprState *onConflictSetWhere = resultRelInfo->ri_onConflict->oc_WhereClause;
HeapTuple oldtuple = NULL;
bool shouldFree = true;
HeapTuple ybOldTuple = NULL;
bool ybShouldFree = false;
TupleTableSlot *existing = resultRelInfo->ri_onConflict->oc_Existing;
TM_FailureData tmfd;
LockTupleMode lockmode;
Expand All @@ -3011,7 +3011,7 @@ ExecOnConflictUpdate(ModifyTableContext *context,
* However, YugaByte writes the conflict tuple including its "ybctid" to execution state "estate"
* and then frees the slot when done.
*/
if (IsYBBackedRelation(relation)) {
if (IsYBRelation(relation)) {
/* Initialize result without calling postgres. */
test = TM_Ok;
ItemPointerSetInvalid(&tmfd.ctid);
Expand Down Expand Up @@ -3139,13 +3139,13 @@ ExecOnConflictUpdate(ModifyTableContext *context,
* snapshot. This is in line with the way UPDATE deals with newer tuple
* versions.
*/
if (!IsYugaByteEnabled())
if (!IsYBRelation(relation))
ExecCheckTupleVisible(context->estate, relation, existing);
else
{
oldtuple = ExecFetchSlotHeapTuple(context->estate->yb_conflict_slot, true, &shouldFree);
ExecStoreBufferHeapTuple(oldtuple, existing, InvalidBuffer);
TABLETUPLE_YBCTID(context->planSlot) = HEAPTUPLE_YBCTID(oldtuple);
ybOldTuple = ExecFetchSlotHeapTuple(context->estate->yb_conflict_slot, true, &ybShouldFree);
ExecStoreBufferHeapTuple(ybOldTuple, existing, InvalidBuffer);
TABLETUPLE_YBCTID(context->planSlot) = HEAPTUPLE_YBCTID(ybOldTuple);
}

/*
Expand Down Expand Up @@ -3200,15 +3200,15 @@ ExecOnConflictUpdate(ModifyTableContext *context,
* wCTE in the ON CONFLICT's SET.
*/

ItemPointer tid = IsYugaByteEnabled() ? NULL : conflictTid;
ItemPointer ybTid = IsYBRelation(relation) ? NULL : conflictTid;
/* Execute UPDATE with projection */
*returning = ExecUpdate(context, resultRelInfo,
tid, oldtuple,
ybTid, ybOldTuple,
resultRelInfo->ri_onConflict->oc_ProjSlot,
canSetTag);

if (shouldFree)
pfree(oldtuple);
if (ybShouldFree)
pfree(ybOldTuple);

/*
* Clear out existing tuple, as there might not be another conflict among
Expand Down
34 changes: 34 additions & 0 deletions src/postgres/src/test/regress/expected/yb_pg15.out
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,40 @@ SELECT * FROM tlateral1 t1 LEFT JOIN LATERAL (SELECT t2.a AS t2a, t2.c AS t2c, t
550 | 0 | 0002 | | | | |
(12 rows)

-- Insert with on conflict on temp table
create temporary table mytmp (id int primary key, name text, count int);
insert into mytmp values (1, 'foo', 0);
insert into mytmp values (1, 'foo') on conflict ON CONSTRAINT mytmp_pkey do update set id = mytmp.id+1;
select * from mytmp;
id | name | count
----+------+-------
2 | foo | 0
(1 row)

CREATE OR REPLACE FUNCTION update_count() RETURNS trigger LANGUAGE plpgsql AS
$func$
BEGIN
NEW.count := NEW.count+1;
RETURN NEW;
END
$func$;
CREATE TRIGGER update_count_trig BEFORE UPDATE ON mytmp FOR ROW EXECUTE PROCEDURE update_count();
insert into mytmp values (2, 'foo') on conflict ON CONSTRAINT mytmp_pkey do update set id = mytmp.id+1;
select * from mytmp;
id | name | count
----+------+-------
3 | foo | 1
(1 row)

create view myview as select * from mytmp;
NOTICE: view "myview" will be a temporary view
insert into myview values (3, 'foo') on conflict (id) do update set id = myview.id + 1;
select * from myview;
id | name | count
----+------+-------
4 | foo | 2
(1 row)

-- Cleanup
DROP TABLE IF EXISTS address, address2, emp, emp2, emp_par1, emp_par1_1_100, emp_par2, emp_par3,
fastpath, myemp, myemp2, myemp2_101_200, myemp2_1_100, p1, p2, pk_range_int_asc,
Expand Down
23 changes: 23 additions & 0 deletions src/postgres/src/test/regress/sql/yb_pg15.sql
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,29 @@ ANALYZE tlateral1, tlateral2;
-- YB_TODO: pg15 used merge join, whereas hash join is expected.
-- EXPLAIN (COSTS FALSE) SELECT * FROM tlateral1 t1 LEFT JOIN LATERAL (SELECT t2.a AS t2a, t2.c AS t2c, t2.b AS t2b, t3.b AS t3b, least(t1.a,t2.a,t3.b) FROM tlateral1 t2 JOIN tlateral2 t3 ON (t2.a = t3.b AND t2.c = t3.c)) ss ON t1.a = ss.t2a WHERE t1.b = 0 ORDER BY t1.a;
SELECT * FROM tlateral1 t1 LEFT JOIN LATERAL (SELECT t2.a AS t2a, t2.c AS t2c, t2.b AS t2b, t3.b AS t3b, least(t1.a,t2.a,t3.b) FROM tlateral1 t2 JOIN tlateral2 t3 ON (t2.a = t3.b AND t2.c = t3.c)) ss ON t1.a = ss.t2a WHERE t1.b = 0 ORDER BY t1.a;

-- Insert with on conflict on temp table
create temporary table mytmp (id int primary key, name text, count int);
insert into mytmp values (1, 'foo', 0);
insert into mytmp values (1, 'foo') on conflict ON CONSTRAINT mytmp_pkey do update set id = mytmp.id+1;
select * from mytmp;

CREATE OR REPLACE FUNCTION update_count() RETURNS trigger LANGUAGE plpgsql AS
$func$
BEGIN
NEW.count := NEW.count+1;
RETURN NEW;
END
$func$;

CREATE TRIGGER update_count_trig BEFORE UPDATE ON mytmp FOR ROW EXECUTE PROCEDURE update_count();
insert into mytmp values (2, 'foo') on conflict ON CONSTRAINT mytmp_pkey do update set id = mytmp.id+1;
select * from mytmp;

create view myview as select * from mytmp;
insert into myview values (3, 'foo') on conflict (id) do update set id = myview.id + 1;
select * from myview;

-- Cleanup
DROP TABLE IF EXISTS address, address2, emp, emp2, emp_par1, emp_par1_1_100, emp_par2, emp_par3,
fastpath, myemp, myemp2, myemp2_101_200, myemp2_1_100, p1, p2, pk_range_int_asc,
Expand Down

0 comments on commit a87fee2

Please sign in to comment.