Skip to content

Commit 7285d66

Browse files
anarazelanarazel
committed
Fix several bugs related to ON CONFLICT's EXCLUDED pseudo relation.
Four related issues: 1) attnos/varnos/resnos for EXCLUDED were out of sync when a column after one dropped in the underlying relation was referenced. 2) References to whole-row variables (i.e. EXCLUDED.*) lead to errors. 3) It was possible to reference system columns in the EXCLUDED pseudo relations, even though they would not have valid contents. 4) References to EXCLUDED were rewritten by the RLS machinery, as EXCLUDED was treated as if it were the underlying relation. To fix the first two issues, generate the excluded targetlist with dropped columns in mind and add an entry for whole row variables. Instead of unconditionally adding a wholerow entry we could pull up the expression if needed, but doing it unconditionally seems simpler. The wholerow entry is only really needed for ruleutils/EXPLAIN support anyway. The remaining two issues are addressed by changing the EXCLUDED RTE to have relkind = composite. That fits with EXCLUDED not actually being a real relation, and allows to treat it differently in the relevant places. scanRTEForColumn now skips looking up system columns when the RTE has a composite relkind; fireRIRrules() already had a corresponding check, thereby preventing RLS expansion on EXCLUDED. Also add tests for these issues, and improve a few comments around excluded handling in setrefs.c. Reported-By: Peter Geoghegan, Geoff Winkless Author: Andres Freund, Amit Langote, Peter Geoghegan Discussion: CAEzk6fdzJ3xYQZGbcuYM2rBd2BuDkUksmK=mY9UYYDugg_GgZg@mail.gmail.com, CAM3SWZS+CauzbiCEcg-GdE6K6ycHE_Bz6Ksszy8AoixcMHOmsA@mail.gmail.com Backpatch: 9.5, where ON CONFLICT was introduced
1 parent 0777a88 commit 7285d66

File tree

7 files changed

+283
-38
lines changed

7 files changed

+283
-38
lines changed

‎src/backend/optimizer/plan/setrefs.c

+17-12
Original file line numberDiff line numberDiff line change
@@ -1929,16 +1929,21 @@ search_indexed_tlist_for_sortgroupref(Node *node,
19291929
* relation target lists. Also perform opcode lookup and add
19301930
* regclass OIDs to root->glob->relationOids.
19311931
*
1932-
* This is used in two different scenarios: a normal join clause, where all
1933-
* the Vars in the clause *must* be replaced by OUTER_VAR or INNER_VAR
1934-
* references; and a RETURNING clause, which may contain both Vars of the
1935-
* target relation and Vars of other relations. In the latter case we want
1936-
* to replace the other-relation Vars by OUTER_VAR references, while leaving
1937-
* target Vars alone.
1938-
*
1939-
* For a normal join, acceptable_rel should be zero so that any failure to
1940-
* match a Var will be reported as an error. For the RETURNING case, pass
1941-
* inner_itlist = NULL and acceptable_rel = the ID of the target relation.
1932+
* This is used in three different scenarios:
1933+
* 1) a normal join clause, where all the Vars in the clause *must* be
1934+
* replaced by OUTER_VAR or INNER_VAR references. In this case
1935+
* acceptable_rel should be zero so that any failure to match a Var will be
1936+
* reported as an error.
1937+
* 2) RETURNING clauses, which may contain both Vars of the target relation
1938+
* and Vars of other relations. In this case we want to replace the
1939+
* other-relation Vars by OUTER_VAR references, while leaving target Vars
1940+
* alone. Thus inner_itlist = NULL and acceptable_rel = the ID of the
1941+
* target relation should be passed.
1942+
* 3) ON CONFLICT UPDATE SET/WHERE clauses. Here references to EXCLUDED are
1943+
* to be replaced with INNER_VAR references, while leaving target Vars (the
1944+
* to-be-updated relation) alone. Correspondingly inner_itlist is to be
1945+
* EXCLUDED elements, outer_itlist = NULL and acceptable_rel the target
1946+
* relation.
19421947
*
19431948
* 'clauses' is the targetlist or list of join clauses
19441949
* 'outer_itlist' is the indexed target list of the outer join relation,
@@ -1981,7 +1986,7 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
19811986
{
19821987
Var *var = (Var *) node;
19831988

1984-
/* First look for the var in the input tlists */
1989+
/* Look for the var in the input tlists, first in the outer */
19851990
if (context->outer_itlist)
19861991
{
19871992
newvar = search_indexed_tlist_for_var(var,
@@ -1992,7 +1997,7 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
19921997
return (Node *) newvar;
19931998
}
19941999

1995-
/* Then in the outer */
2000+
/* then in the inner. */
19962001
if (context->inner_itlist)
19972002
{
19982003
newvar = search_indexed_tlist_for_var(var,

‎src/backend/parser/analyze.c

+62-14
Original file line numberDiff line numberDiff line change
@@ -891,33 +891,81 @@ transformOnConflictClause(ParseState *pstate,
891891
/* Process DO UPDATE */
892892
if (onConflictClause->action == ONCONFLICT_UPDATE)
893893
{
894+
Relation targetrel = pstate->p_target_relation;
895+
Var *var;
896+
TargetEntry *te;
897+
int attno;
898+
894899
/*
895900
* All INSERT expressions have been parsed, get ready for potentially
896901
* existing SET statements that need to be processed like an UPDATE.
897902
*/
898903
pstate->p_is_insert = false;
899904

905+
/*
906+
* Add range table entry for the EXCLUDED pseudo relation; relkind is
907+
* set to composite to signal that we're not dealing with an actual
908+
* relation.
909+
*/
900910
exclRte = addRangeTableEntryForRelation(pstate,
901-
pstate->p_target_relation,
911+
targetrel,
902912
makeAlias("excluded", NIL),
903913
false, false);
914+
exclRte->relkind = RELKIND_COMPOSITE_TYPE;
904915
exclRelIndex = list_length(pstate->p_rtable);
905916

906917
/*
907-
* Build a targetlist for the EXCLUDED pseudo relation. Out of
908-
* simplicity we do that here, because expandRelAttrs() happens to
909-
* nearly do the right thing; specifically it also works with views.
910-
* It'd be more proper to instead scan some pseudo scan node, but it
911-
* doesn't seem worth the amount of code required.
912-
*
913-
* The only caveat of this hack is that the permissions expandRelAttrs
914-
* adds have to be reset. markVarForSelectPriv() will add the exact
915-
* required permissions back.
918+
* Build a targetlist for the EXCLUDED pseudo relation. Have to be
919+
* careful to use resnos that correspond to attnos of the underlying
920+
* relation.
921+
*/
922+
Assert(pstate->p_next_resno == 1);
923+
for (attno = 0; attno < targetrel->rd_rel->relnatts; attno++)
924+
{
925+
Form_pg_attribute attr = targetrel->rd_att->attrs[attno];
926+
char *name;
927+
928+
if (attr->attisdropped)
929+
{
930+
/*
931+
* can't use atttypid here, but it doesn't really matter what
932+
* type the Const claims to be.
933+
*/
934+
var = (Var *) makeNullConst(INT4OID, -1, InvalidOid);
935+
name = "";
936+
}
937+
else
938+
{
939+
var = makeVar(exclRelIndex, attno + 1,
940+
attr->atttypid, attr->atttypmod,
941+
attr->attcollation,
942+
0);
943+
var->location = -1;
944+
945+
name = NameStr(attr->attname);
946+
}
947+
948+
Assert(pstate->p_next_resno == attno + 1);
949+
te = makeTargetEntry((Expr *) var,
950+
pstate->p_next_resno++,
951+
name,
952+
false);
953+
954+
/* don't require select access yet */
955+
exclRelTlist = lappend(exclRelTlist, te);
956+
}
957+
958+
/*
959+
* Additionally add a whole row tlist entry for EXCLUDED. That's
960+
* really only needed for ruleutils' benefit, which expects to find
961+
* corresponding entries in child tlists. Alternatively we could do
962+
* this only when required, but that doesn't seem worth the trouble.
916963
*/
917-
exclRelTlist = expandRelAttrs(pstate, exclRte,
918-
exclRelIndex, 0, -1);
919-
exclRte->requiredPerms = 0;
920-
exclRte->selectedCols = NULL;
964+
var = makeVar(exclRelIndex, InvalidAttrNumber,
965+
RelationGetRelid(targetrel),
966+
-1, InvalidOid, 0);
967+
te = makeTargetEntry((Expr *) var, 0, NULL, true);
968+
exclRelTlist = lappend(exclRelTlist, te);
921969

922970
/*
923971
* Add EXCLUDED and the target RTE to the namespace, so that they can

‎src/backend/parser/parse_relation.c

+5-2
Original file line numberDiff line numberDiff line change
@@ -686,9 +686,12 @@ scanRTEForColumn(ParseState *pstate, RangeTblEntry *rte, char *colname,
686686
return result;
687687

688688
/*
689-
* If the RTE represents a real table, consider system column names.
689+
* If the RTE represents a real relation, consider system column names.
690+
* Composites are only used for pseudo-relations like ON CONFLICT's
691+
* excluded.
690692
*/
691-
if (rte->rtekind == RTE_RELATION)
693+
if (rte->rtekind == RTE_RELATION &&
694+
rte->relkind != RELKIND_COMPOSITE_TYPE)
692695
{
693696
/* quick check to see if name could be a system column */
694697
attnum = specialAttNum(colname);

‎src/test/regress/expected/insert_conflict.out

+121
Original file line numberDiff line numberDiff line change
@@ -380,8 +380,80 @@ ERROR: there is no unique or exclusion constraint matching the ON CONFLICT spec
380380
insert into insertconflicttest values (23, 'Blackberry') on conflict (fruit) where fruit like '%berry' do update set fruit = excluded.fruit;
381381
ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification
382382
drop index partial_key_index;
383+
--
384+
-- Test that wholerow references to ON CONFLICT's EXCLUDED work
385+
--
386+
create unique index plain on insertconflicttest(key);
387+
-- Succeeds, updates existing row:
388+
insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit
389+
where i.* != excluded.* returning *;
390+
key | fruit
391+
-----+-----------
392+
23 | Jackfruit
393+
(1 row)
394+
395+
-- No update this time, though:
396+
insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit
397+
where i.* != excluded.* returning *;
398+
key | fruit
399+
-----+-------
400+
(0 rows)
401+
402+
-- Predicate changed to require match rather than non-match, so updates once more:
403+
insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit
404+
where i.* = excluded.* returning *;
405+
key | fruit
406+
-----+-----------
407+
23 | Jackfruit
408+
(1 row)
409+
410+
-- Assign:
411+
insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.*::text
412+
returning *;
413+
key | fruit
414+
-----+--------------
415+
23 | (23,Avocado)
416+
(1 row)
417+
418+
-- deparse whole row var in WHERE and SET clauses:
419+
explain (costs off) insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.fruit where excluded.* is null;
420+
QUERY PLAN
421+
-----------------------------------------
422+
Insert on insertconflicttest i
423+
Conflict Resolution: UPDATE
424+
Conflict Arbiter Indexes: plain
425+
Conflict Filter: (excluded.* IS NULL)
426+
-> Result
427+
(5 rows)
428+
429+
explain (costs off) insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.*::text;
430+
QUERY PLAN
431+
-----------------------------------
432+
Insert on insertconflicttest i
433+
Conflict Resolution: UPDATE
434+
Conflict Arbiter Indexes: plain
435+
-> Result
436+
(4 rows)
437+
438+
drop index plain;
383439
-- Cleanup
384440
drop table insertconflicttest;
441+
--
442+
-- Verify that EXCLUDED does not allow system column references. These
443+
-- do not make sense because EXCLUDED isn't an already stored tuple
444+
-- (and thus doesn't have a ctid, oids are not assigned yet, etc).
445+
--
446+
create table syscolconflicttest(key int4, data text) WITH OIDS;
447+
insert into syscolconflicttest values (1);
448+
insert into syscolconflicttest values (1) on conflict (key) do update set data = excluded.ctid::text;
449+
ERROR: column excluded.ctid does not exist
450+
LINE 1: ...values (1) on conflict (key) do update set data = excluded.c...
451+
^
452+
insert into syscolconflicttest values (1) on conflict (key) do update set data = excluded.oid::text;
453+
ERROR: column excluded.oid does not exist
454+
LINE 1: ...values (1) on conflict (key) do update set data = excluded.o...
455+
^
456+
drop table syscolconflicttest;
385457
-- ******************************************************************
386458
-- * *
387459
-- * Test inheritance (example taken from tutorial) *
@@ -566,3 +638,52 @@ insert into testoids values(3, '2') on conflict (key) do update set data = exclu
566638
(1 row)
567639

568640
DROP TABLE testoids;
641+
-- check that references to columns after dropped columns are handled correctly
642+
create table dropcol(key int primary key, drop1 int, keep1 text, drop2 numeric, keep2 float);
643+
insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 1, '1', '1', 1);
644+
-- set using excluded
645+
insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 2, '2', '2', 2) on conflict(key)
646+
do update set drop1 = excluded.drop1, keep1 = excluded.keep1, drop2 = excluded.drop2, keep2 = excluded.keep2
647+
where excluded.drop1 is not null and excluded.keep1 is not null and excluded.drop2 is not null and excluded.keep2 is not null
648+
and dropcol.drop1 is not null and dropcol.keep1 is not null and dropcol.drop2 is not null and dropcol.keep2 is not null
649+
returning *;
650+
key | drop1 | keep1 | drop2 | keep2
651+
-----+-------+-------+-------+-------
652+
1 | 2 | 2 | 2 | 2
653+
(1 row)
654+
655+
;
656+
-- set using existing table
657+
insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 3, '3', '3', 3) on conflict(key)
658+
do update set drop1 = dropcol.drop1, keep1 = dropcol.keep1, drop2 = dropcol.drop2, keep2 = dropcol.keep2
659+
returning *;
660+
key | drop1 | keep1 | drop2 | keep2
661+
-----+-------+-------+-------+-------
662+
1 | 2 | 2 | 2 | 2
663+
(1 row)
664+
665+
;
666+
alter table dropcol drop column drop1, drop column drop2;
667+
-- set using excluded
668+
insert into dropcol(key, keep1, keep2) values(1, '4', 4) on conflict(key)
669+
do update set keep1 = excluded.keep1, keep2 = excluded.keep2
670+
where excluded.keep1 is not null and excluded.keep2 is not null
671+
and dropcol.keep1 is not null and dropcol.keep2 is not null
672+
returning *;
673+
key | keep1 | keep2
674+
-----+-------+-------
675+
1 | 4 | 4
676+
(1 row)
677+
678+
;
679+
-- set using existing table
680+
insert into dropcol(key, keep1, keep2) values(1, '5', 5) on conflict(key)
681+
do update set keep1 = dropcol.keep1, keep2 = dropcol.keep2
682+
returning *;
683+
key | keep1 | keep2
684+
-----+-------+-------
685+
1 | 4 | 4
686+
(1 row)
687+
688+
;
689+
DROP TABLE dropcol;

‎src/test/regress/expected/rules.out

+9-9
Original file line numberDiff line numberDiff line change
@@ -2899,15 +2899,15 @@ CREATE RULE hat_upsert AS ON INSERT TO hats
28992899
ON CONFLICT (hat_name)
29002900
DO UPDATE
29012901
SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color
2902-
WHERE excluded.hat_color <> 'forbidden'
2902+
WHERE excluded.hat_color <> 'forbidden' AND hat_data.* != excluded.*
29032903
RETURNING *;
29042904
SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename;
29052905
definition
29062906
-----------------------------------------------------------------------------------------------------------------------------------------
29072907
CREATE RULE hat_upsert AS +
29082908
ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) +
29092909
VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name) DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color+
2910-
WHERE (excluded.hat_color <> 'forbidden'::bpchar) +
2910+
WHERE ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) +
29112911
RETURNING hat_data.hat_name, +
29122912
hat_data.hat_color;
29132913
(1 row)
@@ -2955,19 +2955,19 @@ SELECT tablename, rulename, definition FROM pg_rules
29552955
hats | hat_upsert | CREATE RULE hat_upsert AS +
29562956
| | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) +
29572957
| | VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name) DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color+
2958-
| | WHERE (excluded.hat_color <> 'forbidden'::bpchar) +
2958+
| | WHERE ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) +
29592959
| | RETURNING hat_data.hat_name, +
29602960
| | hat_data.hat_color;
29612961
(1 row)
29622962

29632963
-- ensure explain works for on insert conflict rules
29642964
explain (costs off) INSERT INTO hats VALUES ('h8', 'forbidden') RETURNING *;
2965-
QUERY PLAN
2966-
----------------------------------------------------------------
2965+
QUERY PLAN
2966+
-------------------------------------------------------------------------------------------------
29672967
Insert on hat_data
29682968
Conflict Resolution: UPDATE
29692969
Conflict Arbiter Indexes: hat_data_unique_idx
2970-
Conflict Filter: (excluded.hat_color <> 'forbidden'::bpchar)
2970+
Conflict Filter: ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*))
29712971
-> Result
29722972
(5 rows)
29732973

@@ -2994,12 +2994,12 @@ EXPLAIN (costs off) WITH data(hat_name, hat_color) AS (
29942994
INSERT INTO hats
29952995
SELECT * FROM data
29962996
RETURNING *;
2997-
QUERY PLAN
2998-
----------------------------------------------------------------
2997+
QUERY PLAN
2998+
-------------------------------------------------------------------------------------------------
29992999
Insert on hat_data
30003000
Conflict Resolution: UPDATE
30013001
Conflict Arbiter Indexes: hat_data_unique_idx
3002-
Conflict Filter: (excluded.hat_color <> 'forbidden'::bpchar)
3002+
Conflict Filter: ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*))
30033003
CTE data
30043004
-> Values Scan on "*VALUES*"
30053005
-> CTE Scan on data

0 commit comments

Comments
 (0)