pgsql-at-rework-prep.1.patch

*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 254,259 **** static void validateForeignKeyConstraint(Constraint *fkconstraint,
--- 254,260 ----
  static void createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
  						 Oid constraintOid, Oid indexOid);
  static void ATController(Relation rel, List *cmds, bool recurse);
+ static void ATPermCmd(Relation rel, AlterTableCmd *cmd);
  static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  		  bool recurse, bool recursing);
  static void ATRewriteCatalogs(List **wqueue);
***************
*** 262,267 **** static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
--- 263,271 ----
  static void ATRewriteTables(List **wqueue);
  static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap);
  static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel);
+ static void ATCheckRelkind(Relation rel, bool viewOK, bool indexOK);
+ static void ATCheckOwnership(Relation rel);
+ static void ATCheckNoCatalog(Relation rel);
  static void ATSimplePermissions(Relation rel, bool allowView);
  static void ATSimplePermissionsRelationOrIndex(Relation rel);
  static void ATSimpleRecursion(List **wqueue, Relation rel,
***************
*** 284,291 **** static void ATPrepSetStatistics(Relation rel, const char *colName,
  					Node *newValue);
  static void ATExecSetStatistics(Relation rel, const char *colName,
  					Node *newValue);
- static void ATPrepSetDistinct(Relation rel, const char *colName,
- 					Node *newValue);
  static void ATExecSetDistinct(Relation rel, const char *colName,
  				 Node *newValue);
  static void ATExecSetStorage(Relation rel, const char *colName,
--- 288,293 ----
***************
*** 1929,1942 **** renameatt(Oid myrelid,
  	 *
  	 * normally, only the owner of a class can change its schema.
  	 */
! 	if (!pg_class_ownercheck(myrelid, GetUserId()))
! 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
! 					   RelationGetRelationName(targetrelation));
! 	if (!allowSystemTableMods && IsSystemRelation(targetrelation))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 				 errmsg("permission denied: \"%s\" is a system catalog",
! 						RelationGetRelationName(targetrelation))));
  
  	/*
  	 * if the 'recurse' flag is set then we are supposed to rename this
--- 1931,1938 ----
  	 *
  	 * normally, only the owner of a class can change its schema.
  	 */
! 	ATCheckOwnership(targetrelation);
! 	ATCheckNoCatalog(targetrelation);
  
  	/*
  	 * if the 'recurse' flag is set then we are supposed to rename this
***************
*** 2047,2052 **** RenameRelation(Oid myrelid, const char *newrelname, ObjectType reltype)
--- 2043,2049 ----
  {
  	Relation	targetrelation;
  	Oid			namespaceId;
+ 	AclResult	aclresult;
  	char		relkind;
  
  	/*
***************
*** 2075,2080 **** RenameRelation(Oid myrelid, const char *newrelname, ObjectType reltype)
--- 2072,2089 ----
  						RelationGetRelationName(targetrelation))));
  
  	/*
+ 	 * XXX - we assume CheckRelationOwnership() and pg_namespace_aclcheck()
+ 	 * should be moved to here from ExecRenameStmt().
+ 	 */
+ 	ATCheckOwnership(targetrelation);
+ 	aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_CREATE);
+ 	if (aclresult != ACLCHECK_OK)
+ 		aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
+ 					   get_namespace_name(namespaceId));
+ 
+ 	ATCheckNoCatalog(targetrelation);
+ 
+ 	/*
  	 * Don't allow ALTER TABLE on composite types. We want people to use ALTER
  	 * TYPE for that.
  	 */
***************
*** 2349,2354 **** ATController(Relation rel, List *cmds, bool recurse)
--- 2358,2462 ----
  }
  
  /*
+  * ATPermCmd
+  *
+  * It checks permission on the given Relation to be altered, if possible.
+  * Some of ALTER TABLE option needs to be checked during execution phase.
+  */
+ static void
+ ATPermCmd(Relation rel, AlterTableCmd *cmd)
+ {
+ 	switch (cmd->subtype)
+ 	{
+ 		case AT_AddColumn:		/* ADD COLUMN */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_AddColumnToView:	/* add column via CREATE OR REPLACE VIEW */
+ 			ATSimplePermissions(rel, true);
+ 			break;
+ 		case AT_ColumnDefault:	/* ALTER COLUMN DEFAULT */
+ 			ATSimplePermissions(rel, true);
+ 			break;
+ 		case AT_DropNotNull:	/* ALTER COLUMN DROP NOT NULL */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_SetNotNull:		/* ALTER COLUMN SET NOT NULL */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_SetStatistics:	/* ALTER COLUMN SET STATISTICS */
+ 		case AT_SetDistinct:	/* ALTER COLUMN SET STATISTICS DISTINCT */
+ 			ATCheckRelkind(rel, false, true);
+ 			ATCheckOwnership(rel);
+ 			break;
+ 		case AT_SetStorage:		/* ALTER COLUMN SET STORAGE */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_DropColumn:		/* DROP COLUMN */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_AddIndex:		/* ADD INDEX */
+ 			/* Permission checks are during execution phase */
+ 			break;
+ 		case AT_AddConstraint:	/* ADD CONSTRAINT */
+ 			/* Permission checks are during execution phase */
+ 			break;
+ 		case AT_DropConstraint:	/* DROP CONSTRAINT */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_AlterColumnType:		/* ALTER COLUMN TYPE */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_ChangeOwner:	/* ALTER OWNER */
+ 			/* Permission checks are during execution phase */
+ 			break;
+ 		case AT_ClusterOn:		/* CLUSTER ON */
+ 		case AT_DropCluster:	/* SET WITHOUT CLUSTER */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_AddOids:		/* SET WITH OIDS */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_DropOids:		/* SET WITHOUT OIDS */
+ 			ATSimplePermissions(rel, false);
+ 			/*
+ 			 * Note that ATPrepCmd() with AT_DropOids also calls ATPrepCmd() with
+ 			 * pseudo AT_DropColumn recursively, so it applies permission checks
+ 			 * twice.
+ 			 */
+ 			break;
+ 		case AT_SetTableSpace:	/* SET TABLESPACE */
+ 			/* Permission checks are during ATPrepSetTablespace() */
+ 			break;
+ 		case AT_SetRelOptions:	/* SET (...) */
+ 		case AT_ResetRelOptions:		/* RESET (...) */
+ 			ATSimplePermissionsRelationOrIndex(rel);
+ 			break;
+ 		case AT_EnableTrig:		/* ENABLE TRIGGER variants */
+ 		case AT_EnableAlwaysTrig:
+ 		case AT_EnableReplicaTrig:
+ 		case AT_EnableTrigAll:
+ 		case AT_EnableTrigUser:
+ 		case AT_DisableTrig:	/* DISABLE TRIGGER variants */
+ 		case AT_DisableTrigAll:
+ 		case AT_DisableTrigUser:
+ 		case AT_EnableRule:		/* ENABLE/DISABLE RULE variants */
+ 		case AT_EnableAlwaysRule:
+ 		case AT_EnableReplicaRule:
+ 		case AT_DisableRule:
+ 		case AT_DropInherit:	/* NO INHERIT */
+ 			ATSimplePermissions(rel, false);
+ 			break;
+ 		case AT_AddInherit:		/* INHERIT */
+ 			/* Permission checks are during preparation phase */
+ 			break;
+ 		default:				/* oops */
+ 			elog(ERROR, "unrecognized alter table type: %d",
+ 				 (int) cmd->subtype);
+ 			break;
+ 	}
+ }
+ 
+ /*
   * ATPrepCmd
   *
   * Traffic cop for ALTER TABLE Phase 1 operations, including simple
***************
*** 2382,2395 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  	switch (cmd->subtype)
  	{
  		case AT_AddColumn:		/* ADD COLUMN */
- 			ATSimplePermissions(rel, false);
  			/* Performs own recursion */
  			ATPrepAddColumn(wqueue, rel, recurse, cmd);
  			pass = AT_PASS_ADD_COL;
  			break;
  		case AT_AddColumnToView:		/* add column via CREATE OR REPLACE
  										 * VIEW */
- 			ATSimplePermissions(rel, true);
  			/* Performs own recursion */
  			ATPrepAddColumn(wqueue, rel, recurse, cmd);
  			pass = AT_PASS_ADD_COL;
--- 2490,2501 ----
***************
*** 2402,2444 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  			 * substitutes default values into INSERTs before it expands
  			 * rules.
  			 */
- 			ATSimplePermissions(rel, true);
  			ATSimpleRecursion(wqueue, rel, cmd, recurse);
  			/* No command-specific prep needed */
  			pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
  			break;
  		case AT_DropNotNull:	/* ALTER COLUMN DROP NOT NULL */
- 			ATSimplePermissions(rel, false);
  			ATSimpleRecursion(wqueue, rel, cmd, recurse);
  			/* No command-specific prep needed */
  			pass = AT_PASS_DROP;
  			break;
  		case AT_SetNotNull:		/* ALTER COLUMN SET NOT NULL */
- 			ATSimplePermissions(rel, false);
  			ATSimpleRecursion(wqueue, rel, cmd, recurse);
  			/* No command-specific prep needed */
  			pass = AT_PASS_ADD_CONSTR;
  			break;
  		case AT_SetStatistics:	/* ALTER COLUMN SET STATISTICS */
  			ATSimpleRecursion(wqueue, rel, cmd, recurse);
- 			/* Performs own permission checks */
- 			ATPrepSetStatistics(rel, cmd->name, cmd->def);
  			pass = AT_PASS_COL_ATTRS;
  			break;
  		case AT_SetDistinct:	/* ALTER COLUMN SET STATISTICS DISTINCT */
  			ATSimpleRecursion(wqueue, rel, cmd, recurse);
- 			/* Performs own permission checks */
- 			ATPrepSetDistinct(rel, cmd->name, cmd->def);
  			pass = AT_PASS_COL_ATTRS;
  			break;
  		case AT_SetStorage:		/* ALTER COLUMN SET STORAGE */
- 			ATSimplePermissions(rel, false);
  			ATSimpleRecursion(wqueue, rel, cmd, recurse);
  			/* No command-specific prep needed */
  			pass = AT_PASS_COL_ATTRS;
  			break;
  		case AT_DropColumn:		/* DROP COLUMN */
- 			ATSimplePermissions(rel, false);
  			/* Recursion occurs during execution phase */
  			/* No command-specific prep needed except saving recurse flag */
  			if (recurse)
--- 2508,2541 ----
***************
*** 2446,2458 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  			pass = AT_PASS_DROP;
  			break;
  		case AT_AddIndex:		/* ADD INDEX */
- 			ATSimplePermissions(rel, false);
  			/* This command never recurses */
  			/* No command-specific prep needed */
  			pass = AT_PASS_ADD_INDEX;
  			break;
  		case AT_AddConstraint:	/* ADD CONSTRAINT */
- 			ATSimplePermissions(rel, false);
  			/* Recursion occurs during execution phase */
  			/* No command-specific prep needed except saving recurse flag */
  			if (recurse)
--- 2543,2553 ----
***************
*** 2460,2466 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  			pass = AT_PASS_ADD_CONSTR;
  			break;
  		case AT_DropConstraint:	/* DROP CONSTRAINT */
- 			ATSimplePermissions(rel, false);
  			/* Recursion occurs during execution phase */
  			/* No command-specific prep needed except saving recurse flag */
  			if (recurse)
--- 2555,2560 ----
***************
*** 2468,2474 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  			pass = AT_PASS_DROP;
  			break;
  		case AT_AlterColumnType:		/* ALTER COLUMN TYPE */
- 			ATSimplePermissions(rel, false);
  			/* Performs own recursion */
  			ATPrepAlterColumnType(wqueue, tab, rel, recurse, recursing, cmd);
  			pass = AT_PASS_ALTER_TYPE;
--- 2562,2567 ----
***************
*** 2480,2499 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  			break;
  		case AT_ClusterOn:		/* CLUSTER ON */
  		case AT_DropCluster:	/* SET WITHOUT CLUSTER */
- 			ATSimplePermissions(rel, false);
  			/* These commands never recurse */
  			/* No command-specific prep needed */
  			pass = AT_PASS_MISC;
  			break;
  		case AT_AddOids:		/* SET WITH OIDS */
- 			ATSimplePermissions(rel, false);
  			/* Performs own recursion */
  			if (!rel->rd_rel->relhasoids || recursing)
  				ATPrepAddOids(wqueue, rel, recurse, cmd);
  			pass = AT_PASS_ADD_COL;
  			break;
  		case AT_DropOids:		/* SET WITHOUT OIDS */
- 			ATSimplePermissions(rel, false);
  			/* Performs own recursion */
  			if (rel->rd_rel->relhasoids)
  			{
--- 2573,2589 ----
***************
*** 2507,2524 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  			pass = AT_PASS_DROP;
  			break;
  		case AT_SetTableSpace:	/* SET TABLESPACE */
- 			ATSimplePermissionsRelationOrIndex(rel);
  			/* This command never recurses */
  			ATPrepSetTableSpace(tab, rel, cmd->name);
  			pass = AT_PASS_MISC;	/* doesn't actually matter */
  			break;
  		case AT_SetRelOptions:	/* SET (...) */
  		case AT_ResetRelOptions:		/* RESET (...) */
- 			ATSimplePermissionsRelationOrIndex(rel);
- 			/* This command never recurses */
- 			/* No command-specific prep needed */
- 			pass = AT_PASS_MISC;
- 			break;
  		case AT_EnableTrig:		/* ENABLE TRIGGER variants */
  		case AT_EnableAlwaysTrig:
  		case AT_EnableReplicaTrig:
--- 2597,2608 ----
***************
*** 2533,2539 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
  		case AT_DisableRule:
  		case AT_AddInherit:		/* INHERIT / NO INHERIT */
  		case AT_DropInherit:
- 			ATSimplePermissions(rel, false);
  			/* These commands never recurse */
  			/* No command-specific prep needed */
  			pass = AT_PASS_MISC;
--- 2617,2622 ----
***************
*** 3278,3314 **** ATGetQueueEntry(List **wqueue, Relation rel)
  }
  
  /*
!  * ATSimplePermissions
   *
!  * - Ensure that it is a relation (or possibly a view)
!  * - Ensure this user is the owner
!  * - Ensure that it is not a system table
   */
  static void
! ATSimplePermissions(Relation rel, bool allowView)
  {
! 	if (rel->rd_rel->relkind != RELKIND_RELATION)
! 	{
! 		if (allowView)
! 		{
! 			if (rel->rd_rel->relkind != RELKIND_VIEW)
! 				ereport(ERROR,
! 						(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! 						 errmsg("\"%s\" is not a table or view",
! 								RelationGetRelationName(rel))));
! 		}
! 		else
! 			ereport(ERROR,
! 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! 					 errmsg("\"%s\" is not a table",
! 							RelationGetRelationName(rel))));
! 	}
  
! 	/* Permissions checks */
  	if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
  		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
  					   RelationGetRelationName(rel));
  
  	if (!allowSystemTableMods && IsSystemRelation(rel))
  		ereport(ERROR,
  				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
--- 3361,3399 ----
  }
  
  /*
!  * ATCheckRelkind
!  * ATCheckOwnership
!  * ATCheckOwnership
   *
!  * These are originally checked in ATSimplePermissions
   */
  static void
! ATCheckRelkind(Relation rel, bool viewOK, bool indexOK)
  {
! 	char	relkind = rel->rd_rel->relkind;
  
! 	if (relkind != RELKIND_RELATION &&
! 		(!viewOK || relkind != RELKIND_RELATION) &&
! 		(!indexOK || relkind != RELKIND_INDEX))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! 				 errmsg("\"%s\" is not a table%s%s",
! 						RelationGetRelationName(rel),
! 						!viewOK ? "" : " or view",
! 						!indexOK ? "" : " or index")));
! }
! 
! static void
! ATCheckOwnership(Relation rel)
! {
  	if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
  		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
  					   RelationGetRelationName(rel));
+ }
  
+ static void
+ ATCheckNoCatalog(Relation rel)
+ {
  	if (!allowSystemTableMods && IsSystemRelation(rel))
  		ereport(ERROR,
  				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
***************
*** 3317,3322 **** ATSimplePermissions(Relation rel, bool allowView)
--- 3402,3422 ----
  }
  
  /*
+  * ATSimplePermissions
+  *
+  * - Ensure that it is a relation (or possibly a view)
+  * - Ensure this user is the owner
+  * - Ensure that it is not a system table
+  */
+ static void
+ ATSimplePermissions(Relation rel, bool allowView)
+ {
+ 	ATCheckRelkind(rel, allowView, false);
+ 	ATCheckOwnership(rel);
+ 	ATCheckNoCatalog(rel);
+ }
+ 
+ /*
   * ATSimplePermissionsRelationOrIndex
   *
   * - Ensure that it is a relation or an index
***************
*** 3326,3348 **** ATSimplePermissions(Relation rel, bool allowView)
  static void
  ATSimplePermissionsRelationOrIndex(Relation rel)
  {
! 	if (rel->rd_rel->relkind != RELKIND_RELATION &&
! 		rel->rd_rel->relkind != RELKIND_INDEX)
! 		ereport(ERROR,
! 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! 				 errmsg("\"%s\" is not a table or index",
! 						RelationGetRelationName(rel))));
! 
! 	/* Permissions checks */
! 	if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
! 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
! 					   RelationGetRelationName(rel));
! 
! 	if (!allowSystemTableMods && IsSystemRelation(rel))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 				 errmsg("permission denied: \"%s\" is a system catalog",
! 						RelationGetRelationName(rel))));
  }
  
  /*
--- 3426,3434 ----
  static void
  ATSimplePermissionsRelationOrIndex(Relation rel)
  {
! 	ATCheckRelkind(rel, false, true);
! 	ATCheckOwnership(rel);
! 	ATCheckNoCatalog(rel);
  }
  
  /*
***************
*** 4071,4098 **** ATExecColumnDefault(Relation rel, const char *colName,
   * ALTER TABLE ALTER COLUMN SET STATISTICS
   */
  static void
- ATPrepSetStatistics(Relation rel, const char *colName, Node *newValue)
- {
- 	/*
- 	 * We do our own permission checking because (a) we want to allow SET
- 	 * STATISTICS on indexes (for expressional index columns), and (b) we want
- 	 * to allow SET STATISTICS on system catalogs without requiring
- 	 * allowSystemTableMods to be turned on.
- 	 */
- 	if (rel->rd_rel->relkind != RELKIND_RELATION &&
- 		rel->rd_rel->relkind != RELKIND_INDEX)
- 		ereport(ERROR,
- 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- 				 errmsg("\"%s\" is not a table or index",
- 						RelationGetRelationName(rel))));
- 
- 	/* Permissions checks */
- 	if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
- 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
- 					   RelationGetRelationName(rel));
- }
- 
- static void
  ATExecSetStatistics(Relation rel, const char *colName, Node *newValue)
  {
  	int			newtarget;
--- 4157,4162 ----
***************
*** 4155,4182 **** ATExecSetStatistics(Relation rel, const char *colName, Node *newValue)
   * ALTER TABLE ALTER COLUMN SET STATISTICS DISTINCT
   */
  static void
- ATPrepSetDistinct(Relation rel, const char *colName, Node *newValue)
- {
- 	/*
- 	 * We do our own permission checking because (a) we want to allow SET
- 	 * DISTINCT on indexes (for expressional index columns), and (b) we want
- 	 * to allow SET DISTINCT on system catalogs without requiring
- 	 * allowSystemTableMods to be turned on.
- 	 */
- 	if (rel->rd_rel->relkind != RELKIND_RELATION &&
- 		rel->rd_rel->relkind != RELKIND_INDEX)
- 		ereport(ERROR,
- 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- 				 errmsg("\"%s\" is not a table or index",
- 						RelationGetRelationName(rel))));
- 
- 	/* Permissions checks */
- 	if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
- 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
- 					   RelationGetRelationName(rel));
- }
- 
- static void
  ATExecSetDistinct(Relation rel, const char *colName, Node *newValue)
  {
  	float4		newdistinct;
--- 4219,4224 ----
***************
*** 4525,4530 **** ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
--- 4567,4582 ----
  
  	Assert(IsA(stmt, IndexStmt));
  
+ 	/*
+ 	 * XXX - IMO, we should apply all the permission checks in DefineIndex()
+ 	 * when it is invoked with check_rights == true.
+ 	 * Here are only two code paths. The one is ATExecAddIndex() by AT_AddIndex,
+ 	 * the other is standard_ProcessUtility() by T_IndexStmt which also calls
+ 	 * CheckRelationOwnership() on the relation to be indexed just before
+ 	 * DefineIndex() invocation.
+ 	 */
+ 	ATSimplePermissions(rel, false);
+ 
  	/* suppress schema rights check when rebuilding existing index */
  	check_rights = !is_rebuild;
  	/* skip index build if phase 3 will have to rewrite table anyway */
***************
*** 4634,4642 **** ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
  	List	   *children;
  	ListCell   *child;
  
! 	/* At top level, permission check was done in ATPrepCmd, else do it */
! 	if (recursing)
! 		ATSimplePermissions(rel, false);
  
  	/*
  	 * Call AddRelationNewConstraints to do the work, making sure it works on
--- 4686,4693 ----
  	List	   *children;
  	ListCell   *child;
  
! 	/* ATPermCmd does not check permission */
! 	ATSimplePermissions(rel, false);
  
  	/*
  	 * Call AddRelationNewConstraints to do the work, making sure it works on
***************
*** 4742,4747 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 4793,4804 ----
  	Oid			constrOid;
  
  	/*
+ 	 * Sanity checks can be early
+ 	 */
+ 	ATCheckRelkind(rel, false, false);
+ 	ATCheckNoCatalog(rel);
+ 
+ 	/*
  	 * Grab an exclusive lock on the pk table, so that someone doesn't delete
  	 * rows out from under us. (Although a lesser lock would do for that
  	 * purpose, we'll need exclusive lock anyway to add triggers to the pk
***************
*** 4754,4770 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
  	 * Validity checks (permission checks wait till we have the column
  	 * numbers)
  	 */
! 	if (pkrel->rd_rel->relkind != RELKIND_RELATION)
! 		ereport(ERROR,
! 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! 				 errmsg("referenced relation \"%s\" is not a table",
! 						RelationGetRelationName(pkrel))));
! 
! 	if (!allowSystemTableMods && IsSystemRelation(pkrel))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 				 errmsg("permission denied: \"%s\" is a system catalog",
! 						RelationGetRelationName(pkrel))));
  
  	/*
  	 * Disallow reference from permanent table to temp table or vice versa.
--- 4811,4818 ----
  	 * Validity checks (permission checks wait till we have the column
  	 * numbers)
  	 */
! 	ATCheckRelkind(pkrel, false, false);
! 	ATCheckNoCatalog(pkrel);
  
  	/*
  	 * Disallow reference from permanent table to temp table or vice versa.
***************
*** 4832,4837 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 4880,4886 ----
  	/*
  	 * Now we can check permissions.
  	 */
+ 	ATCheckOwnership(rel);
  	checkFkeyPermissions(pkrel, pkattnum, numpks);
  	checkFkeyPermissions(rel, fkattnum, numfks);
  
***************
*** 6739,6748 **** ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename)
--- 6788,6802 ----
  				 errmsg("tablespace \"%s\" does not exist", tablespacename)));
  
  	/* Check its permissions */
+ 	ATCheckRelkind(rel, false, true);
+ 	ATCheckOwnership(rel);
+ 
  	aclresult = pg_tablespace_aclcheck(tablespaceId, GetUserId(), ACL_CREATE);
  	if (aclresult != ACLCHECK_OK)
  		aclcheck_error(aclresult, ACL_KIND_TABLESPACE, tablespacename);
  
+ 	ATCheckNoCatalog(rel);
+ 
  	/* Save info for Phase 3 to do the real work */
  	if (OidIsValid(tab->newTableSpace))
  		ereport(ERROR,
***************
*** 7131,7139 **** ATExecAddInherit(Relation child_rel, RangeVar *parent)
  	parent_rel = heap_openrv(parent, AccessShareLock);
  
  	/*
! 	 * Must be owner of both parent and child -- child was checked by
! 	 * ATSimplePermissions call in ATPrepCmd
  	 */
  	ATSimplePermissions(parent_rel, false);
  
  	/* Permanent rels cannot inherit from temporary ones */
--- 7185,7193 ----
  	parent_rel = heap_openrv(parent, AccessShareLock);
  
  	/*
! 	 * Must be owner of both parent and child
  	 */
+ 	ATSimplePermissions(child_rel, false);
  	ATSimplePermissions(parent_rel, false);
  
  	/* Permanent rels cannot inherit from temporary ones */
***************
*** 7782,7787 **** AlterTableNamespace(RangeVar *relation, const char *newschema,
--- 7836,7847 ----
  							RelationGetRelationName(rel))));
  	}
  
+ 	/*
+ 	 * XXX - No need to check relkind here
+ 	 */
+ 	ATCheckOwnership(rel);
+ 	ATCheckNoCatalog(rel);
+ 
  	/* get schema OID and check its permissions */
  	nspOid = LookupCreationNamespace(newschema);