diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 567eb7fd6eb..5d25ce9ec88 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -431,6 +431,7 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, CheckAttributeType(NameStr(tupdesc->attrs[i]->attname), tupdesc->attrs[i]->atttypid, tupdesc->attrs[i]->attcollation, + NIL, /* assume we're creating a new rowtype */ allow_system_table_mods); } } @@ -439,15 +440,25 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, * CheckAttributeType * * Verify that the proposed datatype of an attribute is legal. - * This is needed because there are types (and pseudo-types) + * This is needed mainly because there are types (and pseudo-types) * in the catalogs that we do not support as elements of real tuples. + * We also check some other properties required of a table column. + * + * If the attribute is being proposed for addition to an existing table or + * composite type, pass a one-element list of the rowtype OID as + * containing_rowtypes. When checking a to-be-created rowtype, it's + * sufficient to pass NIL, because there could not be any recursive reference + * to a not-yet-existing rowtype. * -------------------------------- */ void -CheckAttributeType(const char *attname, Oid atttypid, Oid attcollation, +CheckAttributeType(const char *attname, + Oid atttypid, Oid attcollation, + List *containing_rowtypes, bool allow_system_table_mods) { char att_typtype = get_typtype(atttypid); + Oid att_typelem; if (atttypid == UNKNOWNOID) { @@ -485,6 +496,20 @@ CheckAttributeType(const char *attname, Oid atttypid, Oid attcollation, TupleDesc tupdesc; int i; + /* + * Check for self-containment. Eventually we might be able to allow + * this (just return without complaint, if so) but it's not clear how + * many other places would require anti-recursion defenses before it + * would be safe to allow tables to contain their own rowtype. + */ + if (list_member_oid(containing_rowtypes, atttypid)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("composite type %s cannot be made a member of itself", + format_type_be(atttypid)))); + + containing_rowtypes = lcons_oid(atttypid, containing_rowtypes); + relation = relation_open(get_typ_typrelid(atttypid), AccessShareLock); tupdesc = RelationGetDescr(relation); @@ -495,19 +520,31 @@ CheckAttributeType(const char *attname, Oid atttypid, Oid attcollation, if (attr->attisdropped) continue; - CheckAttributeType(NameStr(attr->attname), attr->atttypid, attr->attcollation, + CheckAttributeType(NameStr(attr->attname), + attr->atttypid, attr->attcollation, + containing_rowtypes, allow_system_table_mods); } relation_close(relation, AccessShareLock); + + containing_rowtypes = list_delete_first(containing_rowtypes); + } + else if (OidIsValid((att_typelem = get_element_type(atttypid)))) + { + /* + * Must recurse into array types, too, in case they are composite. + */ + CheckAttributeType(attname, att_typelem, attcollation, + containing_rowtypes, + allow_system_table_mods); } /* * This might not be strictly invalid per SQL standard, but it is - * pretty useless, and it cannot be dumped, so we must disallow - * it. + * pretty useless, and it cannot be dumped, so we must disallow it. */ - if (type_is_collatable(atttypid) && !OidIsValid(attcollation)) + if (!OidIsValid(attcollation) && type_is_collatable(atttypid)) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("no collation was derived for column \"%s\" with collatable type %s", diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 7a2629ecd19..679255a199b 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -403,7 +403,9 @@ ConstructTupleDescriptor(Relation heapRelation, * whether a table column is of a safe type (which is why we * needn't check for the non-expression case). */ - CheckAttributeType(NameStr(to->attname), to->atttypid, to->attcollation, false); + CheckAttributeType(NameStr(to->attname), + to->atttypid, to->attcollation, + NIL, false); } /* diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d7573b8ef50..737ab1a7bc8 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -4221,7 +4221,9 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel, collOid = GetColumnDefCollation(NULL, colDef, typeOid); /* make sure datatype is legal for a column */ - CheckAttributeType(colDef->colname, typeOid, collOid, false); + CheckAttributeType(colDef->colname, typeOid, collOid, + list_make1_oid(rel->rd_rel->reltype), + false); /* construct new attribute's pg_attribute entry */ attribute.attrelid = myrelid; @@ -6536,7 +6538,9 @@ ATPrepAlterColumnType(List **wqueue, targetcollid = GetColumnDefCollation(NULL, def, targettype); /* make sure datatype is legal for a column */ - CheckAttributeType(colName, targettype, targetcollid, false); + CheckAttributeType(colName, targettype, targetcollid, + list_make1_oid(rel->rd_rel->reltype), + false); if (tab->relkind == RELKIND_RELATION) { diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h index 5f9a864be5f..463aff0358f 100644 --- a/src/include/catalog/heap.h +++ b/src/include/catalog/heap.h @@ -117,7 +117,9 @@ extern Form_pg_attribute SystemAttributeByName(const char *attname, extern void CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, bool allow_system_table_mods); -extern void CheckAttributeType(const char *attname, Oid atttypid, Oid attcollation, +extern void CheckAttributeType(const char *attname, + Oid atttypid, Oid attcollation, + List *containing_rowtypes, bool allow_system_table_mods); #endif /* HEAP_H */ diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index d5a59d5d1d4..578f94bc64c 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1493,6 +1493,18 @@ create table tab1 (a int, b text); create table tab2 (x int, y tab1); alter table tab1 alter column b type varchar; -- fails ERROR: cannot alter table "tab1" because column "tab2"."y" uses its rowtype +-- disallow recursive containment of row types +create temp table recur1 (f1 int); +alter table recur1 add column f2 recur1; -- fails +ERROR: composite type recur1 cannot be made a member of itself +alter table recur1 add column f2 recur1[]; -- fails +ERROR: composite type recur1 cannot be made a member of itself +create temp table recur2 (f1 int, f2 recur1); +alter table recur1 add column f2 recur2; -- fails +ERROR: composite type recur1 cannot be made a member of itself +alter table recur1 add column f2 int; +alter table recur1 alter column f2 type recur2; -- fails +ERROR: composite type recur1 cannot be made a member of itself -- -- lock levels -- diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 6531a9f162d..54dbb8eaf9d 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1112,6 +1112,15 @@ create table tab1 (a int, b text); create table tab2 (x int, y tab1); alter table tab1 alter column b type varchar; -- fails +-- disallow recursive containment of row types +create temp table recur1 (f1 int); +alter table recur1 add column f2 recur1; -- fails +alter table recur1 add column f2 recur1[]; -- fails +create temp table recur2 (f1 int, f2 recur1); +alter table recur1 add column f2 recur2; -- fails +alter table recur1 add column f2 int; +alter table recur1 alter column f2 type recur2; -- fails + -- -- lock levels --