Hi Marko,
Marko Grujic <[email protected]> wrote:
> This patch introduces ChooseConstraintNameForRelation(Oid relid, ...),
> which wraps ChooseConstraintName, but also excludes all pre-existing
> constraints across the entire partition hierarchy.
I reviewed the patch from your second message on master at 9e17d25e79d.
It applies with git am, builds without warnings, and make check passes
(239 tests). With only the test changes applied, "constraints" fails on
master, so the new tests do exercise the fix.
Besides the four cases from the bug report, I tried the neighbouring
paths. All of these fail on master with the same 42710 error and work
with the patch:
- plain inheritance instead of partitioning (ADD CHECK, SET NOT NULL)
- three levels, with the conflicting constraint on a leaf in a third
schema
- two unnamed CHECKs in one ALTER TABLE, when the partition already
owns both t_a_check and t_a_check1 (the parent gets t_a_check2 and
t_a_check3)
- ADD PRIMARY KEY, through the not-null constraint it creates
- ADD COLUMN ... NOT NULL and ADD COLUMN ... CHECK
Vismay asked on pgsql-bugs whether foreign keys have the same problem.
They don't: with a CHECK named t_a_fkey on the partition, ALTER TABLE
... ADD FOREIGN KEY already works on master, because addFkConstraint()
picks a different name for that partition (t_a_fkey_1). So leaving the
FK caller of ChooseConstraintName alone looks right.
One case is still open. For ADD CONSTRAINT, ATPrepCmd locks all the
descendants before anything else happens, so by the time the name is
chosen the children cannot change. SET NOT NULL does not do that: the
name is chosen in ATExecSetNotNull, and the children are locked only
afterwards, when it recurses. So there find_all_inheritors(relid,
NoLock) reads children that another session can still be altering.
With the patch applied:
create schema parts;
create table t(a int) partition by range (a);
create table parts.t_1_10 partition of t for values from (1) to (10);
S1: begin;
S1: alter table parts.t_1_10
add constraint t_a_not_null check (a is not null);
S2: alter table t alter column a set not null; -- blocks
S1: commit;
S2: ERROR: constraint "t_a_not_null" for relation "t_1_10" already exists
The same sequence with ADD CHECK in S2 works and picks t_a_check1, and
so does ADD COLUMN ... NOT NULL. The result is only the old error, not
anything worse, so you may decide it is acceptable. As an experiment I
made ATPrepCmd lock the descendants for AT_SetNotNull the same way it
does for AT_AddConstraint (attached, 4 lines, on top of your patch).
With it, S2 above succeeds with t_a_not_null1, and make check still
passes. The cost is that SET NOT NULL takes the locks on the whole
tree up front rather than while recursing; it takes all of them either
way.
Two behaviour changes, in cases that work on master today:
1. If the partition in the other schema already has the same CHECK under
the default name, master merges it:
alter table parts.t_1_10 add constraint t_a_check check (a > 0);
alter table t add check (a > 0);
NOTICE: merging constraint "t_a_check" with inherited definition
With the patch the parent gets t_a_check1 and the partition ends up
with both t_a_check and t_a_check1. That is what master already does
when everything is in one schema, so the patch makes the two cases
consistent, but it may be worth a sentence in the commit message.
2. The exclusion list has every constraint name found on the
descendants, including those that would not conflict. If the
partition already has a not-null constraint called t_a_not_null,
SET NOT NULL on the parent works on master and names the parent's
constraint t_a_not_null; with the patch it becomes t_a_not_null1,
although not-null constraints are merged whatever their names are.
This is only cosmetic.
I also looked at the cost of scanning the descendants, since it now
happens for every auto-named constraint. With 5000 partitions in
another schema and 10000 constraints on them, ALTER TABLE ... ADD CHECK
... NOT VALID takes about 300 ms on master and about 300 ms with the
patch (cassert builds), so I don't see a problem there.
I have not tested the back branches.
The SQL for all the cases above, with controls, is attached.
Regards,
Manu
-- #6842 / BUG #19507: auto-named constraints on a partition tree that spans
schemas.
-- Each scenario uses its own pair of schemas (sN = parent, pN = child).
-- After the statement under test, the constraints of the whole tree are listed.
\set VERBOSITY terse
\pset format unaligned
\pset tuples_only on
create function tree(root regclass) returns table (l text) language sql as $$
select format('%s | %s | %s | local=%s inh=%s', c.conrelid::regclass,
c.conname, c.contype, c.conislocal, c.coninhcount)
from pg_constraint c
where c.conrelid in (select root union all select relid from
pg_partition_tree(root)
union all select inhrelid from pg_inherits where
inhparent = root)
order by c.conrelid::regclass::text, c.conname $$;
\echo --- A1 report: SET NOT NULL, child has a CHECK named t_a_not_null
create schema s1; create schema p1;
create table s1.t(a int) partition by range (a);
create table p1.t_1_10 partition of s1.t for values from (1) to (10);
alter table p1.t_1_10 add constraint t_a_not_null check (a is not null);
\echo => TEST
alter table s1.t alter column a set not null;
select tree('s1.t');
\echo --- A2 report: ADD CHECK, child has a different CHECK named t_a_check
create schema s2; create schema p2;
create table s2.t(a int) partition by range (a);
create table p2.t_1_10 partition of s2.t for values from (1) to (10);
alter table p2.t_1_10 add constraint t_a_check check (a > 100);
\echo => TEST
alter table s2.t add check (a > 0);
select tree('s2.t');
\echo --- A3 report: ADD NOT NULL
create schema s3; create schema p3;
create table s3.t(a int) partition by range (a);
create table p3.t_1_10 partition of s3.t for values from (1) to (10);
alter table p3.t_1_10 add constraint t_a_not_null check (a is not null);
\echo => TEST
alter table s3.t add not null a;
select tree('s3.t');
\echo --- A4 report: nobody named anything, the partition is also called t
create schema s4; create schema p4;
create table s4.t(a int) partition by range (a);
create table p4.t partition of s4.t for values from (1) to (10);
alter table p4.t add check (a > 1);
\echo => TEST
alter table s4.t add check (a > 0);
select tree('s4.t');
\echo --- B1 plain inheritance (not partitioning), CHECK
create schema s5; create schema p5;
create table s5.t(a int);
create table p5.c() inherits (s5.t);
alter table p5.c add constraint t_a_check check (a > 100);
\echo => TEST
alter table s5.t add check (a > 0);
select tree('s5.t');
\echo --- B2 plain inheritance, SET NOT NULL
create schema s6; create schema p6;
create table s6.t(a int);
create table p6.c() inherits (s6.t);
alter table p6.c add constraint t_a_not_null check (a is not null);
\echo => TEST
alter table s6.t alter column a set not null;
select tree('s6.t');
\echo --- B3 three levels, the conflict is on the leaf, in a third schema
create schema s7; create schema p7; create schema q7;
create table s7.t(a int) partition by range (a);
create table p7.mid partition of s7.t for values from (1) to (100) partition by
range (a);
create table q7.leaf partition of p7.mid for values from (1) to (10);
alter table q7.leaf add constraint t_a_check check (a > 100);
\echo => TEST
alter table s7.t add check (a > 0);
select tree('s7.t');
\echo --- B4 two unnamed CHECKs in one command, child owns t_a_check and
t_a_check1
create schema s8; create schema p8;
create table s8.t(a int) partition by range (a);
create table p8.t_1_10 partition of s8.t for values from (1) to (10);
alter table p8.t_1_10 add constraint t_a_check check (a > 100), add constraint
t_a_check1 check (a > 101);
\echo => TEST
alter table s8.t add check (a > 0), add check (a < 1000);
select tree('s8.t');
\echo --- C1 FOREIGN KEY, child has a CHECK named t_a_fkey
create schema s9; create schema p9;
create table s9.ref(id int primary key);
create table s9.t(a int) partition by range (a);
create table p9.t_1_10 partition of s9.t for values from (1) to (10);
alter table p9.t_1_10 add constraint t_a_fkey check (a > 100);
\echo => TEST
alter table s9.t add foreign key (a) references s9.ref;
select tree('s9.t');
\echo --- C2 ADD PRIMARY KEY (creates the not-null too), child has a CHECK
named t_a_not_null
create schema s10; create schema p10;
create table s10.t(a int) partition by range (a);
create table p10.t_1_10 partition of s10.t for values from (1) to (10);
alter table p10.t_1_10 add constraint t_a_not_null check (a is not null);
\echo => TEST
alter table s10.t add primary key (a);
select tree('s10.t');
\echo --- C3 ADD COLUMN ... NOT NULL, child has a CHECK named t_b_not_null
create schema s11; create schema p11;
create table s11.t(a int) partition by range (a);
create table p11.t_1_10 partition of s11.t for values from (1) to (10);
alter table p11.t_1_10 add constraint t_b_not_null check (a is not null);
\echo => TEST
alter table s11.t add column b int not null;
select tree('s11.t');
\echo --- C4 ADD COLUMN ... CHECK, child has a CHECK named t_b_check
create schema s12; create schema p12;
create table s12.t(a int) partition by range (a);
create table p12.t_1_10 partition of s12.t for values from (1) to (10);
alter table p12.t_1_10 add constraint t_b_check check (a > 100);
\echo => TEST
alter table s12.t add column b int check (b > 0);
select tree('s12.t');
\echo --- D1 control: child has the SAME check under the default name (master
merges?)
create schema s13; create schema p13;
create table s13.t(a int) partition by range (a);
create table p13.t_1_10 partition of s13.t for values from (1) to (10);
alter table p13.t_1_10 add constraint t_a_check check (a > 0);
\echo => TEST
alter table s13.t add check (a > 0);
select tree('s13.t');
\echo --- D2 control: child already has a real NOT NULL under the default name
create schema s14; create schema p14;
create table s14.t(a int) partition by range (a);
create table p14.t partition of s14.t for values from (1) to (10);
alter table p14.t alter column a set not null;
\echo => TEST
alter table s14.t alter column a set not null;
select tree('s14.t');
\echo --- D3 control: same as A2 but everything in ONE schema
create schema s15;
create table s15.t(a int) partition by range (a);
create table s15.t_1_10 partition of s15.t for values from (1) to (10);
alter table s15.t_1_10 add constraint t_a_check check (a > 100);
\echo => TEST
alter table s15.t add check (a > 0);
select tree('s15.t');
\echo --- D4 control: no conflict at all, names must not change
create schema s16; create schema p16;
create table s16.t(a int) partition by range (a);
create table p16.t_1_10 partition of s16.t for values from (1) to (10);
\echo => TEST
alter table s16.t add check (a > 0);
alter table s16.t alter column a set not null;
select tree('s16.t');
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 67752149538..7d1068adfe1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5079,9 +5079,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd
*cmd,
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
ATSimplePermissions(cmd->subtype, rel,
ATT_TABLE |
ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
- /* Set up recursion for phase 2; no other prep needed */
if (recurse)
+ {
+ /* recurses at exec time; lock descendants and
set flag */
+ (void)
find_all_inheritors(RelationGetRelid(rel), lockmode, NULL);
cmd->recurse = true;
+ }
pass = AT_PASS_COL_ATTRS;
break;
case AT_SetExpression: /* ALTER COLUMN SET EXPRESSION */
