Skip to content

Commit

Permalink
Reject attempts to alter composite types used in indexes.
Browse files Browse the repository at this point in the history
find_composite_type_dependencies() ignored indexes, which is a poor
decision because an expression index could have a stored column of
a composite (or other container) type even when the underlying table
does not.  Teach it to detect such cases and error out.  We have to
work a bit harder than for other relations because the pg_depend entry
won't identify the specific index column of concern, but it's not much
new code.

This does not address bug #17872's original complaint that dropping
a column in such a type might lead to violations of the uniqueness
property that a unique index is supposed to ensure.  That seems of
much less concern to me because it won't lead to crashes.

Per bug #17872 from Alexander Lakhin.  Back-patch to all supported
branches.

Discussion: https://postgr.es/m/17872-d0fbb799dc3fd85d@postgresql.org
  • Loading branch information
tglsfdc committed Mar 27, 2023
1 parent c87aff0 commit a3c9d35
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 10 deletions.
56 changes: 48 additions & 8 deletions src/backend/commands/tablecmds.c
Expand Up @@ -6508,6 +6508,7 @@ find_composite_type_dependencies(Oid typeOid, Relation origRelation,
{
Form_pg_depend pg_depend = (Form_pg_depend) GETSTRUCT(depTup);
Relation rel;
TupleDesc tupleDesc;
Form_pg_attribute att;

/* Check for directly dependent types */
Expand All @@ -6524,18 +6525,57 @@ find_composite_type_dependencies(Oid typeOid, Relation origRelation,
continue;
}

/* Else, ignore dependees that aren't user columns of relations */
/* (we assume system columns are never of interesting types) */
if (pg_depend->classid != RelationRelationId ||
pg_depend->objsubid <= 0)
/* Else, ignore dependees that aren't relations */
if (pg_depend->classid != RelationRelationId)
continue;

rel = relation_open(pg_depend->objid, AccessShareLock);
att = TupleDescAttr(rel->rd_att, pg_depend->objsubid - 1);
tupleDesc = RelationGetDescr(rel);

if (rel->rd_rel->relkind == RELKIND_RELATION ||
rel->rd_rel->relkind == RELKIND_MATVIEW ||
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
/*
* If objsubid identifies a specific column, refer to that in error
* messages. Otherwise, search to see if there's a user column of the
* type. (We assume system columns are never of interesting types.)
* The search is needed because an index containing an expression
* column of the target type will just be recorded as a whole-relation
* dependency. If we do not find a column of the type, the dependency
* must indicate that the type is transiently referenced in an index
* expression but not stored on disk, which we assume is OK, just as
* we do for references in views. (It could also be that the target
* type is embedded in some container type that is stored in an index
* column, but the previous recursion should catch such cases.)
*/
if (pg_depend->objsubid > 0 && pg_depend->objsubid <= tupleDesc->natts)
att = TupleDescAttr(tupleDesc, pg_depend->objsubid - 1);
else
{
att = NULL;
for (int attno = 1; attno <= tupleDesc->natts; attno++)
{
att = TupleDescAttr(tupleDesc, attno - 1);
if (att->atttypid == typeOid && !att->attisdropped)
break;
att = NULL;
}
if (att == NULL)
{
/* No such column, so assume OK */
relation_close(rel, AccessShareLock);
continue;
}
}

/*
* We definitely should reject if the relation has storage. If it's
* partitioned, then perhaps we don't have to reject: if there are
* partitions then we'll fail when we find one, else there is no
* stored data to worry about. However, it's possible that the type
* change would affect conclusions about whether the type is sortable
* or hashable and thus (if it's a partitioning column) break the
* partitioning rule. For now, reject for partitioned rels too.
*/
if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind) ||
RELKIND_HAS_PARTITIONS(rel->rd_rel->relkind))
{
if (origTypeName)
ereport(ERROR,
Expand Down
10 changes: 9 additions & 1 deletion src/test/regress/expected/alter_table.out
Expand Up @@ -3097,6 +3097,13 @@ CREATE TYPE test_type1 AS (a int, b text);
CREATE TABLE test_tbl1 (x int, y test_type1);
ALTER TYPE test_type1 ALTER ATTRIBUTE b TYPE varchar; -- fails
ERROR: cannot alter type "test_type1" because column "test_tbl1.y" uses it
DROP TABLE test_tbl1;
CREATE TABLE test_tbl1 (x int, y text);
CREATE INDEX test_tbl1_idx ON test_tbl1((row(x,y)::test_type1));
ALTER TYPE test_type1 ALTER ATTRIBUTE b TYPE varchar; -- fails
ERROR: cannot alter type "test_type1" because column "test_tbl1_idx.row" uses it
DROP TABLE test_tbl1;
DROP TYPE test_type1;
CREATE TYPE test_type2 AS (a int, b text);
CREATE TABLE test_tbl2 OF test_type2;
CREATE TABLE test_tbl2_subclass () INHERITS (test_tbl2);
Expand Down Expand Up @@ -3208,7 +3215,8 @@ Typed table of type: test_type2
c | text | | |
Inherits: test_tbl2

DROP TABLE test_tbl2_subclass;
DROP TABLE test_tbl2_subclass, test_tbl2;
DROP TYPE test_type2;
CREATE TYPE test_typex AS (a int, b text);
CREATE TABLE test_tblx (x int, y test_typex check ((y).a > 0));
ALTER TYPE test_typex DROP ATTRIBUTE a; -- fails
Expand Down
11 changes: 10 additions & 1 deletion src/test/regress/sql/alter_table.sql
Expand Up @@ -1983,6 +1983,14 @@ CREATE TYPE test_type1 AS (a int, b text);
CREATE TABLE test_tbl1 (x int, y test_type1);
ALTER TYPE test_type1 ALTER ATTRIBUTE b TYPE varchar; -- fails

DROP TABLE test_tbl1;
CREATE TABLE test_tbl1 (x int, y text);
CREATE INDEX test_tbl1_idx ON test_tbl1((row(x,y)::test_type1));
ALTER TYPE test_type1 ALTER ATTRIBUTE b TYPE varchar; -- fails

DROP TABLE test_tbl1;
DROP TYPE test_type1;

CREATE TYPE test_type2 AS (a int, b text);
CREATE TABLE test_tbl2 OF test_type2;
CREATE TABLE test_tbl2_subclass () INHERITS (test_tbl2);
Expand Down Expand Up @@ -2010,7 +2018,8 @@ ALTER TYPE test_type2 RENAME ATTRIBUTE a TO aa CASCADE;
\d test_tbl2
\d test_tbl2_subclass

DROP TABLE test_tbl2_subclass;
DROP TABLE test_tbl2_subclass, test_tbl2;
DROP TYPE test_type2;

CREATE TYPE test_typex AS (a int, b text);
CREATE TABLE test_tblx (x int, y test_typex check ((y).a > 0));
Expand Down

0 comments on commit a3c9d35

Please sign in to comment.