Skip to content

Commit e55a946

Browse files
tglsfdctglsfdc
committed
Fix two bugs in merging of inherited CHECK constraints.
Historically, we've allowed users to add a CHECK constraint to a child table and then add an identical CHECK constraint to the parent. This results in "merging" the two constraints so that the pre-existing child constraint ends up with both conislocal = true and coninhcount > 0. However, if you tried to do it in the other order, you got a duplicate constraint error. This is problematic for pg_dump, which needs to issue separated ADD CONSTRAINT commands in some cases, but has no good way to ensure that the constraints will be added in the required order. And it's more than a bit arbitrary, too. The goal of complaining about duplicated ADD CONSTRAINT commands can be served if we reject the case of adding a constraint when the existing one already has conislocal = true; but if it has conislocal = false, let's just make the ADD CONSTRAINT set conislocal = true. In this way, either order of adding the constraints has the same end result. Another problem was that the code allowed creation of a parent constraint marked convalidated that is merged with a child constraint that is !convalidated. In this case, an inheritance scan of the parent table could emit some rows violating the constraint condition, which would be an unexpected result given the marking of the parent constraint as validated. Hence, forbid merging of constraints in this case. (Note: valid child and not-valid parent seems fine, so continue to allow that.) Per report from Benedikt Grundmann. Back-patch to 9.2 where we introduced possibly-not-valid check constraints. The second bug obviously doesn't apply before that, and I think the first doesn't either, because pg_dump only gets into this situation when dealing with not-valid constraints. Report: <CADbMkNPT-Jz5PRSQ4RbUASYAjocV_KHUWapR%2Bg8fNvhUAyRpxA%40mail.gmail.com> Discussion: <22108.1475874586@sss.pgh.pa.us>
1 parent 8811f5d commit e55a946

File tree

5 files changed

+135
-9
lines changed

5 files changed

+135
-9
lines changed

‎src/backend/catalog/heap.c

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ static void StoreConstraints(Relation rel, List *cooked_constraints,
105105
bool is_internal);
106106
static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
107107
bool allow_merge, bool is_local,
108+
bool is_initially_valid,
108109
bool is_no_inherit);
109110
static void SetRelationNumChecks(Relation rel, int numchecks);
110111
static Node *cookConstraint(ParseState *pstate,
@@ -2301,6 +2302,7 @@ AddRelationNewConstraints(Relation rel,
23012302
*/
23022303
if (MergeWithExistingConstraint(rel, ccname, expr,
23032304
allow_merge, is_local,
2305+
cdef->initially_valid,
23042306
cdef->is_no_inherit))
23052307
continue;
23062308
}
@@ -2389,6 +2391,7 @@ AddRelationNewConstraints(Relation rel,
23892391
static bool
23902392
MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
23912393
bool allow_merge, bool is_local,
2394+
bool is_initially_valid,
23922395
bool is_no_inherit)
23932396
{
23942397
bool found;
@@ -2436,22 +2439,47 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
24362439
if (equal(expr, stringToNode(TextDatumGetCString(val))))
24372440
found = true;
24382441
}
2442+
2443+
/*
2444+
* If the existing constraint is purely inherited (no local
2445+
* definition) then interpret addition of a local constraint as a
2446+
* legal merge. This allows ALTER ADD CONSTRAINT on parent and
2447+
* child tables to be given in either order with same end state.
2448+
*/
2449+
if (is_local && !con->conislocal)
2450+
allow_merge = true;
2451+
24392452
if (!found || !allow_merge)
24402453
ereport(ERROR,
24412454
(errcode(ERRCODE_DUPLICATE_OBJECT),
24422455
errmsg("constraint \"%s\" for relation \"%s\" already exists",
24432456
ccname, RelationGetRelationName(rel))));
24442457

2445-
tup = heap_copytuple(tup);
2446-
con = (Form_pg_constraint) GETSTRUCT(tup);
2447-
2448-
/* If the constraint is "no inherit" then cannot merge */
2458+
/* If the child constraint is "no inherit" then cannot merge */
24492459
if (con->connoinherit)
24502460
ereport(ERROR,
24512461
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
24522462
errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"",
24532463
ccname, RelationGetRelationName(rel))));
24542464

2465+
/*
2466+
* If the child constraint is "not valid" then cannot merge with a
2467+
* valid parent constraint
2468+
*/
2469+
if (is_initially_valid && !con->convalidated)
2470+
ereport(ERROR,
2471+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
2472+
errmsg("constraint \"%s\" conflicts with NOT VALID constraint on relation \"%s\"",
2473+
ccname, RelationGetRelationName(rel))));
2474+
2475+
/* OK to update the tuple */
2476+
ereport(NOTICE,
2477+
(errmsg("merging constraint \"%s\" with inherited definition",
2478+
ccname)));
2479+
2480+
tup = heap_copytuple(tup);
2481+
con = (Form_pg_constraint) GETSTRUCT(tup);
2482+
24552483
if (is_local)
24562484
con->conislocal = true;
24572485
else
@@ -2461,10 +2489,6 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
24612489
Assert(is_local);
24622490
con->connoinherit = true;
24632491
}
2464-
/* OK to update the tuple */
2465-
ereport(NOTICE,
2466-
(errmsg("merging constraint \"%s\" with inherited definition",
2467-
ccname)));
24682492
simple_heap_update(conDesc, &tup->t_self, tup);
24692493
CatalogUpdateIndexes(conDesc, tup);
24702494
break;

‎src/backend/commands/tablecmds.c

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10373,14 +10373,25 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
1037310373
RelationGetRelationName(child_rel),
1037410374
NameStr(parent_con->conname))));
1037510375

10376-
/* If the constraint is "no inherit" then cannot merge */
10376+
/* If the child constraint is "no inherit" then cannot merge */
1037710377
if (child_con->connoinherit)
1037810378
ereport(ERROR,
1037910379
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
1038010380
errmsg("constraint \"%s\" conflicts with non-inherited constraint on child table \"%s\"",
1038110381
NameStr(child_con->conname),
1038210382
RelationGetRelationName(child_rel))));
1038310383

10384+
/*
10385+
* If the child constraint is "not valid" then cannot merge with a
10386+
* valid parent constraint
10387+
*/
10388+
if (parent_con->convalidated && !child_con->convalidated)
10389+
ereport(ERROR,
10390+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
10391+
errmsg("constraint \"%s\" conflicts with NOT VALID constraint on child table \"%s\"",
10392+
NameStr(child_con->conname),
10393+
RelationGetRelationName(child_rel))));
10394+
1038410395
/*
1038510396
* OK, bump the child constraint's inheritance count. (If we fail
1038610397
* later on, this change will just roll back.)

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1090,6 +1090,56 @@ Inherits: test_foreign_constraints
10901090
DROP TABLE test_foreign_constraints_inh;
10911091
DROP TABLE test_foreign_constraints;
10921092
DROP TABLE test_primary_constraints;
1093+
-- Test that parent and child CHECK constraints can be created in either order
1094+
create table p1(f1 int);
1095+
create table p1_c1() inherits(p1);
1096+
alter table p1 add constraint inh_check_constraint1 check (f1 > 0);
1097+
alter table p1_c1 add constraint inh_check_constraint1 check (f1 > 0);
1098+
NOTICE: merging constraint "inh_check_constraint1" with inherited definition
1099+
alter table p1_c1 add constraint inh_check_constraint2 check (f1 < 10);
1100+
alter table p1 add constraint inh_check_constraint2 check (f1 < 10);
1101+
NOTICE: merging constraint "inh_check_constraint2" with inherited definition
1102+
select conrelid::regclass::text as relname, conname, conislocal, coninhcount
1103+
from pg_constraint where conname like 'inh\_check\_constraint%'
1104+
order by 1, 2;
1105+
relname | conname | conislocal | coninhcount
1106+
---------+-----------------------+------------+-------------
1107+
p1 | inh_check_constraint1 | t | 0
1108+
p1 | inh_check_constraint2 | t | 0
1109+
p1_c1 | inh_check_constraint1 | t | 1
1110+
p1_c1 | inh_check_constraint2 | t | 1
1111+
(4 rows)
1112+
1113+
drop table p1 cascade;
1114+
NOTICE: drop cascades to table p1_c1
1115+
-- Test that a valid child can have not-valid parent, but not vice versa
1116+
create table invalid_check_con(f1 int);
1117+
create table invalid_check_con_child() inherits(invalid_check_con);
1118+
alter table invalid_check_con_child add constraint inh_check_constraint check(f1 > 0) not valid;
1119+
alter table invalid_check_con add constraint inh_check_constraint check(f1 > 0); -- fail
1120+
ERROR: constraint "inh_check_constraint" conflicts with NOT VALID constraint on relation "invalid_check_con_child"
1121+
alter table invalid_check_con_child drop constraint inh_check_constraint;
1122+
insert into invalid_check_con values(0);
1123+
alter table invalid_check_con_child add constraint inh_check_constraint check(f1 > 0);
1124+
alter table invalid_check_con add constraint inh_check_constraint check(f1 > 0) not valid;
1125+
NOTICE: merging constraint "inh_check_constraint" with inherited definition
1126+
insert into invalid_check_con values(0); -- fail
1127+
ERROR: new row for relation "invalid_check_con" violates check constraint "inh_check_constraint"
1128+
DETAIL: Failing row contains (0).
1129+
insert into invalid_check_con_child values(0); -- fail
1130+
ERROR: new row for relation "invalid_check_con_child" violates check constraint "inh_check_constraint"
1131+
DETAIL: Failing row contains (0).
1132+
select conrelid::regclass::text as relname, conname,
1133+
convalidated, conislocal, coninhcount, connoinherit
1134+
from pg_constraint where conname like 'inh\_check\_constraint%'
1135+
order by 1, 2;
1136+
relname | conname | convalidated | conislocal | coninhcount | connoinherit
1137+
-------------------------+----------------------+--------------+------------+-------------+--------------
1138+
invalid_check_con | inh_check_constraint | f | t | 0 | f
1139+
invalid_check_con_child | inh_check_constraint | t | t | 1 | f
1140+
(2 rows)
1141+
1142+
-- We don't drop the invalid_check_con* tables, to test dump/reload with
10931143
--
10941144
-- Test parameterized append plans for inheritance trees
10951145
--

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ int2_tbl|f
6262
int4_tbl|f
6363
int8_tbl|f
6464
interval_tbl|f
65+
invalid_check_con|f
66+
invalid_check_con_child|f
6567
iportaltest|f
6668
kd_point_tbl|t
6769
line_tbl|f

‎src/test/regress/sql/inherit.sql

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,45 @@ DROP TABLE test_foreign_constraints_inh;
334334
DROP TABLE test_foreign_constraints;
335335
DROP TABLE test_primary_constraints;
336336

337+
-- Test that parent and child CHECK constraints can be created in either order
338+
create table p1(f1 int);
339+
create table p1_c1() inherits(p1);
340+
341+
alter table p1 add constraint inh_check_constraint1 check (f1 > 0);
342+
alter table p1_c1 add constraint inh_check_constraint1 check (f1 > 0);
343+
344+
alter table p1_c1 add constraint inh_check_constraint2 check (f1 < 10);
345+
alter table p1 add constraint inh_check_constraint2 check (f1 < 10);
346+
347+
select conrelid::regclass::text as relname, conname, conislocal, coninhcount
348+
from pg_constraint where conname like 'inh\_check\_constraint%'
349+
order by 1, 2;
350+
351+
drop table p1 cascade;
352+
353+
-- Test that a valid child can have not-valid parent, but not vice versa
354+
create table invalid_check_con(f1 int);
355+
create table invalid_check_con_child() inherits(invalid_check_con);
356+
357+
alter table invalid_check_con_child add constraint inh_check_constraint check(f1 > 0) not valid;
358+
alter table invalid_check_con add constraint inh_check_constraint check(f1 > 0); -- fail
359+
alter table invalid_check_con_child drop constraint inh_check_constraint;
360+
361+
insert into invalid_check_con values(0);
362+
363+
alter table invalid_check_con_child add constraint inh_check_constraint check(f1 > 0);
364+
alter table invalid_check_con add constraint inh_check_constraint check(f1 > 0) not valid;
365+
366+
insert into invalid_check_con values(0); -- fail
367+
insert into invalid_check_con_child values(0); -- fail
368+
369+
select conrelid::regclass::text as relname, conname,
370+
convalidated, conislocal, coninhcount, connoinherit
371+
from pg_constraint where conname like 'inh\_check\_constraint%'
372+
order by 1, 2;
373+
374+
-- We don't drop the invalid_check_con* tables, to test dump/reload with
375+
337376
--
338377
-- Test parameterized append plans for inheritance trees
339378
--

0 commit comments

Comments
 (0)