Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit ad22783

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 1023194 commit ad22783

File tree

7 files changed

+283
-38
lines changed

7 files changed

+283
-38
lines changed

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

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1935,16 +1935,21 @@ search_indexed_tlist_for_sortgroupref(Node *node,
19351935
* relation target lists. Also perform opcode lookup and add
19361936
* regclass OIDs to root->glob->relationOids.
19371937
*
1938-
* This is used in two different scenarios: a normal join clause, where all
1939-
* the Vars in the clause *must* be replaced by OUTER_VAR or INNER_VAR
1940-
* references; and a RETURNING clause, which may contain both Vars of the
1941-
* target relation and Vars of other relations. In the latter case we want
1942-
* to replace the other-relation Vars by OUTER_VAR references, while leaving
1943-
* target Vars alone.
1944-
*
1945-
* For a normal join, acceptable_rel should be zero so that any failure to
1946-
* match a Var will be reported as an error. For the RETURNING case, pass
1947-
* inner_itlist = NULL and acceptable_rel = the ID of the target relation.
1938+
* This is used in three different scenarios:
1939+
* 1) a normal join clause, where all the Vars in the clause *must* be
1940+
* replaced by OUTER_VAR or INNER_VAR references. In this case
1941+
* acceptable_rel should be zero so that any failure to match a Var will be
1942+
* reported as an error.
1943+
* 2) RETURNING clauses, which may contain both Vars of the target relation
1944+
* and Vars of other relations. In this case we want to replace the
1945+
* other-relation Vars by OUTER_VAR references, while leaving target Vars
1946+
* alone. Thus inner_itlist = NULL and acceptable_rel = the ID of the
1947+
* target relation should be passed.
1948+
* 3) ON CONFLICT UPDATE SET/WHERE clauses. Here references to EXCLUDED are
1949+
* to be replaced with INNER_VAR references, while leaving target Vars (the
1950+
* to-be-updated relation) alone. Correspondingly inner_itlist is to be
1951+
* EXCLUDED elements, outer_itlist = NULL and acceptable_rel the target
1952+
* relation.
19481953
*
19491954
* 'clauses' is the targetlist or list of join clauses
19501955
* 'outer_itlist' is the indexed target list of the outer join relation,
@@ -1987,7 +1992,7 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
19871992
{
19881993
Var *var = (Var *) node;
19891994

1990-
/* First look for the var in the input tlists */
1995+
/* Look for the var in the input tlists, first in the outer */
19911996
if (context->outer_itlist)
19921997
{
19931998
newvar = search_indexed_tlist_for_var(var,
@@ -1998,7 +2003,7 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
19982003
return (Node *) newvar;
19992004
}
20002005

2001-
/* Then in the outer */
2006+
/* then in the inner. */
20022007
if (context->inner_itlist)
20032008
{
20042009
newvar = search_indexed_tlist_for_var(var,

‎src/backend/parser/analyze.c

Lines changed: 62 additions & 14 deletions
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

Lines changed: 5 additions & 2 deletions
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

Lines changed: 121 additions & 0 deletions
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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2900,15 +2900,15 @@ CREATE RULE hat_upsert AS ON INSERT TO hats
29002900
ON CONFLICT (hat_name)
29012901
DO UPDATE
29022902
SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color
2903-
WHERE excluded.hat_color <> 'forbidden'
2903+
WHERE excluded.hat_color <> 'forbidden' AND hat_data.* != excluded.*
29042904
RETURNING *;
29052905
SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename;
29062906
definition
29072907
-----------------------------------------------------------------------------------------------------------------------------------------
29082908
CREATE RULE hat_upsert AS +
29092909
ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) +
29102910
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+
2911-
WHERE (excluded.hat_color <> 'forbidden'::bpchar) +
2911+
WHERE ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) +
29122912
RETURNING hat_data.hat_name, +
29132913
hat_data.hat_color;
29142914
(1 row)
@@ -2956,19 +2956,19 @@ SELECT tablename, rulename, definition FROM pg_rules
29562956
hats | hat_upsert | CREATE RULE hat_upsert AS +
29572957
| | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) +
29582958
| | 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+
2959-
| | WHERE (excluded.hat_color <> 'forbidden'::bpchar) +
2959+
| | WHERE ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) +
29602960
| | RETURNING hat_data.hat_name, +
29612961
| | hat_data.hat_color;
29622962
(1 row)
29632963

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

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

0 commit comments

Comments
 (0)