8000 Fix handling of inherited check constraints in ALTER COLUMN TYPE (aga… · home201448/postgres@64349f1 · GitHub
[go: up one dir, main page]
Skip to content

Commit 64349f1

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 ae81d4f commit 64349f1

File tree

4 files changed

+232
-44
lines changed

4 files changed

+232
-44
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 27 additions & 16 deletions
< 629A td data-grid-cell-id="diff-d373fb790605aded212c2e3a0fbe8c015d110077ff46c07d0a6d6647a2d7e928-5733-5723-1" data-selected="false" role="gridcell" style="background-color:var(--diffBlob-deletionNum-bgColor, var(--diffBlob-deletion-bgColor-num));text-align:center" tabindex="-1" valign="top" class="focusable-grid-cell diff-line-number position-relative left-side">
Original file line numberDiff line numberDiff line change
@@ -3298,7 +3298,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
32983298
case AT_ReAddConstraint: /* Re-add pre-existing check
32993299
* constraint */
33003300
ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
3301-
false, true, lockmode);
3301+
true, true, lockmode);
33023302
break;
33033303
case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
33043304
ATExecAddIndexConstraint(tab, rel, (IndexStmt *) cmd->def, lockmode);
@@ -5649,13 +5649,6 @@ ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
56495649
* AddRelationNewConstraints would normally assign different names to the
56505650
* child constraints. To fix that, we must capture the name assigned at
56515651
* the parent table and pass that down.
5652-
*
5653-
* When re-adding a previously existing constraint (during ALTER COLUMN TYPE),
5654-
* we don't need to recurse here, because recursion will be carried out at a
5655-
* higher level; the constraint name issue doesn't apply because the names
5656-
* have already been assigned and are just being re-used. We need a separate
5657-
* "is_readd" flag for that; just setting recurse=false would result in an
5658-
* error if there are child tables.
56595652
*/
56605653
static void
56615654
ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
@@ -5683,7 +5676,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
56835676
*/
56845677
newcons = AddRelationNewConstraints(rel, NIL,
56855678
list_make1(copyObject(constr)),
5686-
recursing, /* allow_merge */
5679+
recursing | is_readd, /* allow_merge */
56875680
!recursing, /* is_local */
56885681
is_readd); /* is_internal */
56895682

@@ -5727,10 +5720,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
57275720

57285721
/*
57295722
* If adding a NO INHERIT constraint, no need to find our children.
5730-
* Likewise, in a re-add operation, we don't need to recurse (that will be
5731-
* handled at higher levels).
57325723
*/
5733-
if (constr->is_no_inherit || is_readd)
5724+
if (constr->is_no_inherit)
57345725
return;
57355726

57365727
/*
@@ -7937,11 +7928,31 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
79377928
forboth(oid_item, tab->changedConstraintOids,
79387929
def_item, tab->changedConstraintDefs)
79397930
{
7940-
Oid oldId = lfirst_oid(oid_item);
7941-
Oid relid;
7942-
Oid confrelid;
7931+
Oid oldId = lfirst_oid(oid_item);
7932+
HeapTuple tup;
7933+
Form_pg_constraint con;
7934+
Oid relid;
7935+
A92E Oid confrelid;
7936+
bool conislocal;
7937+
7938+
tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId));
7939+
if (!HeapTupleIsValid(tup)) /* should not happen */
7940+
elog(ERROR, "cache lookup failed for constraint %u", oldId);
7941+
con = (Form_pg_constraint) GETSTRUCT(tup);
7942+
relid = con->conrelid;
7943+
confrelid = con->confrelid;
7944+
conislocal = con->conislocal;
7945+
ReleaseSysCache(tup);
7946+
7947+
/*
7948+
* If the constraint is inherited (only), we don't want to inject a
7949+
* new definition here; it'll get recreated when ATAddCheckConstraint
7950+
* recurses from adding the parent table's constraint. But we had to
7951+
* carry the info this far so that we can drop the constraint below.
7952+
*/
7953+
if (!conislocal)
7954+
continue;
79437955

7944-
get_constraint_relation_oids(oldId, &relid, &confrelid);
79457956
ATPostAlterTypeParse(oldId, relid, confrelid,
79467957
(char *) lfirst(def_item),
79477958
wqueue, lockmode, tab->rewrite);

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

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,7 @@ static Node *processIndirection(Node *node, deparse_context *context,
418418
static void printSubscripts(ArrayRef *aref, deparse_context *context);
419419
static char *get_relation_name(Oid relid);
420420
static char *generate_relation_name(Oid relid, List *namespaces);
421+
static char *generate_qualified_relation_name(Oid relid);
421422
static char *generate_function_name(Oid funcid, int nargs,
422423
List *argnames, Oid *argtypes,
423424
bool was_variadic, bool *use_variadic_p);
@@ -1296,7 +1297,9 @@ pg_get_constraintdef_ext(PG_FUNCTION_ARGS)
12961297
prettyFlags)));
12971298
}
12981299

1299-
/* Internal version that returns a palloc'd C string; no pretty-printing */
1300+
/*
1301+
* Internal version that returns a full ALTER TABLE ... ADD CONSTRAINT command
1302+
*/
13001303
char *
13011304
pg_get_constraintdef_string(Oid constraintId)
13021305
{
@@ -1318,10 +1321,16 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
13181321

13191322
initStringInfo(&buf);
13201323

1321-
if (fullCommand && OidIsValid(conForm->conrelid))
1324+
if (fullCommand)
13221325
{
1323-
appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
1324-
generate_relation_name(conForm->conrelid, NIL),
1326+
/*
1327+
* Currently, callers want ALTER TABLE (without ONLY) for CHECK
1328+
* constraints, and other types of constraints don't inherit anyway so
1329+
* it doesn't matter whether we say ONLY or not. Someday we might
1330+
* need to let callers specify whether to put ONLY in the command.
1331+
*/
1332+
appendStringInfo(&buf, "ALTER TABLE %s ADD CONSTRAINT %s ",
1333+
generate_qualified_relation_name(conForm->conrelid),
13251334
quote_identifier(NameStr(conForm->conname)));
13261335
}
13271336

@@ -1844,28 +1853,9 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
18441853

18451854
if (OidIsValid(sequenceId))
18461855
{
1847-
HeapTuple classtup;
1848-
Form_pg_class classtuple;
1849-
char *nspname;
18501856
char *result;
18511857

1852-
/* Get the sequence's pg_class entry */
1853-
classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(sequenceId));
1854-
if (!HeapTupleIsValid(classtup))
1855-
elog(ERROR, "cache lookup failed for relation %u", sequenceId);
1856-
classtuple = (Form_pg_class) GETSTRUCT(classtup);
1857-
1858-
/* Get the namespace */
1859-
nspname = get_namespace_name(classtuple->relnamespace);
1860-
if (!nspname)
1861-
elog(ERROR, "cache lookup failed for namespace %u",
1862-
classtuple->relnamespace);
1863-
1864-
/* And construct the result string */
1865-
result = quote_qualified_identifier(nspname,
1866-
NameStr(classtuple->relname));
1867-
1868-
ReleaseSysCache(classtup);
1858+
result = generate_qualified_relation_name(sequenceId);
18691859

18701860
PG_RETURN_TEXT_P(string_to_text(result));
18711861
}
@@ -8862,6 +8852,39 @@ generate_relation_name(Oid relid, List *namespaces)
88628852
return result;
88638853
}
88648854

8855+
/*
8856+
* generate_qualified_relation_name
8857+
* Compute the name to display for a relation specified by OID
8858+
*
8859+
* As above, but unconditionally schema-qualify the name.
8860+
*/
8861+
static char *
8862+
generate_qualified_relation_name(Oid relid)
8863+
{
8864+
HeapTuple tp;
8865+
Form_pg_class reltup;
8866+
char *relname;
8867+
char *nspname;
8868+
char *result;
8869+
8870+
tp = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
8871+
if (!HeapTupleIsValid(tp))
8872+
elog(ERROR, "cache lookup failed for relation %u", relid);
8873+
reltup = (Form_pg_class) GETSTRUCT(tp);
8874+
relname = NameStr(reltup->relname);
8875+
8876+
nspname = get_namespace_name(reltup->relnamespace);
8877+
if (!nspname)
8878+
elog(ERROR, "cache lookup failed for namespace %u",
8879+
reltup->relnamespace);
8880+
8881+
result = quote_qualified_identifier(nspname, relname);
8882+
8883+
ReleaseSysCache(tp);
8884+
8885+
return result;
8886+
}
8887+
88658888
/*
88668889
* generate_function_name
88678890
* Compute the name to display for a function specified by OID,

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

Lines changed: 128 additions & 2 deletions
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

Lines changed: 30 additions & 2 deletions
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)
0