Re: [PATCH v1] [BUG #19507] Prevent constraint name conflicts in partition trees spanning multiple schemas

Hi Marko,

Marko Grujic <markoog(at)gmail(dot)com> 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