Skip to content

Commit 47ea461

Browse files
tglsfdctglsfdc
committed
Fix handling of inherited check constraints in ALTER COLUMN TYPE (again).
The previous way of reconstructing check constraints was to do a separate "ALTER TABLE ONLY tab ADD CONSTRAINT" for each table in an inheritance hierarchy. However, that way has no hope of reconstructing the check constraints' own inheritance properties correctly, as pointed out in bug #13779 from Jan Dirk Zijlstra. What we should do instead is to do a regular "ALTER TABLE", allowing recursion, at the topmost table that has a particular constraint, and then suppress the work queue entries for inherited instances of the constraint. Annoyingly, we'd tried to fix this behavior before, in commit 5ed6546, but we failed to notice that it wasn't reconstructing the pg_constraint field values correctly. As long as I'm touching pg_get_constraintdef_worker anyway, tweak it to always schema-qualify the target table name; this seems like useful backup to the protections installed by commit 5f17304. In HEAD/9.5, get rid of get_constraint_relation_oids, which is now unused. (I could alternatively have modified it to also return conislocal, but that seemed like a pretty single-purpose API, so let's not pretend it has some other use.) It's unused in the back branches as well, but I left it in place just in case some third-party code has decided to use it. In HEAD/9.5, also rename pg_get_constraintdef_string to pg_get_constraintdef_command, as the previous name did nothing to explain what that entry point did differently from others (and its comment was equally useless). Again, that change doesn't seem like material for back-patching. I did a bit of re-pgindenting in tablecmds.c in HEAD/9.5, as well. Otherwise, back-patch to all supported branches.
1 parent 9892cc2 commit 47ea461

File tree

4 files changed

+229
-41
lines changed

4 files changed

+229
-41
lines changed

‎src/backend/commands/tablecmds.c

+24-13
Original file line numberDiff line numberDiff line change
@@ -3390,7 +3390,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
33903390
case AT_ReAddConstraint: /* Re-add pre-existing check
33913391
* constraint */
33923392
ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
3393-
false, true, lockmode);
3393+
true, true, lockmode);
33943394
break;
33953395
case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
33963396
ATExecAddIndexConstraint(tab, rel, (IndexStmt *) cmd->def, lockmode);
@@ -5764,13 +5764,6 @@ ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
57645764
* AddRelationNewConstraints would normally assign different names to the
57655765
* child constraints. To fix that, we must capture the name assigned at
57665766
* the parent table and pass that down.
5767-
*
5768-
* When re-adding a previously existing constraint (during ALTER COLUMN TYPE),
5769-
* we don't need to recurse here, because recursion will be carried out at a
5770-
* higher level; the constraint name issue doesn't apply because the names
5771-
* have already been assigned and are just being re-used. We need a separate
5772-
* "is_readd" flag for that; just setting recurse=false would result in an
5773-
* error if there are child tables.
57745767
*/
57755768
static void
57765769
ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
@@ -5798,7 +5791,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
57985791
*/
57995792
newcons = AddRelationNewConstraints(rel, NIL,
58005793
list_make1(copyObject(constr)),
5801-
recursing, /* allow_merge */
5794+
recursing | is_readd, /* allow_merge */
58025795
!recursing, /* is_local */
58035796
is_readd); /* is_internal */
58045797

@@ -5842,10 +5835,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
58425835

58435836
/*
58445837
* If adding a NO INHERIT constraint, no need to find our children.
5845-
* Likewise, in a re-add operation, we don't need to recurse (that will be
5846-
* handled at higher levels).
58475838
*/
5848-
if (constr->is_no_inherit || is_readd)
5839+
if (constr->is_no_inherit)
58495840
return;
58505841

58515842
/*
@@ -8204,10 +8195,30 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
82048195
def_item, tab->changedConstraintDefs)
82058196
{
82068197
Oid oldId = lfirst_oid(oid_item);
8198+
HeapTuple tup;
8199+
Form_pg_constraint con;
82078200
Oid relid;
82088201
Oid confrelid;
8202+
bool conislocal;
8203+
8204+
tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId));
8205+
if (!HeapTupleIsValid(tup)) /* should not happen */
8206+
elog(ERROR, "cache lookup failed for constraint %u", oldId);
8207+
con = (Form_pg_constraint) GETSTRUCT(tup);
8208+
relid = con->conrelid;
8209+
confrelid = con->confrelid;
8210+
conislocal = con->conislocal;
8211+
ReleaseSysCache(tup);
8212+
8213+
/*
8214+
* If the constraint is inherited (only), we don't want to inject a
8215+
* new definition here; it'll get recreated when ATAddCheckConstraint
8216+
* recurses from adding the parent table's constraint. But we had to
8217+
* carry the info this far so that we can drop the constraint below.
8218+
*/
8219+
if (!conislocal)
8220+
continue;
82098221

8210-
get_constraint_relation_oids(oldId, &relid, &confrelid);
82118222
ATPostAlterTypeParse(oldId, relid, confrelid,
82128223
(char *) lfirst(def_item),
82138224
wqueue, lockmode, tab->rewrite);

‎src/backend/utils/adt/ruleutils.c

+47-24
Original file line numberDiff line numberDiff line change
@@ -420,6 +420,7 @@ static Node *processIndirection(Node *node, deparse_context *context,
420420
static void printSubscripts(ArrayRef *aref, deparse_context *context);
421421
static char *get_relation_name(Oid relid);
422422
static char *generate_relation_name(Oid relid, List *namespaces);
423+
static char *generate_qualified_relation_name(Oid relid);
423424
static char *generate_function_name(Oid funcid, int nargs,
424425
List *argnames, Oid *argtypes,
425426
bool has_variadic, bool *use_variadic_p);
@@ -1298,7 +1299,9 @@ pg_get_constraintdef_ext(PG_FUNCTION_ARGS)
12981299
prettyFlags)));
12991300
}
13001301

1301-
/* Internal version that returns a palloc'd C string; no pretty-printing */
1302+
/*
1303+
* Internal version that returns a full ALTER TABLE ... ADD CONSTRAINT command
1304+
*/
13021305
char *
13031306
pg_get_constraintdef_string(Oid constraintId)
13041307
{
@@ -1347,10 +1350,16 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
13471350

13481351
initStringInfo(&buf);
13491352

1350-
if (fullCommand && OidIsValid(conForm->conrelid))
1353+
if (fullCommand)
13511354
{
1352-
appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
1353-
generate_relation_name(conForm->conrelid, NIL),
1355+
/*
1356+
* Currently, callers want ALTER TABLE (without ONLY) for CHECK
1357+
* constraints, and other types of constraints don't inherit anyway so
1358+
* it doesn't matter whether we say ONLY or not. Someday we might
1359+
* need to let callers specify whether to put ONLY in the command.
1360+
*/
1361+
appendStringInfo(&buf, "ALTER TABLE %s ADD CONSTRAINT %s ",
1362+
generate_qualified_relation_name(conForm->conrelid),
13541363
quote_identifier(NameStr(conForm->conname)));
13551364
}
13561365

@@ -1874,28 +1883,9 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
18741883

18751884
if (OidIsValid(sequenceId))
18761885
{
1877-
HeapTuple classtup;
1878-
Form_pg_class classtuple;
1879-
char *nspname;
18801886
char *result;
18811887

1882-
/* Get the sequence's pg_class entry */
1883-
classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(sequenceId));
1884-
if (!HeapTupleIsValid(classtup))
1885-
elog(ERROR, "cache lookup failed for relation %u", sequenceId);
1886-
classtuple = (Form_pg_class) GETSTRUCT(classtup);
1887-
1888-
/* Get the namespace */
1889-
nspname = get_namespace_name(classtuple->relnamespace);
1890-
if (!nspname)
1891-
elog(ERROR, "cache lookup failed for namespace %u",
1892-
classtuple->relnamespace);
1893-
1894-
/* And construct the result string */
1895-
result = quote_qualified_identifier(nspname,
1896-
NameStr(classtuple->relname));
1897-
1898-
ReleaseSysCache(classtup);
1888+
result = generate_qualified_relation_name(sequenceId);
18991889

19001890
PG_RETURN_TEXT_P(string_to_text(result));
19011891
}
@@ -9138,6 +9128,39 @@ generate_relation_name(Oid relid, List *namespaces)
91389128
return result;
91399129
}
91409130

9131+
/*
9132+
* generate_qualified_relation_name
9133+
* Compute the name to display for a relation specified by OID
9134+
*
9135+
* As above, but unconditionally schema-qualify the name.
9136+
*/
9137+
static char *
9138+
generate_qualified_relation_name(Oid relid)
9139+
{
9140+
HeapTuple tp;
9141+
Form_pg_class reltup;
9142+
char *relname;
9143+
char *nspname;
9144+
char *result;
9145+
9146+
tp = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
9147+
if (!HeapTupleIsValid(tp))
9148+
elog(ERROR, "cache lookup failed for relation %u", relid);
9149+
reltup = (Form_pg_class) GETSTRUCT(tp);
9150+
relname = NameStr(reltup->relname);
9151+
9152+
nspname = get_namespace_name(reltup->relnamespace);
9153+
if (!nspname)
9154+
elog(ERROR, "cache lookup failed for namespace %u",
9155+
reltup->relnamespace);
9156+
9157+
result = quote_qualified_identifier(nspname, relname);
9158+
9159+
ReleaseSysCache(tp);
9160+
9161+
return result;
9162+
}
9163+
91419164
/*
91429165
* generate_function_name
91439166
* Compute the name to display for a function specified by OID,

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

+128-2
Original file line numberDiff line numberDiff line change
@@ -1805,16 +1805,125 @@ where oid = 'test_storage'::regclass;
18051805
t
18061806
(1 row)
18071807

1808-
-- ALTER TYPE with a check constraint and a child table (bug before Nov 2012)
1809-
CREATE TABLE test_inh_check (a float check (a > 10.2));
1808+
-- ALTER COLUMN TYPE with a check constraint and a child table (bug #13779)
1809+
CREATE TABLE test_inh_check (a float check (a > 10.2), b float);
18101810
CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
1811+
\d test_inh_check
1812+
Table "public.test_inh_check"
1813+
Column | Type | Modifiers
1814+
--------+------------------+-----------
1815+
a | double precision |
1816+
b | double precision |
1817+
Check constraints:
1818+
"test_inh_check_a_check" CHECK (a > 10.2::double precision)
1819+
Number of child tables: 1 (Use \d+ to list them.)
1820+
1821+
\d test_inh_check_child
1822+
Table "public.test_inh_check_child"
1823+
Column | Type | Modifiers
1824+
--------+------------------+-----------
1825+
a | double precision |
1826+
b | double precision |
1827+
Check constraints:
1828+
"test_inh_check_a_check" CHECK (a > 10.2::double precision)
1829+
Inherits: test_inh_check
1830+
1831+
select relname, conname, coninhcount, conislocal, connoinherit
1832+
from pg_constraint c, pg_class r
1833+
where relname like 'test_inh_check%' and c.conrelid = r.oid
1834+
order by 1, 2;
1835+
relname | conname | coninhcount | conislocal | connoinherit
1836+
----------------------+------------------------+-------------+------------+--------------
1837+
test_inh_check | test_inh_check_a_check | 0 | t | f
1838+
test_inh_check_child | test_inh_check_a_check | 1 | f | f
1839+
(2 rows)
1840+
18111841
ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric;
1842+
\d test_inh_check
1843+
Table "public.test_inh_check"
1844+
Column | Type | Modifiers
1845+
--------+------------------+-----------
1846+
a | numeric |
1847+
b | double precision |
1848+
Check constraints:
1849+
"test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
1850+
Number of child tables: 1 (Use \d+ to list them.)
1851+
1852+
\d test_inh_check_child
1853+
Table "public.test_inh_check_child"
1854+
Column | Type | Modifiers
1855+
--------+------------------+-----------
1856+
a | numeric |
1857+
b | double precision |
1858+
Check constraints:
1859+
"test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
1860+
Inherits: test_inh_check
1861+
1862+
select relname, conname, coninhcount, conislocal, connoinherit
1863+
from pg_constraint c, pg_class r
1864+
where relname like 'test_inh_check%' and c.conrelid = r.oid
1865+
order by 1, 2;
1866+
relname | conname | coninhcount | conislocal | connoinherit
1867+
----------------------+------------------------+-------------+------------+--------------
1868+
test_inh_check | test_inh_check_a_check | 0 | t | f
1869+
test_inh_check_child | test_inh_check_a_check | 1 | f | f
1870+
(2 rows)
1871+
1872+
-- also try noinherit, local, and local+inherited cases
1873+
ALTER TABLE test_inh_check ADD CONSTRAINT bnoinherit CHECK (b > 100) NO INHERIT;
1874+
ALTER TABLE test_inh_check_child ADD CONSTRAINT blocal CHECK (b < 1000);
1875+
ALTER TABLE test_inh_check_child ADD CONSTRAINT bmerged CHECK (b > 1);
1876+
ALTER TABLE test_inh_check ADD CONSTRAINT bmerged CHECK (b > 1);
1877+
NOTICE: merging constraint "bmerged" with inherited definition
1878+
\d test_inh_check
1879+
Table "public.test_inh_check"
1880+
Column | Type | Modifiers
1881+
--------+------------------+-----------
1882+
a | numeric |
1883+
b | double precision |
1884+
Check constraints:
1885+
"bmerged" CHECK (b > 1::double precision)
1886+
"bnoinherit" CHECK (b > 100::double precision) NO INHERIT
1887+
"test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
1888+
Number of child tables: 1 (Use \d+ to list them.)
1889+
1890+
\d test_inh_check_child
1891+
Table "public.test_inh_check_child"
1892+
Column | Type | Modifiers
1893+
--------+------------------+-----------
1894+
a | numeric |
1895+
b | double precision |
1896+
Check constraints:
1897+
"blocal" CHECK (b < 1000::double precision)
1898+
"bmerged" CHECK (b > 1::double precision)
1899+
"test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
1900+
Inherits: test_inh_check
1901+
1902+
select relname, conname, coninhcount, conislocal, connoinherit
1903+
from pg_constraint c, pg_class r
1904+
where relname like 'test_inh_check%' and c.conrelid = r.oid
1905+
order by 1, 2;
1906+
relname | conname | coninhcount | conislocal | connoinherit
1907+
----------------------+------------------------+-------------+------------+--------------
1908+
test_inh_check | bmerged | 0 | t | f
1909+
test_inh_check | bnoinherit | 0 | t | t
1910+
test_inh_check | test_inh_check_a_check | 0 | t | f
1911+
test_inh_check_child | blocal | 0 | t | f
1912+
test_inh_check_child | bmerged | 1 | t | f
1913+
test_inh_check_child | test_inh_check_a_check | 1 | f | f
1914+
(6 rows)
1915+
1916+
ALTER TABLE test_inh_check ALTER COLUMN b TYPE numeric;
1917+
NOTICE: merging constraint "bmerged" with inherited definition
18121918
\d test_inh_check
18131919
Table "public.test_inh_check"
18141920
Column | Type | Modifiers
18151921
--------+---------+-----------
18161922
a | numeric |
1923+
b | numeric |
18171924
Check constraints:
1925+
"bmerged" CHECK (b::double precision > 1::double precision)
1926+
"bnoinherit" CHECK (b::double precision > 100::double precision) NO INHERIT
18181927
"test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
18191928
Number of child tables: 1 (Use \d+ to list them.)
18201929

@@ -1823,10 +1932,27 @@ Table "public.test_inh_check_child"
18231932
Column | Type | Modifiers
18241933
--------+---------+-----------
18251934
a | numeric |
1935+
b | numeric |
18261936
Check constraints:
1937+
"blocal" CHECK (b::double precision < 1000::double precision)
1938+
"bmerged" CHECK (b::double precision > 1::double precision)
18271939
"test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
18281940
Inherits: test_inh_check
18291941

1942+
select relname, conname, coninhcount, conislocal, connoinherit
1943+
from pg_constraint c, pg_class r
1944+
where relname like 'test_inh_check%' and c.conrelid = r.oid
1945+
order by 1, 2;
1946+
relname | conname | coninhcount | conislocal | connoinherit
1947+
----------------------+------------------------+-------------+------------+--------------
1948+
test_inh_check | bmerged | 0 | t | f
1949+
test_inh_check | bnoinherit | 0 | t | t
1950+
test_inh_check | test_inh_check_a_check | 0 | t | f
1951+
test_inh_check_child | blocal | 0 | t | f
1952+
test_inh_check_child | bmerged | 1 | t | f
1953+
test_inh_check_child | test_inh_check_a_check | 1 | f | f
1954+
(6 rows)
1955+
18301956
-- check for rollback of ANALYZE corrupting table property flags (bug #11638)
18311957
CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);
18321958
CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t text);

‎src/test/regress/sql/alter_table.sql

+30-2
Original file line numberDiff line numberDiff line change
@@ -1252,12 +1252,40 @@ select reltoastrelid <> 0 as has_toast_table
12521252
from pg_class
12531253
where oid = 'test_storage'::regclass;
12541254

1255-
-- ALTER TYPE with a check constraint and a child table (bug before Nov 2012)
1256-
CREATE TABLE test_inh_check (a float check (a > 10.2));
1255+
-- ALTER COLUMN TYPE with a check constraint and a child table (bug #13779)
1256+
CREATE TABLE test_inh_check (a float check (a > 10.2), b float);
12571257
CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
1258+
\d test_inh_check
1259+
\d test_inh_check_child
1260+
select relname, conname, coninhcount, conislocal, connoinherit
1261+
from pg_constraint c, pg_class r
1262+
where relname like 'test_inh_check%' and c.conrelid = r.oid
1263+
order by 1, 2;
12581264
ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric;
12591265
\d test_inh_check
12601266
\d test_inh_check_child
1267+
select relname, conname, coninhcount, conislocal, connoinherit
1268+
from pg_constraint c, pg_class r
1269+
where relname like 'test_inh_check%' and c.conrelid = r.oid
1270+
order by 1, 2;
1271+
-- also try noinherit, local, and local+inherited cases
1272+
ALTER TABLE test_inh_check ADD CONSTRAINT bnoinherit CHECK (b > 100) NO INHERIT;
1273+
ALTER TABLE test_inh_check_child ADD CONSTRAINT blocal CHECK (b < 1000);
1274+
ALTER TABLE test_inh_check_child ADD CONSTRAINT bmerged CHECK (b > 1);
1275+
ALTER TABLE test_inh_check ADD CONSTRAINT bmerged CHECK (b > 1);
1276+
\d test_inh_check
1277+
\d test_inh_check_child
1278+
select relname, conname, coninhcount, conislocal, connoinherit
1279+
from pg_constraint c, pg_class r
1280+
where relname like 'test_inh_check%' and c.conrelid = r.oid
1281+
order by 1, 2;
1282+
ALTER TABLE test_inh_check ALTER COLUMN b TYPE numeric;
1283+
\d test_inh_check
1284+
\d test_inh_check_child
1285+
select relname, conname, coninhcount, conislocal, connoinherit
1286+
from pg_constraint c, pg_class r
1287+
where relname like 'test_inh_check%' and c.conrelid = r.oid
1288+
order by 1, 2;
12611289

12621290
-- check for rollback of ANALYZE corrupting table property flags (bug #11638)
12631291
CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);

0 commit comments

Comments
 (0)