Skip to content

Commit 17b7c30

Browse files
tglsfdctglsfdc
committed
Fully enforce uniqueness of constraint names.
It's been true for a long time that we expect names of table and domain constraints to be unique among the constraints of that table or domain. However, the enforcement of that has been pretty haphazard, and it missed some corner cases such as creating a CHECK constraint and then an index constraint of the same name (as per recent report from André Hänsel). Also, due to the lack of an actual unique index enforcing this, duplicates could be created through race conditions. Moreover, the code that searches pg_constraint has been quite inconsistent about how to handle duplicate names if one did occur: some places checked and threw errors if there was more than one match, while others just processed the first match they came to. To fix, create a unique index on (conrelid, contypid, conname). Since either conrelid or contypid is zero, this will separately enforce uniqueness of constraint names among constraints of any one table and any one domain. (If we ever implement SQL assertions, and put them into this catalog, more thought might be needed. But it'd be at least as reasonable to put them into a new catalog; having overloaded this one catalog with two kinds of constraints was a mistake already IMO.) This index can replace the existing non-unique index on conrelid, though we need to keep the one on contypid for query performance reasons. Having done that, we can simplify the logic in various places that either coped with duplicates or neglected to, as well as potentially improve lookup performance when searching for a constraint by name. Also, as per our usual practice, install a preliminary check so that you get something more friendly than a unique-index violation report in the case complained of by André. And teach ChooseIndexName to avoid choosing autogenerated names that would draw such a failure. While it's not possible to make such a change in the back branches, it doesn't seem quite too late to put this into v11, so do so. Discussion: https://postgr.es/m/0c1001d4428f$0942b430$1bc81c90$@webkr.de
1 parent f30c6f5 commit 17b7c30

File tree

17 files changed

+494
-366
lines changed

17 files changed

+494
-366
lines changed

‎doc/src/sgml/ref/alter_index.sgml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ ALTER INDEX ALL IN TABLESPACE <replaceable class="parameter">name</replaceable>
4848
<listitem>
4949
<para>
5050
The <literal>RENAME</literal> form changes the name of the index.
51+
If the index is associated with a table constraint (either
52+
<literal>UNIQUE</literal>, <literal>PRIMARY KEY</literal>,
53+
or <literal>EXCLUDE</literal>), the constraint is renamed as well.
5154
There is no effect on the stored data.
5255
</para>
5356
</listitem>

‎doc/src/sgml/ref/alter_table.sgml

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,8 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
474474
<term><literal>DROP CONSTRAINT [ IF EXISTS ]</literal></term>
475475
<listitem>
476476
<para>
477-
This form drops the specified constraint on a table.
477+
This form drops the specified constraint on a table, along with
478+
any index underlying the constraint.
478479
If <literal>IF EXISTS</literal> is specified and the constraint
479480
does not exist, no error is thrown. In this case a notice is issued instead.
480481
</para>
@@ -822,8 +823,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
822823
<listitem>
823824
<para>
824825
The <literal>RENAME</literal> forms change the name of a table
825-
(or an index, sequence, view, materialized view, or foreign table), the name
826-
of an individual column in a table, or the name of a constraint of the table.
826+
(or an index, sequence, view, materialized view, or foreign table), the
827+
name of an individual column in a table, or the name of a constraint of
828+
the table. When renaming a constraint that has an underlying index,
829+
the index is renamed as well.
827830
There is no effect on the stored data.
828831
</para>
829832
</listitem>
@@ -1270,10 +1273,12 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
12701273
<para>
12711274
If a table has any descendant tables, it is not permitted to add,
12721275
rename, or change the type of a column in the parent table without doing
1273-
same to the descendants. This ensures that the descendants always have
1274-
columns matching the parent. Similarly, a constraint cannot be renamed
1275-
in the parent without also renaming it in all descendants, so that
1276-
constraints also match between the parent and its descendants.
1276+
the same to the descendants. This ensures that the descendants always
1277+
have columns matching the parent. Similarly, a <literal>CHECK</literal>
1278+
constraint cannot be renamed in the parent without also renaming it in
1279+
all descendants, so that <literal>CHECK</literal> constraints also match
1280+
between the parent and its descendants. (That restriction does not apply
1281+
to index-based constraints, however.)
12771282
Also, because selecting from the parent also selects from its descendants,
12781283
a constraint on the parent cannot be marked valid unless it is also marked
12791284
valid for those descendants. In all of these cases, <command>ALTER TABLE
@@ -1481,35 +1486,35 @@ ALTER TABLE distributors DROP CONSTRAINT distributors_pkey,
14811486
</programlisting></para>
14821487

14831488
<para>
1484-
Attach a partition to range partitioned table:
1489+
To attach a partition to a range-partitioned table:
14851490
<programlisting>
14861491
ALTER TABLE measurement
14871492
ATTACH PARTITION measurement_y2016m07 FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
14881493
</programlisting></para>
14891494

14901495
<para>
1491-
Attach a partition to list partitioned table:
1496+
To attach a partition to a list-partitioned table:
14921497
<programlisting>
14931498
ALTER TABLE cities
14941499
ATTACH PARTITION cities_ab FOR VALUES IN ('a', 'b');
14951500
</programlisting></para>
14961501

14971502
<para>
1498-
Attach a default partition to a partitioned table:
1503+
To attach a partition to a hash-partitioned table:
14991504
<programlisting>
1500-
ALTER TABLE cities
1501-
ATTACH PARTITION cities_partdef DEFAULT;
1505+
ALTER TABLE orders
1506+
ATTACH PARTITION orders_p4 FOR VALUES WITH (MODULUS 4, REMAINDER 3);
15021507
</programlisting></para>
15031508

15041509
<para>
1505-
Attach a partition to hash partitioned table:
1510+
To attach a default partition to a partitioned table:
15061511
<programlisting>
1507-
ALTER TABLE orders
1508-
ATTACH PARTITION orders_p4 FOR VALUES WITH (MODULUS 4, REMAINDER 3);
1512+
ALTER TABLE cities
1513+
ATTACH PARTITION cities_partdef DEFAULT;
15091514
</programlisting></para>
15101515

15111516
<para>
1512-
Detach a partition from partitioned table:
1517+
To detach a partition from a partitioned table:
15131518
<programlisting>
15141519
ALTER TABLE measurement
15151520
DETACH PARTITION measurement_y2015m12;

‎doc/src/sgml/ref/create_table.sgml

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2004,6 +2004,30 @@ CREATE TABLE cities_partdef
20042004
</para>
20052005
</refsect2>
20062006

2007+
<refsect2>
2008+
<title>Constraint Naming</title>
2009+
2010+
<para>
2011+
The SQL standard says that table and domain constraints must have names
2012+
that are unique across the schema containing the table or domain.
2013+
<productname>PostgreSQL</productname> is laxer: it only requires
2014+
constraint names to be unique across the constraints attached to a
2015+
particular table or domain. However, this extra freedom does not exist
2016+
for index-based constraints (<literal>UNIQUE</literal>,
2017+
<literal>PRIMARY KEY</literal>, and <literal>EXCLUDE</literal>
2018+
constraints), because the associated index is named the same as the
2019+
constraint, and index names must be unique across all relations within
2020+
the same schema.
2021+
</para>
2022+
2023+
<para>
2024+
Currently, <productname>PostgreSQL</productname> does not record names
2025+
for <literal>NOT NULL</literal> constraints at all, so they are not
2026+
subject to the uniqueness restriction. This might change in a future
2027+
release.
2028+
</para>
2029+
</refsect2>
2030+
20072031
<refsect2>
20082032
<title>Inheritance</title>
20092033

‎src/backend/catalog/heap.c

Lines changed: 95 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -2708,7 +2708,7 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
27082708
bool found;
27092709
Relation conDesc;
27102710
SysScanDesc conscan;
2711-
ScanKeyData skey[2];
2711+
ScanKeyData skey[3];
27122712
HeapTuple tup;
27132713

27142714
/* Search for a pg_constraint entry with same name and relation */
@@ -2717,120 +2717,120 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
27172717
found = false;
27182718

27192719
ScanKeyInit(&skey[0],
2720+
Anum_pg_constraint_conrelid,
2721+
BTEqualStrategyNumber, F_OIDEQ,
2722+
ObjectIdGetDatum(RelationGetRelid(rel)));
2723+
ScanKeyInit(&skey[1],
2724+
Anum_pg_constraint_contypid,
2725+
BTEqualStrategyNumber, F_OIDEQ,
2726+
ObjectIdGetDatum(InvalidOid));
2727+
ScanKeyInit(&skey[2],
27202728
Anum_pg_constraint_conname,
27212729
BTEqualStrategyNumber, F_NAMEEQ,
27222730
CStringGetDatum(ccname));
27232731

2724-
ScanKeyInit(&skey[1],
2725-
Anum_pg_constraint_connamespace,
2726-
BTEqualStrategyNumber, F_OIDEQ,
2727-
ObjectIdGetDatum(RelationGetNamespace(rel)));
2728-
2729-
conscan = systable_beginscan(conDesc, ConstraintNameNspIndexId, true,
2730-
NULL, 2, skey);
2732+
conscan = systable_beginscan(conDesc, ConstraintRelidTypidNameIndexId, true,
2733+
NULL, 3, skey);
27312734

2732-
while (HeapTupleIsValid(tup = systable_getnext(conscan)))
2735+
/* There can be at most one matching row */
2736+
if (HeapTupleIsValid(tup = systable_getnext(conscan)))
27332737
{
27342738
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(tup);
27352739

2736-
if (con->conrelid == RelationGetRelid(rel))
2740+
/* Found it. Conflicts if not identical check constraint */
2741+
if (con->contype == CONSTRAINT_CHECK)
27372742
{
2738-
/* Found it. Conflicts if not identical check constraint */
2739-
if (con->contype == CONSTRAINT_CHECK)
2740-
{
2741-
Datum val;
2742-
bool isnull;
2743-
2744-
val = fastgetattr(tup,
2745-
Anum_pg_constraint_conbin,
2746-
conDesc->rd_att, &isnull);
2747-
if (isnull)
2748-
elog(ERROR, "null conbin for rel %s",
2749-
RelationGetRelationName(rel));
2750-
if (equal(expr, stringToNode(TextDatumGetCString(val))))
2751-
found = true;
2752-
}
2743+
Datum val;
2744+
bool isnull;
2745+
2746+
val = fastgetattr(tup,
2747+
Anum_pg_constraint_conbin,
2748+
conDesc->rd_att, &isnull);
2749+
if (isnull)
2750+
elog(ERROR, "null conbin for rel %s",
2751+
RelationGetRelationName(rel));
2752+
if (equal(expr, stringToNode(TextDatumGetCString(val))))
2753+
found = true;
2754+
}
27532755

2754-
/*
2755-
* If the existing constraint is purely inherited (no local
2756-
* definition) then interpret addition of a local constraint as a
2757-
* legal merge. This allows ALTER ADD CONSTRAINT on parent and
2758-
* child tables to be given in either order with same end state.
2759-
* However if the relation is a partition, all inherited
2760-
* constraints are always non-local, including those that were
2761-
* merged.
2762-
*/
2763-
if (is_local && !con->conislocal && !rel->rd_rel->relispartition)
2764-
allow_merge = true;
2756+
/*
2757+
* If the existing constraint is purely inherited (no local
2758+
* definition) then interpret addition of a local constraint as a
2759+
* legal merge. This allows ALTER ADD CONSTRAINT on parent and child
2760+
* tables to be given in either order with same end state. However if
2761+
* the relation is a partition, all inherited constraints are always
2762+
* non-local, including those that were merged.
2763+
*/
2764+
if (is_local && !con->conislocal && !rel->rd_rel->relispartition)
2765+
allow_merge = true;
27652766

2766-
if (!found || !allow_merge)
2767-
ereport(ERROR,
2768-
(errcode(ERRCODE_DUPLICATE_OBJECT),
2769-
errmsg("constraint \"%s\" for relation \"%s\" already exists",
2770-
ccname, RelationGetRelationName(rel))));
2767+
if (!found || !allow_merge)
2768+
ereport(ERROR,
2769+
(errcode(ERRCODE_DUPLICATE_OBJECT),
2770+
errmsg("constraint \"%s\" for relation \"%s\" already exists",
2771+
ccname, RelationGetRelationName(rel))));
27712772

2772-
/* If the child constraint is "no inherit" then cannot merge */
2773-
if (con->connoinherit)
2774-
ereport(ERROR,
2775-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
2776-
errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"",
2777-
ccname, RelationGetRelationName(rel))));
2773+
/* If the child constraint is "no inherit" then cannot merge */
2774+
if (con->connoinherit)
2775+
ereport(ERROR,
2776+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
2777+
errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"",
2778+
ccname, RelationGetRelationName(rel))));
27782779

2779-
/*
2780-
* Must not change an existing inherited constraint to "no
2781-
* inherit" status. That's because inherited constraints should
2782-
* be able to propagate to lower-level children.
2783-
*/
2784-
if (con->coninhcount > 0 && is_no_inherit)
2785-
ereport(ERROR,
2786-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
2787-
errmsg("constraint \"%s\" conflicts with inherited constraint on relation \"%s\"",
2788-
ccname, RelationGetRelationName(rel))));
2780+
/*
2781+
* Must not change an existing inherited constraint to "no inherit"
2782+
* status. That's because inherited constraints should be able to
2783+
* propagate to lower-level children.
2784+
*/
2785+
if (con->coninhcount > 0 && is_no_inherit)
2786+
ereport(ERROR,
2787+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
2788+
errmsg("constraint \"%s\" conflicts with inherited constraint on relation \"%s\"",
2789+
ccname, RelationGetRelationName(rel))));
27892790

2790-
/*
2791-
* If the child constraint is "not valid" then cannot merge with a
2792-
* valid parent constraint
2793-
*/
2794-
if (is_initially_valid && !con->convalidated)
2795-
ereport(ERROR,
2796-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
2797-
errmsg("constraint \"%s\" conflicts with NOT VALID constraint on relation \"%s\"",
2798-
ccname, RelationGetRelationName(rel))));
2791+
/*
2792+
* If the child constraint is "not valid" then cannot merge with a
2793+
* valid parent constraint.
2794+
*/
2795+
if (is_initially_valid && !con->convalidated)
2796+
ereport(ERROR,
2797+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
2798+
errmsg("constraint \"%s\" conflicts with NOT VALID constraint on relation \"%s\"",
2799+
ccname, RelationGetRelationName(rel))));
27992800

2800-
/* OK to update the tuple */
2801-
ereport(NOTICE,
2802-
(errmsg("merging constraint \"%s\" with inherited definition",
2803-
ccname)));
2801+
/* OK to update the tuple */
2802+
ereport(NOTICE,
2803+
(errmsg("merging constraint \"%s\" with inherited definition",
2804+
ccname)));
28042805

2805-
tup = heap_copytuple(tup);
2806-
con = (Form_pg_constraint) GETSTRUCT(tup);
2806+
tup = heap_copytuple(tup);
2807+
con = (Form_pg_constraint) GETSTRUCT(tup);
28072808

2808-
/*
2809-
* In case of partitions, an inherited constraint must be
2810-
* inherited only once since it cannot have multiple parents and
2811-
* it is never considered local.
2812-
*/
2813-
if (rel->rd_rel->relispartition)
2814-
{
2815-
con->coninhcount = 1;
2816-
con->conislocal = false;
2817-
}
2809+
/*
2810+
* In case of partitions, an inherited constraint must be inherited
2811+
* only once since it cannot have multiple parents and it is never
2812+
* considered local.
2813+
*/
2814+
if (rel->rd_rel->relispartition)
2815+
{
2816+
con->coninhcount = 1;
2817+
con->conislocal = false;
2818+
}
2819+
else
2820+
{
2821+
if (is_local)
2822+
con->conislocal = true;
28182823
else
2819-
{
2820-
if (is_local)
2821-
con->conislocal = true;
2822-
else
2823-
con->coninhcount++;
2824-
}
2824+
con->coninhcount++;
2825+
}
28252826

2826-
if (is_no_inherit)
2827-
{
2828-
Assert(is_local);
2829-
con->connoinherit = true;
2830-
}
2831-
CatalogTupleUpdate(conDesc, &tup->t_self, tup);
2832-
break;
2827+
if (is_no_inherit)
2828+
{
2829+
Assert(is_local);
2830+
con->connoinherit = true;
28332831
}
2832+
2833+
CatalogTupleUpdate(conDesc, &tup->t_self, tup);
28342834
}
28352835

28362836
systable_endscan(conscan);

‎src/backend/catalog/index.c

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -842,6 +842,12 @@ index_create(Relation heapRelation,
842842
if (shared_relation && tableSpaceId != GLOBALTABLESPACE_OID)
843843
elog(ERROR, "shared relations must be placed in pg_global tablespace");
844844

845+
/*
846+
* Check for duplicate name (both as to the index, and as to the
847+
* associated constraint if any). Such cases would fail on the relevant
848+
* catalogs' unique indexes anyway, but we prefer to give a friendlier
849+
* error message.
850+
*/
845851
if (get_relname_relid(indexRelationName, namespaceId))
846852
{
847853
if ((flags & INDEX_CREATE_IF_NOT_EXISTS) != 0)
@@ -860,6 +866,20 @@ index_create(Relation heapRelation,
860866
indexRelationName)));
861867
}
862868

869+
if ((flags & INDEX_CREATE_ADD_CONSTRAINT) != 0 &&
870+
ConstraintNameIsUsed(CONSTRAINT_RELATION, heapRelationId,
871+
indexRelationName))
872+
{
873+
/*
874+
* INDEX_CREATE_IF_NOT_EXISTS does not apply here, since the
875+
* conflicting constraint is not an index.
876+
*/
877+
ereport(ERROR,
878+
(errcode(ERRCODE_DUPLICATE_OBJECT),
879+
errmsg("constraint \"%s\" for relation \"%s\" already exists",
880+
indexRelationName, RelationGetRelationName(heapRelation))));
881+
}
882+
863883
/*
864884
* construct tuple descriptor for index tuples
865885
*/

0 commit comments

Comments
 (0)