Re: RangeVarGetRelid()

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