Re: ALTER TYPE RENAME - Mailing list pgsql-patches

This has been applied by Tom.

---------------------------------------------------------------------------

Petr Jelinek wrote:
> Tom Lane wrote:
> > Hm, I'm not entirely sure if you got the point or not.  For either
> > relations or composite types, there is both a pg_class entry and a
> > pg_type entry, and their names *must* stay in sync.  We could allow
> > people to rename both entries using either ALTER TABLE or ALTER TYPE,
> > but the general consensus seems to be that ALTER TYPE should be used
> > for composite types and ALTER TABLE for tables/views/etc.  The fact
> > that there's a pg_class entry for a composite type is really an
> > implementation detail that would best not be exposed to users, so
> > enforcing the use of the appropriate command seems reasonable to me.
> >
> >             regards, tom lane
> >
> Yes, that's exactly what I meant (my language skills are not best).
>
> Anyway, I am sending second version of the patch. Changes are:
>  - renamed TypeRename function to RenameTypeInternal and changed its
> header comment
>  - throw error when using ALTER TYPE to rename rowtype
>  - split function renamerel to RenameRelation and RenameRelationInternal
> where RenameRelation does permission checks and stuff and also checks if
> it's not used for composite types and RenameRelationInternal  does the
> actual rename. And I also did a little cleanup in those functions
> (removed unused code and changed some hardcoded relkind types to globaly
> predefined constants)
>  - RenameType now calls RenameRelationInternal  for composite types
> (which calls RenameTypeInternal automatically)
>
> Any other comments ?
>
> --
> Regards
> Petr Jelinek (PJMODOS)
>

> Index: doc/src/sgml/ref/alter_type.sgml
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/alter_type.sgml,v
> retrieving revision 1.4
> diff -c -r1.4 alter_type.sgml
> *** doc/src/sgml/ref/alter_type.sgml    16 Sep 2006 00:30:16 -0000    1.4
> --- doc/src/sgml/ref/alter_type.sgml    29 Sep 2007 05:43:14 -0000
> ***************
> *** 26,31 ****
> --- 26,32 ----
>     <synopsis>
>   ALTER TYPE <replaceable class="PARAMETER">name</replaceable> OWNER TO <replaceable
class="PARAMETER">new_owner</replaceable> 
>   ALTER TYPE <replaceable class="PARAMETER">name</replaceable> SET SCHEMA <replaceable
class="PARAMETER">new_schema</replaceable>
> + ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RNAME TO <replaceable
class="PARAMETER">new_name</replaceable>
>     </synopsis>
>    </refsynopsisdiv>
>
> ***************
> *** 83,88 ****
> --- 84,98 ----
>         </listitem>
>        </varlistentry>
>
> +      <varlistentry>
> +       <term><replaceable class="PARAMETER">new_name</replaceable></term>
> +       <listitem>
> +        <para>
> +         The new name for the type.
> +        </para>
> +       </listitem>
> +      </varlistentry>
> +
>       </variablelist>
>      </para>
>     </refsect1>
> Index: src/backend/catalog/pg_type.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/catalog/pg_type.c,v
> retrieving revision 1.113
> diff -c -r1.113 pg_type.c
> *** src/backend/catalog/pg_type.c    12 May 2007 00:54:59 -0000    1.113
> --- src/backend/catalog/pg_type.c    30 Sep 2007 04:20:03 -0000
> ***************
> *** 552,566 ****
>   }
>
>   /*
> !  * TypeRename
>    *        This renames a type, as well as any associated array type.
>    *
> !  * Note: this isn't intended to be a user-exposed function; it doesn't check
> !  * permissions etc.  (Perhaps TypeRenameInternal would be a better name.)
> !  * Currently this is only used for renaming table rowtypes.
>    */
>   void
> ! TypeRename(Oid typeOid, const char *newTypeName, Oid typeNamespace)
>   {
>       Relation    pg_type_desc;
>       HeapTuple    tuple;
> --- 552,567 ----
>   }
>
>   /*
> !  * RenameTypeInternal
>    *        This renames a type, as well as any associated array type.
>    *
> !  * Caller must have already checked privileges.
> !  *
> !  * Currently this is used for renaming table rowtypes and for
> !  * ALTER TYPE RENAME TO command.
>    */
>   void
> ! RenameTypeInternal(Oid typeOid, const char *newTypeName, Oid typeNamespace)
>   {
>       Relation    pg_type_desc;
>       HeapTuple    tuple;
> ***************
> *** 606,612 ****
>       {
>           char   *arrname = makeArrayTypeName(newTypeName, typeNamespace);
>
> !         TypeRename(arrayOid, arrname, typeNamespace);
>           pfree(arrname);
>       }
>   }
> --- 607,613 ----
>       {
>           char   *arrname = makeArrayTypeName(newTypeName, typeNamespace);
>
> !         RenameTypeInternal(arrayOid, arrname, typeNamespace);
>           pfree(arrname);
>       }
>   }
> ***************
> *** 706,712 ****
>       newname = makeArrayTypeName(typeName, typeNamespace);
>
>       /* Apply the rename */
> !     TypeRename(typeOid, newname, typeNamespace);
>
>       /*
>        * We must bump the command counter so that any subsequent use of
> --- 707,713 ----
>       newname = makeArrayTypeName(typeName, typeNamespace);
>
>       /* Apply the rename */
> !     RenameTypeInternal(typeOid, newname, typeNamespace);
>
>       /*
>        * We must bump the command counter so that any subsequent use of
> Index: src/backend/commands/alter.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/commands/alter.c,v
> retrieving revision 1.25
> diff -c -r1.25 alter.c
> *** src/backend/commands/alter.c    21 Aug 2007 01:11:14 -0000    1.25
> --- src/backend/commands/alter.c    30 Sep 2007 03:53:05 -0000
> ***************
> *** 117,123 ****
>                                   aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
>                                               get_namespace_name(namespaceId));
>
> !                             renamerel(relid, stmt->newname, stmt->renameType);
>                               break;
>                           }
>                       case OBJECT_COLUMN:
> --- 117,123 ----
>                                   aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
>                                               get_namespace_name(namespaceId));
>
> !                             RenameRelation(relid, stmt->newname, stmt->renameType);
>                               break;
>                           }
>                       case OBJECT_COLUMN:
> ***************
> *** 154,159 ****
> --- 154,164 ----
>               RenameTSConfiguration(stmt->object, stmt->newname);
>               break;
>
> +         case OBJECT_TYPE:
> +         case OBJECT_DOMAIN:
> +             RenameType(stmt->object, stmt->newname);
> +             break;
> +
>           default:
>               elog(ERROR, "unrecognized rename stmt type: %d",
>                    (int) stmt->renameType);
> Index: src/backend/commands/tablecmds.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/commands/tablecmds.c,v
> retrieving revision 1.233
> diff -c -r1.233 tablecmds.c
> *** src/backend/commands/tablecmds.c    29 Sep 2007 17:18:58 -0000    1.233
> --- src/backend/commands/tablecmds.c    30 Sep 2007 03:55:31 -0000
> ***************
> *** 1612,1637 ****
>       relation_close(targetrelation, NoLock);        /* close rel but keep lock */
>   }
>
>   /*
> !  *        renamerel        - change the name of a relation
> !  *
> !  *        XXX - When renaming sequences, we don't bother to modify the
> !  *              sequence name that is stored within the sequence itself
> !  *              (this would cause problems with MVCC). In the future,
> !  *              the sequence name should probably be removed from the
> !  *              sequence, AFAIK there's no need for it to be there.
>    */
>   void
> ! renamerel(Oid myrelid, const char *newrelname, ObjectType reltype)
>   {
>       Relation    targetrelation;
> -     Relation    relrelation;    /* for RELATION relation */
> -     HeapTuple    reltup;
> -     Form_pg_class relform;
>       Oid            namespaceId;
> -     char       *oldrelname;
>       char        relkind;
> -     bool        relhastriggers;
>
>       /*
>        * Grab an exclusive lock on the target table, index, sequence or
> --- 1612,1627 ----
>       relation_close(targetrelation, NoLock);        /* close rel but keep lock */
>   }
>
> +
>   /*
> !  * Execute ALTER TABLE/VIEW/SEQUENCE RENAME
>    */
>   void
> ! RenameRelation(Oid myrelid, const char *newrelname, ObjectType reltype)
>   {
>       Relation    targetrelation;
>       Oid            namespaceId;
>       char        relkind;
>
>       /*
>        * Grab an exclusive lock on the target table, index, sequence or
> ***************
> *** 1639,1645 ****
>        */
>       targetrelation = relation_open(myrelid, AccessExclusiveLock);
>
> -     oldrelname = pstrdup(RelationGetRelationName(targetrelation));
>       namespaceId = RelationGetNamespace(targetrelation);
>
>       if (!allowSystemTableMods && IsSystemRelation(targetrelation))
> --- 1629,1634 ----
> ***************
> *** 1654,1672 ****
>        * view.
>        */
>       relkind = targetrelation->rd_rel->relkind;
> !     if (reltype == OBJECT_SEQUENCE && relkind != 'S')
>           ereport(ERROR,
>                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                    errmsg("\"%s\" is not a sequence",
>                           RelationGetRelationName(targetrelation))));
>
> !     if (reltype == OBJECT_VIEW && relkind != 'v')
>           ereport(ERROR,
>                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                    errmsg("\"%s\" is not a view",
>                           RelationGetRelationName(targetrelation))));
>
> !     relhastriggers = (targetrelation->rd_rel->reltriggers > 0);
>
>       /*
>        * Find relation's pg_class tuple, and make sure newrelname isn't in use.
> --- 1643,1702 ----
>        * view.
>        */
>       relkind = targetrelation->rd_rel->relkind;
> !     if (reltype == OBJECT_SEQUENCE && relkind != RELKIND_SEQUENCE)
>           ereport(ERROR,
>                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                    errmsg("\"%s\" is not a sequence",
>                           RelationGetRelationName(targetrelation))));
>
> !     if (reltype == OBJECT_VIEW && relkind != RELKIND_VIEW)
>           ereport(ERROR,
>                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                    errmsg("\"%s\" is not a view",
>                           RelationGetRelationName(targetrelation))));
>
> !     /*
> !      * Don't allow ALTER TABLE on composite types.
> !      * We want people to use ALTER TYPE for that.
> !      */
> !     if (relkind == RELKIND_COMPOSITE_TYPE)
> !         ereport(ERROR,
> !                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> !                  errmsg("\"%s\" is a composite type",
> !                         RelationGetRelationName(targetrelation)),
> !                  errhint("Use ALTER TYPE RENAME TO instead.")));
> !
> !     /* Do the work */
> !     RenameRelationInternal(myrelid, newrelname, namespaceId);
> !
> !     /*
> !      * Close rel, but keep exclusive lock!
> !      */
> !     relation_close(targetrelation, NoLock);
> ! }
> !
> ! /*
> !  *        RenameRelationInternal - change the name of a relation
> !  *
> !  *        XXX - When renaming sequences, we don't bother to modify the
> !  *              sequence name that is stored within the sequence itself
> !  *              (this would cause problems with MVCC). In the future,
> !  *              the sequence name should probably be removed from the
> !  *              sequence, AFAIK there's no need for it to be there.
> !  */
> ! void
> ! RenameRelationInternal(Oid myrelid, const char *newrelname, Oid namespaceId)
> ! {
> !     Relation    targetrelation;
> !     Relation    relrelation;    /* for RELATION relation */
> !     HeapTuple    reltup;
> !     Form_pg_class relform;
> !
> !     /*
> !      * Grab an exclusive lock on the target table, index, sequence or
> !      * view, which we will NOT release until end of transaction.
> !      */
> !     targetrelation = relation_open(myrelid, AccessExclusiveLock);
>
>       /*
>        * Find relation's pg_class tuple, and make sure newrelname isn't in use.
> ***************
> *** 1704,1710 ****
>        * Also rename the associated type, if any.
>        */
>       if (OidIsValid(targetrelation->rd_rel->reltype))
> !         TypeRename(targetrelation->rd_rel->reltype, newrelname, namespaceId);
>
>       /*
>        * Close rel, but keep exclusive lock!
> --- 1734,1740 ----
>        * Also rename the associated type, if any.
>        */
>       if (OidIsValid(targetrelation->rd_rel->reltype))
> !         RenameTypeInternal(targetrelation->rd_rel->reltype, newrelname, namespaceId);
>
>       /*
>        * Close rel, but keep exclusive lock!
> Index: src/backend/commands/typecmds.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/commands/typecmds.c,v
> retrieving revision 1.108
> diff -c -r1.108 typecmds.c
> *** src/backend/commands/typecmds.c    29 Sep 2007 17:18:58 -0000    1.108
> --- src/backend/commands/typecmds.c    30 Sep 2007 04:36:08 -0000
> ***************
> *** 2654,2656 ****
> --- 2654,2723 ----
>       if (OidIsValid(arrayOid))
>           AlterTypeNamespaceInternal(arrayOid, nspOid, true, true);
>   }
> +
> +
> + /*
> +  * Execute ALTER TYPE RENAME
> +  */
> + void
> + RenameType(List *names, const char *newTypeName)
> + {
> +     TypeName   *typename;
> +     Oid            typeOid;
> +     Relation    rel;
> +     HeapTuple    tup;
> +     Form_pg_type typTup;
> +
> +     /* Make a TypeName so we can use standard type lookup machinery */
> +     typename = makeTypeNameFromNameList(names);
> +     typeOid = typenameTypeId(NULL, typename);
> +
> +     /* Look up the type in the type table */
> +     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);
> +
> +     /* check permissions on type */
> +     if (!pg_type_ownercheck(typeOid, GetUserId()))
> +         aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TYPE,
> +                        format_type_be(typeOid));
> +
> +     /*
> +      * If it's a composite type, we need to check that it really is a
> +      * free-standing composite type, and not a table's rowtype. We
> +      * want people to use ALTER TABLE not ALTER TYPE for that case.
> +      */
> +     if (typTup->typtype == TYPTYPE_COMPOSITE &&
> +         get_rel_relkind(typTup->typrelid) != RELKIND_COMPOSITE_TYPE)
> +         ereport(ERROR,
> +                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                  errmsg("\"%s\" is a table's row type",
> +                         TypeNameToString(typename))));
> +
> +     /* don't allow direct alteration of array types, either */
> +     if (OidIsValid(typTup->typelem) &&
> +         get_array_type(typTup->typelem) == typeOid)
> +         ereport(ERROR,
> +                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                  errmsg("cannot alter array type %s",
> +                         format_type_be(typeOid)),
> +                  errhint("You can alter type %s, which will alter the array type as well.",
> +                          format_type_be(typTup->typelem))));
> +
> +     /*
> +      * If type is composite type we need to rename associated relation too.
> +      * RenameRelationInternal will call RenameTypeInternal automatically.
> +      */
> +     if (typTup->typtype == TYPTYPE_COMPOSITE)
> +         RenameRelationInternal(typTup->typrelid, newTypeName, typTup->typnamespace);
> +     else
> +         RenameTypeInternal(typeOid, newTypeName, typTup->typnamespace);
> +
> +     /* Clean up */
> +     heap_close(rel, RowExclusiveLock);
> + }
> Index: src/backend/parser/gram.y
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v
> retrieving revision 2.603
> diff -c -r2.603 gram.y
> *** src/backend/parser/gram.y    24 Sep 2007 01:29:28 -0000    2.603
> --- src/backend/parser/gram.y    29 Sep 2007 05:13:21 -0000
> ***************
> *** 4748,4753 ****
> --- 4748,4761 ----
>                       n->newname = $8;
>                       $$ = (Node *)n;
>                   }
> +             | ALTER TYPE_P any_name RENAME TO name
> +                 {
> +                     RenameStmt *n = makeNode(RenameStmt);
> +                     n->renameType = OBJECT_TYPE;
> +                     n->object = $3;
> +                     n->newname = $6;
> +                     $$ = (Node *)n;
> +                 }
>           ;
>
>   opt_column: COLUMN                                    { $$ = COLUMN; }
> Index: src/include/catalog/pg_type.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/include/catalog/pg_type.h,v
> retrieving revision 1.188
> diff -c -r1.188 pg_type.h
> *** src/include/catalog/pg_type.h    3 Sep 2007 02:30:45 -0000    1.188
> --- src/include/catalog/pg_type.h    29 Sep 2007 23:04:57 -0000
> ***************
> *** 678,684 ****
>                            Node *defaultExpr,
>                            bool rebuild);
>
> ! extern void TypeRename(Oid typeOid, const char *newTypeName,
>                          Oid typeNamespace);
>
>   extern char *makeArrayTypeName(const char *typeName, Oid typeNamespace);
> --- 678,684 ----
>                            Node *defaultExpr,
>                            bool rebuild);
>
> ! extern void RenameTypeInternal(Oid typeOid, const char *newTypeName,
>                          Oid typeNamespace);
>
>   extern char *makeArrayTypeName(const char *typeName, Oid typeNamespace);
> Index: src/include/commands/tablecmds.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/include/commands/tablecmds.h,v
> retrieving revision 1.34
> diff -c -r1.34 tablecmds.h
> *** src/include/commands/tablecmds.h    3 Jul 2007 01:30:37 -0000    1.34
> --- src/include/commands/tablecmds.h    30 Sep 2007 03:49:44 -0000
> ***************
> *** 42,51 ****
>             bool recurse,
>             bool recursing);
>
> ! extern void renamerel(Oid myrelid,
>             const char *newrelname,
>             ObjectType reltype);
>
>   extern void find_composite_type_dependencies(Oid typeOid,
>                                                const char *origTblName,
>                                                const char *origTypeName);
> --- 42,55 ----
>             bool recurse,
>             bool recursing);
>
> ! extern void RenameRelation(Oid myrelid,
>             const char *newrelname,
>             ObjectType reltype);
>
> + extern void RenameRelationInternal(Oid myrelid,
> +           const char *newrelname,
> +           Oid namespaceId);
> +
>   extern void find_composite_type_dependencies(Oid typeOid,
>                                                const char *origTblName,
>                                                const char *origTypeName);
> Index: src/include/commands/typecmds.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/include/commands/typecmds.h,v
> retrieving revision 1.19
> diff -c -r1.19 typecmds.h
> *** src/include/commands/typecmds.h    11 May 2007 17:57:14 -0000    1.19
> --- src/include/commands/typecmds.h    29 Sep 2007 05:11:57 -0000
> ***************
> *** 43,46 ****
> --- 43,48 ----
>                                          bool isImplicitArray,
>                                          bool errorOnTableType);
>
> + extern void RenameType(List *names, const char *newTypeName);
> +
>   #endif   /* TYPECMDS_H */

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
>                http://www.postgresql.org/docs/faq

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://postgres.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +