From 33f5bf97009811d7f6b5408e37c6ad68110985b0 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 4 Aug 2005 01:09:29 +0000 Subject: [PATCH] ALTER TABLE OWNER must change the ownership of the table's rowtype too. This was not especially critical before, but it is now that we track ownership dependencies --- the dependency for the rowtype *must* shift to the new owner. Spotted by Bernd Helmle. Also fix a problem introduced by recent change to allow non-superusers to do ALTER OWNER in some cases: if the table had a toast table, ALTER OWNER failed *even for superusers*, because the test being applied would conclude that the new would-be owner had no create rights on pg_toast. A side-effect of the fix is to disallow changing the ownership of indexes or toast tables separately from their parent table, which seems a good idea on the whole. --- doc/src/sgml/ref/alter_table.sgml | 4 +- src/backend/commands/tablecmds.c | 70 ++++++++++++++++-------- src/backend/commands/typecmds.c | 44 ++++++++++++++- src/include/commands/typecmds.h | 3 +- src/test/regress/expected/dependency.out | 8 ++- src/test/regress/sql/dependency.sql | 5 +- 6 files changed, 102 insertions(+), 32 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 26dabbb79e..8e1d298424 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -1,5 +1,5 @@ @@ -235,7 +235,7 @@ where action is one of: OWNER - This form changes the owner of the table, index, sequence, or view to the + This form changes the owner of the table, sequence, or view to the specified user. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7d9a73917d..aaf9a2ce74 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.165 2005/08/01 04:03:55 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.166 2005/08/04 01:09:28 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -238,7 +238,7 @@ static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, const char *colName, TypeName *typename); static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab); static void ATPostAlterTypeParse(char *cmd, List **wqueue); -static void ATExecChangeOwner(Oid relationOid, Oid newOwnerId); +static void ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing); static void change_owner_recurse_to_sequences(Oid relationOid, Oid newOwnerId); static void ATExecClusterOn(Relation rel, const char *indexName); @@ -2141,7 +2141,8 @@ ATExecCmd(AlteredTableInfo *tab, Relation rel, AlterTableCmd *cmd) break; case AT_ChangeOwner: /* ALTER OWNER */ ATExecChangeOwner(RelationGetRelid(rel), - get_roleid_checked(cmd->name)); + get_roleid_checked(cmd->name), + false); break; case AT_ClusterOn: /* CLUSTER ON */ ATExecClusterOn(rel, cmd->name); @@ -5238,9 +5239,15 @@ ATPostAlterTypeParse(char *cmd, List **wqueue) /* * ALTER TABLE OWNER + * + * recursing is true if we are recursing from a table to its indexes or + * toast table. We don't allow the ownership of those things to be + * changed separately from the parent table. Also, we can skip permission + * checks (this is necessary not just an optimization, else we'd fail to + * handle toast tables properly). */ static void -ATExecChangeOwner(Oid relationOid, Oid newOwnerId) +ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing) { Relation target_rel; Relation class_rel; @@ -5267,16 +5274,19 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId) switch (tuple_class->relkind) { case RELKIND_RELATION: - case RELKIND_INDEX: case RELKIND_VIEW: case RELKIND_SEQUENCE: - case RELKIND_TOASTVALUE: /* ok to change owner */ break; + case RELKIND_INDEX: + case RELKIND_TOASTVALUE: + if (recursing) + break; + /* FALL THRU */ default: ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table, TOAST table, index, view, or sequence", + errmsg("\"%s\" is not a table, view, or sequence", NameStr(tuple_class->relname)))); } @@ -5293,23 +5303,28 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId) Datum aclDatum; bool isNull; HeapTuple newtuple; - Oid namespaceOid = tuple_class->relnamespace; - AclResult aclresult; - /* Otherwise, must be owner of the existing object */ - if (!pg_class_ownercheck(relationOid,GetUserId())) - aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, - RelationGetRelationName(target_rel)); + /* skip permission checks when recursing to index or toast table */ + if (!recursing) + { + Oid namespaceOid = tuple_class->relnamespace; + AclResult aclresult; - /* Must be able to become new owner */ - check_is_member_of_role(GetUserId(), newOwnerId); + /* Otherwise, must be owner of the existing object */ + if (!pg_class_ownercheck(relationOid,GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, + RelationGetRelationName(target_rel)); - /* New owner must have CREATE privilege on namespace */ - aclresult = pg_namespace_aclcheck(namespaceOid, newOwnerId, - ACL_CREATE); - if (aclresult != ACLCHECK_OK) - aclcheck_error(aclresult, ACL_KIND_NAMESPACE, - get_namespace_name(namespaceOid)); + /* Must be able to become new owner */ + check_is_member_of_role(GetUserId(), newOwnerId); + + /* New owner must have CREATE privilege on namespace */ + aclresult = pg_namespace_aclcheck(namespaceOid, newOwnerId, + ACL_CREATE); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, ACL_KIND_NAMESPACE, + get_namespace_name(namespaceOid)); + } memset(repl_null, ' ', sizeof(repl_null)); memset(repl_repl, ' ', sizeof(repl_repl)); @@ -5342,6 +5357,12 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId) /* Update owner dependency reference */ changeDependencyOnOwner(RelationRelationId, relationOid, newOwnerId); + /* + * Also change the ownership of the table's rowtype, if it has one + */ + if (tuple_class->relkind != RELKIND_INDEX) + AlterTypeOwnerInternal(tuple_class->reltype, newOwnerId); + /* * If we are operating on a table, also change the ownership of * any indexes and sequences that belong to the table, as well as @@ -5358,7 +5379,7 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId) /* For each index, recursively change its ownership */ foreach(i, index_oid_list) - ATExecChangeOwner(lfirst_oid(i), newOwnerId); + ATExecChangeOwner(lfirst_oid(i), newOwnerId, true); list_free(index_oid_list); } @@ -5367,7 +5388,8 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId) { /* If it has a toast table, recurse to change its ownership */ if (tuple_class->reltoastrelid != InvalidOid) - ATExecChangeOwner(tuple_class->reltoastrelid, newOwnerId); + ATExecChangeOwner(tuple_class->reltoastrelid, newOwnerId, + true); /* If it has dependent sequences, recurse to change them too */ change_owner_recurse_to_sequences(relationOid, newOwnerId); @@ -5437,7 +5459,7 @@ change_owner_recurse_to_sequences(Oid relationOid, Oid newOwnerId) } /* We don't need to close the sequence while we alter it. */ - ATExecChangeOwner(depForm->objid, newOwnerId); + ATExecChangeOwner(depForm->objid, newOwnerId, false); /* Now we can close it. Keep the lock till end of transaction. */ relation_close(seqRel, NoLock); diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index 80d394b293..31e43cd428 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/typecmds.c,v 1.77 2005/08/01 04:03:55 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/commands/typecmds.c,v 1.78 2005/08/04 01:09:28 tgl Exp $ * * DESCRIPTION * The "DefineFoo" routines take the parse tree and pick out the @@ -2057,7 +2057,8 @@ AlterTypeOwner(List *names, Oid newOwnerId) * free-standing composite type, and not a table's underlying type. We * want people to use ALTER TABLE not ALTER TYPE for that case. */ - if (typTup->typtype == 'c' && get_rel_relkind(typTup->typrelid) != 'c') + if (typTup->typtype == 'c' && + get_rel_relkind(typTup->typrelid) != RELKIND_COMPOSITE_TYPE) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("\"%s\" is a table's row type", @@ -2102,6 +2103,45 @@ AlterTypeOwner(List *names, Oid newOwnerId) heap_close(rel, RowExclusiveLock); } +/* + * AlterTypeOwnerInternal - change type owner unconditionally + * + * This is currently only used to propagate ALTER TABLE OWNER to the + * table's rowtype. It assumes the caller has done all needed checks. + */ +void +AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId) +{ + Relation rel; + HeapTuple tup; + Form_pg_type typTup; + + rel = heap_open(TypeRelationId, RowExclusiveLock); + + tup = SearchSysCacheCopy(TYPEOID, + ObjectIdGetDatum(typeOid), + 0, 0, 0); + if (!HeapTupleIsValid(tup)) + elog(ERROR, "cache lookup failed for type %u", typeOid); + typTup = (Form_pg_type) GETSTRUCT(tup); + + /* + * Modify the owner --- okay to scribble on typTup because it's a + * copy + */ + typTup->typowner = newOwnerId; + + simple_heap_update(rel, &tup->t_self, tup); + + CatalogUpdateIndexes(rel, tup); + + /* Update owner dependency reference */ + changeDependencyOnOwner(TypeRelationId, typeOid, newOwnerId); + + /* Clean up */ + heap_close(rel, RowExclusiveLock); +} + /* * Execute ALTER TYPE SET SCHEMA */ diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h index a070a27a29..d53cf672a6 100644 --- a/src/include/commands/typecmds.h +++ b/src/include/commands/typecmds.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/commands/typecmds.h,v 1.12 2005/08/01 04:03:58 tgl Exp $ + * $PostgreSQL: pgsql/src/include/commands/typecmds.h,v 1.13 2005/08/04 01:09:29 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -35,6 +35,7 @@ extern void AlterDomainDropConstraint(List *names, const char *constrName, extern List *GetDomainConstraints(Oid typeOid); extern void AlterTypeOwner(List *names, Oid newOwnerId); +extern void AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId); extern void AlterTypeNamespace(List *names, const char *newschema); extern void AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid, bool errorOnTableType); diff --git a/src/test/regress/expected/dependency.out b/src/test/regress/expected/dependency.out index 4ee3e8b6a8..2c31e581bf 100644 --- a/src/test/regress/expected/dependency.out +++ b/src/test/regress/expected/dependency.out @@ -5,7 +5,9 @@ CREATE USER regression_user; CREATE USER regression_user2; CREATE USER regression_user3; CREATE GROUP regression_group; -CREATE TABLE deptest (); +CREATE TABLE deptest (f1 serial primary key, f2 text); +NOTICE: CREATE TABLE will create implicit sequence "deptest_f1_seq" for serial column "deptest.f1" +NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "deptest_pkey" for table "deptest" GRANT SELECT ON TABLE deptest TO GROUP regression_group; GRANT ALL ON TABLE deptest TO regression_user, regression_user2; -- can't drop neither because they have privileges somewhere @@ -30,10 +32,12 @@ DROP USER regression_user; REVOKE ALL ON deptest FROM regression_user2; DROP USER regression_user2; -- can't drop the owner of an object +-- the error message detail here would include a pg_toast_nnn name that +-- is not constant, so suppress it +\set VERBOSITY terse ALTER TABLE deptest OWNER TO regression_user3; DROP USER regression_user3; ERROR: role "regression_user3" cannot be dropped because some objects depend on it -DETAIL: owner of table deptest -- if we drop the object, we can drop the user too DROP TABLE deptest; DROP USER regression_user3; diff --git a/src/test/regress/sql/dependency.sql b/src/test/regress/sql/dependency.sql index 6d52b62dee..3e4a232ea7 100644 --- a/src/test/regress/sql/dependency.sql +++ b/src/test/regress/sql/dependency.sql @@ -7,7 +7,7 @@ CREATE USER regression_user2; CREATE USER regression_user3; CREATE GROUP regression_group; -CREATE TABLE deptest (); +CREATE TABLE deptest (f1 serial primary key, f2 text); GRANT SELECT ON TABLE deptest TO GROUP regression_group; GRANT ALL ON TABLE deptest TO regression_user, regression_user2; @@ -33,6 +33,9 @@ REVOKE ALL ON deptest FROM regression_user2; DROP USER regression_user2; -- can't drop the owner of an object +-- the error message detail here would include a pg_toast_nnn name that +-- is not constant, so suppress it +\set VERBOSITY terse ALTER TABLE deptest OWNER TO regression_user3; DROP USER regression_user3;