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