Re: RangeVarGetRelid() - Mailing list pgsql-hackers

On Mon, Dec 19, 2011 at 11:52:54PM -0500, Robert Haas wrote:
> After staring at this for quite a while longer, it seemed to me that
> the logic for renaming a relation was similar enough to the logic for
> changing a schema that the two calbacks could reasonably be combined
> using a bit of conditional logic; and that, further, the same callback
> could be used, with a small amount of additional modification, for
> ALTER TABLE.  Here's a patch to do that.

Nice.

> I also notice that cluster() - which doesn't have a callback - has
> exactly the same needs as ReindexRelation() - which does.  So that
> case can certainly share code; though I'm not quite sure what to call
> the shared callback, or which file to put it in.
> RangeVarCallbackForStorageRewrite?

I'd put it in tablecmds.c and name it RangeVarCallbackOwnsTable.


A few things on the patch:

> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c

> @@ -2560,90 +2500,26 @@ CheckTableNotInUse(Relation rel, const char *stmt)
>   * Thanks to the magic of MVCC, an error anywhere along the way rolls back
>   * the whole operation; we don't have to do anything special to clean up.
>   *
> - * We lock the table as the first action, with an appropriate lock level
> + * The caller must lock the relation, with an appropriate lock level 
>   * for the subcommands requested. Any subcommand that needs to rewrite
>   * tuples in the table forces the whole command to be executed with
> - * AccessExclusiveLock. If all subcommands do not require rewrite table
> - * then we may be able to use lower lock levels. We pass the lock level down
> + * AccessExclusiveLock (actually, that is currently required always, but
> + * we hope to relax it at some point).  We pass the lock level down
>   * so that we can apply it recursively to inherited tables. Note that the
> - * lock level we want as we recurse may well be higher than required for
> + * lock level we want as we recurse might well be higher than required for
>   * that specific subcommand. So we pass down the overall lock requirement,
>   * rather than reassess it at lower levels.
>   */
>  void
> -AlterTable(AlterTableStmt *stmt)
> +AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt)
>  {
>      Relation    rel;
> -    LOCKMODE    lockmode = AlterTableGetLockLevel(stmt->cmds);
>  
> -    /*
> -     * Acquire same level of lock as already acquired during parsing.
> -     */
> -    rel = relation_openrv(stmt->relation, lockmode);
> +    /* Caller is required to provide an adequate lock. */
> +    rel = relation_open(relid, NoLock);
>  
>      CheckTableNotInUse(rel, "ALTER TABLE");
>  
> -    /* Check relation type against type specified in the ALTER command */
> -    switch (stmt->relkind)
> -    {
> -        case OBJECT_TABLE:
> -
> -            /*
> -             * For mostly-historical reasons, we allow ALTER TABLE to apply to
> -             * almost all relation types.
> -             */
> -            if (rel->rd_rel->relkind == RELKIND_COMPOSITE_TYPE
> -                || rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
> -                ereport(ERROR,
> -                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> -                         errmsg("\"%s\" is not a table",
> -                                RelationGetRelationName(rel))));

RangeVarCallbackForAlterRelation() does not preserve ALTER TABLE's refusal to
operate on foreign tables.

> -            break;
> -
> -        case OBJECT_INDEX:
> -            if (rel->rd_rel->relkind != RELKIND_INDEX)
> -                ereport(ERROR,
> -                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> -                         errmsg("\"%s\" is not an index",
> -                                RelationGetRelationName(rel))));
> -            break;
> -
> -        case OBJECT_SEQUENCE:
> -            if (rel->rd_rel->relkind != RELKIND_SEQUENCE)
> -                ereport(ERROR,
> -                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> -                         errmsg("\"%s\" is not a sequence",
> -                                RelationGetRelationName(rel))));
> -            break;
> -
> -        case OBJECT_TYPE:
> -            if (rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
> -                ereport(ERROR,
> -                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> -                         errmsg("\"%s\" is not a composite type",
> -                                RelationGetRelationName(rel))));
> -            break;
> -
> -        case OBJECT_VIEW:
> -            if (rel->rd_rel->relkind != RELKIND_VIEW)
> -                ereport(ERROR,
> -                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> -                         errmsg("\"%s\" is not a view",
> -                                RelationGetRelationName(rel))));
> -            break;
> -
> -        case OBJECT_FOREIGN_TABLE:
> -            if (rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
> -                ereport(ERROR,
> -                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> -                         errmsg("\"%s\" is not a foreign table",
> -                                RelationGetRelationName(rel))));
> -            break;
> -
> -        default:
> -            elog(ERROR, "unrecognized object type: %d", (int) stmt->relkind);

RangeVarCallbackForAlterRelation() does not preserve the check for unexpected
object types.

> -    }
> -
>      ATController(rel, stmt->cmds, interpretInhOption(stmt->relation->inhOpt),
>                   lockmode);
>  }

> --- a/src/backend/tcop/utility.c
> +++ b/src/backend/tcop/utility.c
> @@ -699,12 +699,23 @@ standard_ProcessUtility(Node *parsetree,
>  
>          case T_AlterTableStmt:
>              {
> +                AlterTableStmt *atstmt = (AlterTableStmt *) parsetree;
> +                Oid            relid;
>                  List       *stmts;
>                  ListCell   *l;
> +                LOCKMODE    lockmode;
> +
> +                /*
> +                 * Figure out lock mode, and acquire lock.  This also does
> +                 * basic permissions checks, so that we won't wait for a lock
> +                 * on (for example) a relation on which we have no
> +                 * permissions.
> +                 */
> +                lockmode = AlterTableGetLockLevel(atstmt->cmds);
> +                relid = AlterTableLookupRelation(atstmt, lockmode);
>  
>                  /* Run parse analysis ... */
> -                stmts = transformAlterTableStmt((AlterTableStmt *) parsetree,
> -                                                queryString);
> +                stmts = transformAlterTableStmt(atstmt, queryString);

utility.c doesn't take locks for any other command; parse analysis usually
does that.  To preserve that modularity, you could add a "bool toplevel"
argument to transformAlterTableStmt().  Pass true here, false in
ATPostAlterTypeParse().  If true, use AlterTableLookupRelation() to get full
security checks.  Otherwise, just call relation_openrv() as now.  Would that
be an improvement?

>  
>                  /* ... and do it */
>                  foreach(l, stmts)

nm