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

Commit 52898c6

Browse files
tglsfdctglsfdc
committed
Make INSERT-from-multiple-VALUES-rows handle domain target columns.
Commit a3c7a99 fixed some cases involving target columns that are arrays or composites by applying transformAssignedExpr to the VALUES entries, and then stripping off any assignment ArrayRefs or FieldStores that the transformation added. But I forgot about domains over arrays or composites :-(. Such cases would either fail with surprising complaints about mismatched datatypes, or insert unexpected coercions that could lead to odd results. To fix, extend the stripping logic to get rid of CoerceToDomain if it's atop an ArrayRef or FieldStore. While poking at this, I realized that there's a poorly documented and not-at-all-tested behavior nearby: we coerce each VALUES column to the domain type separately, and rely on the rewriter to merge those operations so that the domain constraints are checked only once. If that merging did not happen, it's entirely possible that we'd get unexpected domain constraint failures due to checking a partially-updated container value. There's no bug there, but while we're here let's improve the commentary about it and add some test cases that explicitly exercise that behavior. Per bug #18393 from Pablo Kharo. Back-patch to all supported branches. Discussion: https://postgr.es/m/18393-65fedb1a0de9260d@postgresql.org
1 parent d4c573d commit 52898c6

File tree

5 files changed

+227
-11
lines changed

5 files changed

+227
-11
lines changed

‎src/backend/parser/analyze.c

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,17 +1072,28 @@ transformInsertRow(ParseState *pstate, List *exprlist,
10721072

10731073
if (strip_indirection)
10741074
{
1075+
/*
1076+
* We need to remove top-level FieldStores and SubscriptingRefs,
1077+
* as well as any CoerceToDomain appearing above one of those ---
1078+
* but not a CoerceToDomain that isn't above one of those.
1079+
*/
10751080
while (expr)
10761081
{
1077-
if (IsA(expr, FieldStore))
1082+
Expr *subexpr = expr;
1083+
1084+
while (IsA(subexpr, CoerceToDomain))
1085+
{
1086+
subexpr = ((CoerceToDomain *) subexpr)->arg;
1087+
}
1088+
if (IsA(subexpr, FieldStore))
10781089
{
1079-
FieldStore *fstore = (FieldStore *) expr;
1090+
FieldStore *fstore = (FieldStore *) subexpr;
10801091

10811092
expr = (Expr *) linitial(fstore->newvals);
10821093
}
1083-
else if (IsA(expr, SubscriptingRef))
1094+
else if (IsA(subexpr, SubscriptingRef))
10841095
{
1085-
SubscriptingRef *sbsref = (SubscriptingRef *) expr;
1096+
SubscriptingRef *sbsref = (SubscriptingRef *) subexpr;
10861097

10871098
if (sbsref->refassgnexpr == NULL)
10881099
break;

‎src/backend/parser/parse_target.c

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -821,7 +821,16 @@ transformAssignmentIndirection(ParseState *pstate,
821821
fstore->fieldnums = list_make1_int(attnum);
822822
fstore->resulttype = baseTypeId;
823823

824-
/* If target is a domain, apply constraints */
824+
/*
825+
* If target is a domain, apply constraints. Notice that this
826+
* isn't totally right: the expression tree we build would check
827+
* the domain's constraints on a composite value with only this
828+
* one field populated or updated, possibly leading to an unwanted
829+
* failure. The rewriter will merge together any subfield
830+
* assignments to the same table column, resulting in the domain's
831+
* constraints being checked only once after we've assigned to all
832+
* the fields that the INSERT or UPDATE means to.
833+
*/
825834
if (baseTypeId != targetTypeId)
826835
return coerce_to_domain((Node *) fstore,
827836
baseTypeId, baseTypeMod,
@@ -967,7 +976,12 @@ transformAssignmentSubscripts(ParseState *pstate,
967976

968977
result = (Node *) sbsref;
969978

970-
/* If target was a domain over container, need to coerce up to the domain */
979+
/*
980+
* If target was a domain over container, need to coerce up to the domain.
981+
* As in transformAssignmentIndirection, this coercion is premature if the
982+
* query assigns to multiple elements of the container; but we'll fix that
983+
* during query rewrite.
984+
*/
971985
if (containerType != targetTypeId)
972986
{
973987
Oid resulttype = exprType(result);

‎src/backend/rewrite/rewriteHandler.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1087,9 +1087,9 @@ process_matched_tle(TargetEntry *src_tle,
10871087
* resulting in each assignment containing a CoerceToDomain node over a
10881088
* FieldStore or SubscriptingRef. These should have matching target
10891089
* domains, so we strip them and reconstitute a single CoerceToDomain over
1090-
* the combined FieldStore/SubscriptingRef nodes. (Notice that this has the
1091-
* result that the domain's checks are applied only after we do all the
1092-
* field or element updates, not after each one. This is arguably desirable.)
1090+
* the combined FieldStore/SubscriptingRef nodes. (Notice that this has
1091+
* the result that the domain's checks are applied only after we do all
1092+
* the field or element updates, not after each one. This is desirable.)
10931093
*----------
10941094
*/
10951095
src_expr = (Node *) src_tle->expr;

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

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,121 @@ Rules:
180180

181181
drop table inserttest2;
182182
drop table inserttest;
183-
drop type insert_test_type;
183+
-- Make the same tests with domains over the array and composite fields
184+
create domain insert_pos_ints as int[] check (value[1] > 0);
185+
create domain insert_test_domain as insert_test_type
186+
check ((value).if2[1] is not null);
187+
create table inserttesta (f1 int, f2 insert_pos_ints);
188+
create table inserttestb (f3 insert_test_domain, f4 insert_test_domain[]);
189+
insert into inserttesta (f2[1], f2[2]) values (1,2);
190+
insert into inserttesta (f2[1], f2[2]) values (3,4), (5,6);
191+
insert into inserttesta (f2[1], f2[2]) select 7,8;
192+
insert into inserttesta (f2[1], f2[2]) values (1,default); -- not supported
193+
ERROR: cannot set an array element to DEFAULT
194+
LINE 1: insert into inserttesta (f2[1], f2[2]) values (1,default);
195+
^
196+
insert into inserttesta (f2[1], f2[2]) values (0,2);
197+
ERROR: value for domain insert_pos_ints violates check constraint "insert_pos_ints_check"
198+
insert into inserttesta (f2[1], f2[2]) values (3,4), (0,6);
199+
ERROR: value for domain insert_pos_ints violates check constraint "insert_pos_ints_check"
200+
insert into inserttesta (f2[1], f2[2]) select 0,8;
201+
ERROR: value for domain insert_pos_ints violates check constraint "insert_pos_ints_check"
202+
insert into inserttestb (f3.if1, f3.if2) values (1,array['foo']);
203+
insert into inserttestb (f3.if1, f3.if2) values (1,'{foo}'), (2,'{bar}');
204+
insert into inserttestb (f3.if1, f3.if2) select 3, '{baz,quux}';
205+
insert into inserttestb (f3.if1, f3.if2) values (1,default); -- not supported
206+
ERROR: cannot set a subfield to DEFAULT
207+
LINE 1: insert into inserttestb (f3.if1, f3.if2) values (1,default);
208+
^
209+
insert into inserttestb (f3.if1, f3.if2) values (1,array[null]);
210+
ERROR: value for domain insert_test_domain violates check constraint "insert_test_domain_check"
211+
insert into inserttestb (f3.if1, f3.if2) values (1,'{null}'), (2,'{bar}');
212+
ERROR: value for domain insert_test_domain violates check constraint "insert_test_domain_check"
213+
insert into inserttestb (f3.if1, f3.if2) select 3, '{null,quux}';
214+
ERROR: value for domain insert_test_domain violates check constraint "insert_test_domain_check"
215+
insert into inserttestb (f3.if2[1], f3.if2[2]) values ('foo', 'bar');
216+
insert into inserttestb (f3.if2[1], f3.if2[2]) values ('foo', 'bar'), ('baz', 'quux');
217+
insert into inserttestb (f3.if2[1], f3.if2[2]) select 'bear', 'beer';
218+
insert into inserttestb (f3, f4[1].if2[1], f4[1].if2[2]) values (row(1,'{x}'), 'foo', 'bar');
219+
insert into inserttestb (f3, f4[1].if2[1], f4[1].if2[2]) values (row(1,'{x}'), 'foo', 'bar'), (row(2,'{y}'), 'baz', 'quux');
220+
insert into inserttestb (f3, f4[1].if2[1], f4[1].if2[2]) select row(1,'{x}')::insert_test_domain, 'bear', 'beer';
221+
select * from inserttesta;
222+
f1 | f2
223+
----+-------
224+
| {1,2}
225+
| {3,4}
226+
| {5,6}
227+
| {7,8}
228+
(4 rows)
229+
230+
select * from inserttestb;
231+
f3 | f4
232+
------------------+------------------------
233+
(1,{foo}) |
234+
(1,{foo}) |
235+
(2,{bar}) |
236+
(3,"{baz,quux}") |
237+
(,"{foo,bar}") |
238+
(,"{foo,bar}") |
239+
(,"{baz,quux}") |
240+
(,"{bear,beer}") |
241+
(1,{x}) | {"(,\"{foo,bar}\")"}
242+
(1,{x}) | {"(,\"{foo,bar}\")"}
243+
(2,{y}) | {"(,\"{baz,quux}\")"}
244+
(1,{x}) | {"(,\"{bear,beer}\")"}
245+
(12 rows)
246+
247+
-- also check reverse-listing
248+
create table inserttest2 (f1 bigint, f2 text);
249+
create rule irule1 as on insert to inserttest2 do also
250+
insert into inserttestb (f3.if2[1], f3.if2[2])
251+
values (new.f1,new.f2);
252+
create rule irule2 as on insert to inserttest2 do also
253+
insert into inserttestb (f4[1].if1, f4[1].if2[2])
254+
values (1,'fool'),(new.f1,new.f2);
255+
create rule irule3 as on insert to inserttest2 do also
256+
insert into inserttestb (f4[1].if1, f4[1].if2[2])
257+
select new.f1, new.f2;
258+
\d+ inserttest2
259+
Table "public.inserttest2"
260+
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
261+
--------+--------+-----------+----------+---------+----------+--------------+-------------
262+
f1 | bigint | | | | plain | |
263+
f2 | text | | | | extended | |
264+
Rules:
265+
irule1 AS
266+
ON INSERT TO inserttest2 DO INSERT INTO inserttestb (f3.if2[1], f3.if2[2])
267+
VALUES (new.f1, new.f2)
268+
irule2 AS
269+
ON INSERT TO inserttest2 DO INSERT INTO inserttestb (f4[1].if1, f4[1].if2[2]) VALUES (1,'fool'::text), (new.f1,new.f2)
270+
irule3 AS
271+
ON INSERT TO inserttest2 DO INSERT INTO inserttestb (f4[1].if1, f4[1].if2[2]) SELECT new.f1,
272+
new.f2
273+
274+
drop table inserttest2;
275+
drop table inserttesta;
276+
drop table inserttestb;
277+
drop domain insert_pos_ints;
278+
drop domain insert_test_domain;
279+
-- Verify that multiple inserts to subfields of a domain-over-container
280+
-- check the domain constraints only on the finished value
281+
create domain insert_nnarray as int[]
282+
check (value[1] is not null and value[2] is not null);
283+
create domain insert_test_domain as insert_test_type
284+
check ((value).if1 is not null and (value).if2 is not null);
285+
create table inserttesta (f1 insert_nnarray);
286+
insert into inserttesta (f1[1]) values (1); -- fail
287+
ERROR: value for domain insert_nnarray violates check constraint "insert_nnarray_check"
288+
insert into inserttesta (f1[1], f1[2]) values (1, 2);
289+
create table inserttestb (f1 insert_test_domain);
290+
insert into inserttestb (f1.if1) values (1); -- fail
291+
ERROR: value for domain insert_test_domain violates check constraint "insert_test_domain_check"
292+
insert into inserttestb (f1.if1, f1.if2) values (1, '{foo}');
293+
drop table inserttesta;
294+
drop table inserttestb;
295+
drop domain insert_nnarray;
296+
drop type insert_test_type cascade;
297+
NOTICE: drop cascades to type insert_test_domain
184298
-- direct partition inserts should check partition bound constraint
185299
create table range_parted (
186300
a text,

‎src/test/regress/sql/insert.sql

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,84 @@ create rule irule3 as on insert to inserttest2 do also
105105

106106
drop table inserttest2;
107107
drop table inserttest;
108-
drop type insert_test_type;
108+
109+
-- Make the same tests with domains over the array and composite fields
110+
111+
create domain insert_pos_ints as int[] check (value[1] > 0);
112+
113+
create domain insert_test_domain as insert_test_type
114+
check ((value).if2[1] is not null);
115+
116+
create table inserttesta (f1 int, f2 insert_pos_ints);
117+
create table inserttestb (f3 insert_test_domain, f4 insert_test_domain[]);
118+
119+
insert into inserttesta (f2[1], f2[2]) values (1,2);
120+
insert into inserttesta (f2[1], f2[2]) values (3,4), (5,6);
121+
insert into inserttesta (f2[1], f2[2]) select 7,8;
122+
insert into inserttesta (f2[1], f2[2]) values (1,default); -- not supported
123+
insert into inserttesta (f2[1], f2[2]) values (0,2);
124+
insert into inserttesta (f2[1], f2[2]) values (3,4), (0,6);
125+
insert into inserttesta (f2[1], f2[2]) select 0,8;
126+
127+
insert into inserttestb (f3.if1, f3.if2) values (1,array['foo']);
128+
insert into inserttestb (f3.if1, f3.if2) values (1,'{foo}'), (2,'{bar}');
129+
insert into inserttestb (f3.if1, f3.if2) select 3, '{baz,quux}';
130+
insert into inserttestb (f3.if1, f3.if2) values (1,default); -- not supported
131+
insert into inserttestb (f3.if1, f3.if2) values (1,array[null]);
132+
insert into inserttestb (f3.if1, f3.if2) values (1,'{null}'), (2,'{bar}');
133+
insert into inserttestb (f3.if1, f3.if2) select 3, '{null,quux}';
134+
135+
insert into inserttestb (f3.if2[1], f3.if2[2]) values ('foo', 'bar');
136+
insert into inserttestb (f3.if2[1], f3.if2[2]) values ('foo', 'bar'), ('baz', 'quux');
137+
insert into inserttestb (f3.if2[1], f3.if2[2]) select 'bear', 'beer';
138+
139+
insert into inserttestb (f3, f4[1].if2[1], f4[1].if2[2]) values (row(1,'{x}'), 'foo', 'bar');
140+
insert into inserttestb (f3, f4[1].if2[1], f4[1].if2[2]) values (row(1,'{x}'), 'foo', 'bar'), (row(2,'{y}'), 'baz', 'quux');
141+
insert into inserttestb (f3, f4[1].if2[1], f4[1].if2[2]) select row(1,'{x}')::insert_test_domain, 'bear', 'beer';
142+
143+
select * from inserttesta;
144+
select * from inserttestb;
145+
146+
-- also check reverse-listing
147+
create table inserttest2 (f1 bigint, f2 text);
148+
create rule irule1 as on insert to inserttest2 do also
149+
insert into inserttestb (f3.if2[1], f3.if2[2])
150+
values (new.f1,new.f2);
151+
create rule irule2 as on insert to inserttest2 do also
152+
insert into inserttestb (f4[1].if1, f4[1].if2[2])
153+
values (1,'fool'),(new.f1,new.f2);
154+
create rule irule3 as on insert to inserttest2 do also
155+
insert into inserttestb (f4[1].if1, f4[1].if2[2])
156+
select new.f1, new.f2;
157+
\d+ inserttest2
158+
159+
drop table inserttest2;
160+
drop table inserttesta;
161+
drop table inserttestb;
162+
drop domain insert_pos_ints;
163+
drop domain insert_test_domain;
164+
165+
-- Verify that multiple inserts to subfields of a domain-over-container
166+
-- check the domain constraints only on the finished value
167+
168+
create domain insert_nnarray as int[]
169+
check (value[1] is not null and value[2] is not null);
170+
171+
create domain insert_test_domain as insert_test_type
172+
check ((value).if1 is not null and (value).if2 is not null);
173+
174+
create table inserttesta (f1 insert_nnarray);
175+
insert into inserttesta (f1[1]) values (1); -- fail
176+
insert into inserttesta (f1[1], f1[2]) values (1, 2);
177+
178+
create table inserttestb (f1 insert_test_domain);
179+
insert into inserttestb (f1.if1) values (1); -- fail
180+
insert into inserttestb (f1.if1, f1.if2) values (1, '{foo}');
181+
182+
drop table inserttesta;
183+
drop table inserttestb;
184+
drop domain insert_nnarray;
185+
drop type insert_test_type cascade;
109186

110187
-- direct partition inserts should check partition bound constraint
111188
create table range_parted (

0 commit comments

Comments
 (0)