Improve behavior of concurrent rename statements.

Previously, renaming a table, sequence, view, index, foreign table,
column, or trigger checked permissions before locking the object, which
meant that if permissions were revoked during the lock wait, we would
still allow the operation.  Similarly, if the original object is dropped
and a new one with the same name is created, the operation will be allowed
if we had permissions on the old object; the permissions on the new
object don't matter.  All this is now fixed.

Along the way, attempting to rename a trigger on a foreign table now gives
the same error message as trying to create one there in the first place
(i.e. that it's not a table or view) rather than simply stating that no
trigger by that name exists.

Patch by me; review by Noah Misch.

1 files changed, 51 insertions, 12 deletions

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b205deca29f..f4c93e5b254 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1152,6 +1152,39 @@ get_trigger_oid(Oid relid, const char *trigname, bool missing_ok)
 }
 
 /*
+ * Perform permissions and integrity checks before acquiring a relation lock.
+ */
+static void
+RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
+								 void *arg)
+{
+    HeapTuple       tuple;
+    Form_pg_class   form;
+
+	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+	if (!HeapTupleIsValid(tuple))
+		return;                         /* concurrently dropped */
+	form = (Form_pg_class) GETSTRUCT(tuple);
+
+	/* only tables and views can have triggers */
+    if (form->relkind != RELKIND_RELATION && form->relkind != RELKIND_VIEW)
+        ereport(ERROR,
+                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                 errmsg("\"%s\" is not a table or view", rv->relname)));
+
+	/* you must own the table to rename one of its triggers */
+    if (!pg_class_ownercheck(relid, GetUserId()))
+        aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, rv->relname);
+    if (!allowSystemTableMods && IsSystemClass(form))
+        ereport(ERROR,
+                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                 errmsg("permission denied: \"%s\" is a system catalog",
+						rv->relname)));
+
+	ReleaseSysCache(tuple);
+}
+
+/*
  *		renametrig		- changes the name of a trigger on a relation
  *
  *		trigger name is changed in trigger catalog.
@@ -1165,21 +1198,26 @@ get_trigger_oid(Oid relid, const char *trigname, bool missing_ok)
  *		update row in catalog
  */
 void
-renametrig(Oid relid,
-		   const char *oldname,
-		   const char *newname)
+renametrig(RenameStmt *stmt)
 {
 	Relation	targetrel;
 	Relation	tgrel;
 	HeapTuple	tuple;
 	SysScanDesc tgscan;
 	ScanKeyData key[2];
+	Oid			relid;
 
 	/*
-	 * Grab an exclusive lock on the target table, which we will NOT release
-	 * until end of transaction.
+	 * Look up name, check permissions, and acquire lock (which we will NOT
+	 * release until end of transaction).
 	 */
-	targetrel = heap_open(relid, AccessExclusiveLock);
+	relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
+									 false, false,
+									 RangeVarCallbackForRenameTrigger,
+									 NULL);
+
+	/* Have lock already, so just need to build relcache entry. */
+	targetrel = relation_open(relid, NoLock);
 
 	/*
 	 * Scan pg_trigger twice for existing triggers on relation.  We do this in
@@ -1202,14 +1240,14 @@ renametrig(Oid relid,
 	ScanKeyInit(&key[1],
 				Anum_pg_trigger_tgname,
 				BTEqualStrategyNumber, F_NAMEEQ,
-				PointerGetDatum(newname));
+				PointerGetDatum(stmt->newname));
 	tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true,
 								SnapshotNow, 2, key);
 	if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
 		ereport(ERROR,
 				(errcode(ERRCODE_DUPLICATE_OBJECT),
 				 errmsg("trigger \"%s\" for relation \"%s\" already exists",
-						newname, RelationGetRelationName(targetrel))));
+						stmt->newname, RelationGetRelationName(targetrel))));
 	systable_endscan(tgscan);
 
 	/*
@@ -1222,7 +1260,7 @@ renametrig(Oid relid,
 	ScanKeyInit(&key[1],
 				Anum_pg_trigger_tgname,
 				BTEqualStrategyNumber, F_NAMEEQ,
-				PointerGetDatum(oldname));
+				PointerGetDatum(stmt->subname));
 	tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true,
 								SnapshotNow, 2, key);
 	if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
@@ -1232,7 +1270,8 @@ renametrig(Oid relid,
 		 */
 		tuple = heap_copytuple(tuple);	/* need a modifiable copy */
 
-		namestrcpy(&((Form_pg_trigger) GETSTRUCT(tuple))->tgname, newname);
+		namestrcpy(&((Form_pg_trigger) GETSTRUCT(tuple))->tgname,
+				   stmt->newname);
 
 		simple_heap_update(tgrel, &tuple->t_self, tuple);
 
@@ -1251,7 +1290,7 @@ renametrig(Oid relid,
 		ereport(ERROR,
 				(errcode(ERRCODE_UNDEFINED_OBJECT),
 				 errmsg("trigger \"%s\" for table \"%s\" does not exist",
-						oldname, RelationGetRelationName(targetrel))));
+						stmt->subname, RelationGetRelationName(targetrel))));
 	}
 
 	systable_endscan(tgscan);
@@ -1261,7 +1300,7 @@ renametrig(Oid relid,
 	/*
 	 * Close rel, but keep exclusive lock!
 	 */
-	heap_close(targetrel, NoLock);
+	relation_close(targetrel, NoLock);
 }