Some small polishing of Mark Hollomon's cleanup of DROP command: might

as well allow DROP multiple INDEX, RULE, TYPE as well.  Add missing
CommandCounterIncrement to DROP loop, which could cause trouble otherwise
with multiple DROP of items affecting same catalog entries.  Try to
bring a little consistency to various error messages using 'does not exist',
'nonexistent', etc --- I standardized on 'does not exist' since that's
what the vast majority of the existing uses seem to be.

src/backend/catalog/heap.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/catalog/heap.c,v 1.149 2000/10/16 14:52:02 vadim Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/catalog/heap.c,v 1.150 2000/10/22 23:32:38 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -1091,7 +1091,7 @@ DeleteRelationTuple(Relation rel)
 	if (!HeapTupleIsValid(tup))
 	{
 		heap_close(pg_class_desc, RowExclusiveLock);
-		elog(ERROR, "Relation '%s' does not exist",
+		elog(ERROR, "Relation \"%s\" does not exist",
 			 RelationGetRelationName(rel));
 	}
 
@@ -1342,7 +1342,7 @@ DeleteTypeTuple(Relation rel)
 	{
 		heap_endscan(pg_type_scan);
 		heap_close(pg_type_desc, RowExclusiveLock);
-		elog(ERROR, "DeleteTypeTuple: %s type nonexistent",
+		elog(ERROR, "DeleteTypeTuple: type \"%s\" does not exist",
 			 RelationGetRelationName(rel));
 	}
 

src/backend/catalog/pg_operator.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/catalog/pg_operator.c,v 1.51 2000/08/21 17:22:35 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/catalog/pg_operator.c,v 1.52 2000/10/22 23:32:38 tgl Exp $
  *
  * NOTES
  *	  these routines moved here from commands/define.c and somewhat cleaned up.
@@ -181,7 +181,7 @@ OperatorGet(char *operatorName,
 		leftObjectId = TypeGet(leftTypeName, &leftDefined);
 
 		if (!OidIsValid(leftObjectId) || !leftDefined)
-			elog(ERROR, "OperatorGet: left type '%s' nonexistent",
+			elog(ERROR, "OperatorGet: left type \"%s\" does not exist",
 				 leftTypeName);
 	}
 
@@ -190,7 +190,7 @@ OperatorGet(char *operatorName,
 		rightObjectId = TypeGet(rightTypeName, &rightDefined);
 
 		if (!OidIsValid(rightObjectId) || !rightDefined)
-			elog(ERROR, "OperatorGet: right type '%s' nonexistent",
+			elog(ERROR, "OperatorGet: right type \"%s\" does not exist",
 				 rightTypeName);
 	}
 
@@ -527,7 +527,7 @@ OperatorDef(char *operatorName,
 		leftTypeId = TypeGet(leftTypeName, &leftDefined);
 
 		if (!OidIsValid(leftTypeId) || !leftDefined)
-			elog(ERROR, "OperatorDef: left type '%s' nonexistent",
+			elog(ERROR, "OperatorDef: left type \"%s\" does not exist",
 				 leftTypeName);
 	}
 
@@ -536,7 +536,7 @@ OperatorDef(char *operatorName,
 		rightTypeId = TypeGet(rightTypeName, &rightDefined);
 
 		if (!OidIsValid(rightTypeId) || !rightDefined)
-			elog(ERROR, "OperatorDef: right type '%s' nonexistent",
+			elog(ERROR, "OperatorDef: right type \"%s\" does not exist",
 				 rightTypeName);
 	}
 
@@ -739,7 +739,7 @@ OperatorDef(char *operatorName,
 											  otherRightTypeName);
 				if (!OidIsValid(other_oid))
 					elog(ERROR,
-						 "OperatorDef: can't create operator shell '%s'",
+						 "OperatorDef: can't create operator shell \"%s\"",
 						 name[j]);
 				values[i++] = ObjectIdGetDatum(other_oid);
 			}

src/backend/commands/indexcmds.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v 1.38 2000/09/06 14:15:16 petere Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v 1.39 2000/10/22 23:32:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -610,7 +610,7 @@ RemoveIndex(char *name)
 								0, 0, 0);
 
 	if (!HeapTupleIsValid(tuple))
-		elog(ERROR, "index \"%s\" nonexistent", name);
+		elog(ERROR, "index \"%s\" does not exist", name);
 
 	if (((Form_pg_class) GETSTRUCT(tuple))->relkind != RELKIND_INDEX)
 	{
@@ -640,7 +640,7 @@ ReindexIndex(const char *name, bool force /* currently unused */ )
 								0, 0, 0);
 
 	if (!HeapTupleIsValid(tuple))
-		elog(ERROR, "index \"%s\" nonexistent", name);
+		elog(ERROR, "index \"%s\" does not exist", name);
 
 	if (((Form_pg_class) GETSTRUCT(tuple))->relkind != RELKIND_INDEX)
 	{
@@ -650,7 +650,7 @@ ReindexIndex(const char *name, bool force /* currently unused */ )
 	}
 
 	if (!reindex_index(tuple->t_data->t_oid, force))
-		elog(NOTICE, "index '%s' wasn't reindexed", name);
+		elog(NOTICE, "index \"%s\" wasn't reindexed", name);
 }
 
 /*
@@ -671,7 +671,7 @@ ReindexTable(const char *name, bool force)
 								0, 0, 0);
 
 	if (!HeapTupleIsValid(tuple))
-		elog(ERROR, "table \"%s\" nonexistent", name);
+		elog(ERROR, "table \"%s\" does not exist", name);
 
 	if (((Form_pg_class) GETSTRUCT(tuple))->relkind != RELKIND_RELATION)
 	{
@@ -681,7 +681,7 @@ ReindexTable(const char *name, bool force)
 	}
 
 	if (!reindex_relation(tuple->t_data->t_oid, force))
-		elog(NOTICE, "table '%s' wasn't reindexed", name);
+		elog(NOTICE, "table \"%s\" wasn't reindexed", name);
 }
 
 /*
@@ -719,7 +719,7 @@ ReindexDatabase(const char *dbname, bool force, bool all)
 	scan = heap_beginscan(relation, 0, SnapshotNow, 1, &scankey);
 	dbtuple = heap_getnext(scan, 0);
 	if (!HeapTupleIsValid(dbtuple))
-		elog(ERROR, "Database \"%s\" doesn't exist", dbname);
+		elog(ERROR, "Database \"%s\" does not exist", dbname);
 	db_id = dbtuple->t_data->t_oid;
 	db_owner = ((Form_pg_database) GETSTRUCT(dbtuple))->datdba;
 	heap_endscan(scan);
@@ -789,7 +789,7 @@ ReindexDatabase(const char *dbname, bool force, bool all)
 	{
 		StartTransactionCommand();
 		if (reindex_relation(relids[i], force))
-			elog(NOTICE, "relation %d was reindexed", relids[i]);
+			elog(NOTICE, "relation %u was reindexed", relids[i]);
 		CommitTransactionCommand();
 	}
 	StartTransactionCommand();

src/backend/commands/rename.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/commands/Attic/rename.c,v 1.50 2000/10/20 02:53:10 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/commands/Attic/rename.c,v 1.51 2000/10/22 23:32:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -137,7 +137,7 @@ renameatt(char *relname,
 										PointerGetDatum(oldattname),
 										0, 0);
 	if (!HeapTupleIsValid(oldatttup))
-		elog(ERROR, "renameatt: attribute \"%s\" nonexistent", oldattname);
+		elog(ERROR, "renameatt: attribute \"%s\" does not exist", oldattname);
 
 	if (((Form_pg_attribute) GETSTRUCT(oldatttup))->attnum < 0)
 		elog(ERROR, "renameatt: system attribute \"%s\" not renamed", oldattname);

src/backend/parser/gram.y

@@ -11,7 +11,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/parser/gram.y,v 2.196 2000/10/18 16:16:05 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/parser/gram.y,v 2.197 2000/10/22 23:32:48 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -125,7 +125,7 @@ static void doNegateFloat(Value *v);
 		DropUserStmt, DropdbStmt, ExplainStmt, ExtendStmt, FetchStmt,
 		GrantStmt, IndexStmt, InsertStmt, ListenStmt, LoadStmt, LockStmt,
 		NotifyStmt, OptimizableStmt, ProcedureStmt, ReindexStmt,
-		RemoveAggrStmt, RemoveFuncStmt, RemoveOperStmt, RemoveStmt,
+		RemoveAggrStmt, RemoveFuncStmt, RemoveOperStmt,
 		RenameStmt, RevokeStmt, RuleActionStmt, RuleActionStmtOrEmpty,
 		RuleStmt, SelectStmt, SetSessionStmt, TransactionStmt, TruncateStmt,
 		UnlistenStmt, UpdateStmt, VacuumStmt, VariableResetStmt,
@@ -202,7 +202,7 @@ static void doNegateFloat(Value *v);
 				opt_with_copy, index_opt_unique, opt_verbose, opt_analyze
 %type <boolean> opt_cursor
 
-%type <ival>	copy_dirn, def_type, direction, reindex_type, remove_type,
+%type <ival>	copy_dirn, def_type, direction, reindex_type, drop_type,
 		opt_column, event, comment_type, comment_cl,
 		comment_ag, comment_fn, comment_op, comment_tg
 
@@ -447,7 +447,6 @@ stmt :	AlterSchemaStmt
 		| RemoveAggrStmt
 		| RemoveOperStmt
 		| RemoveFuncStmt
-		| RemoveStmt
 		| RenameStmt
 		| RevokeStmt
 		| OptimizableStmt
@@ -1935,33 +1934,28 @@ def_arg:  func_return  					{  $$ = (Node *)$1; }
 /*****************************************************************************
  *
  *		QUERY:
- *				drop <relname1> [, <relname2> .. <relnameN> ]
+ *
+ *		DROP itemtype itemname [, itemname ...]
  *
  *****************************************************************************/
 
-DropStmt:  DROP TABLE relation_name_list
+DropStmt:  DROP drop_type name_list
 				{
 					DropStmt *n = makeNode(DropStmt);
+					n->removeType = $2;
 					n->names = $3;
-					n->removeType = DROP_TABLE;
-					$$ = (Node *)n;
-				}
-		| DROP SEQUENCE relation_name_list
-				{
-					DropStmt *n = makeNode(DropStmt);
-					n->names = $3;
-					n->removeType = DROP_SEQUENCE;
-					$$ = (Node *)n;
-				}
-		| DROP VIEW relation_name_list
-				{
-					DropStmt *n = makeNode(DropStmt);
-					n->names = $3;
-					n->removeType = DROP_VIEW;
 					$$ = (Node *)n;
 				}
 		;
 
+drop_type: TABLE								{ $$ = DROP_TABLE; }
+		| SEQUENCE								{ $$ = DROP_SEQUENCE; }
+		| VIEW									{ $$ = DROP_VIEW; }
+		| INDEX									{ $$ = DROP_INDEX; }
+		| RULE									{ $$ = DROP_RULE; }
+		| TYPE_P								{ $$ = DROP_TYPE_P; }
+		;
+
 /*****************************************************************************
  *
  *		QUERY:
@@ -2550,34 +2544,21 @@ func_return:  Typename
  *
  *		QUERY:
  *
- *		remove function <funcname>
- *				(REMOVE FUNCTION "funcname" (arg1, arg2, ...))
- *		remove aggregate <aggname>
- *				(REMOVE AGGREGATE "aggname" "aggtype")
- *		remove operator <opname>
- *				(REMOVE OPERATOR "opname" (leftoperand_typ rightoperand_typ))
- *		remove type <typename>
- *				(REMOVE TYPE "typename")
- *		remove rule <rulename>
- *				(REMOVE RULE "rulename")
+ *		DROP FUNCTION funcname (arg1, arg2, ...)
+ *		DROP AGGREGATE aggname aggtype
+ *		DROP OPERATOR opname (leftoperand_typ rightoperand_typ)
  *
  *****************************************************************************/
 
-RemoveStmt:  DROP remove_type name
+RemoveFuncStmt:  DROP FUNCTION func_name func_args
 				{
-					DropStmt *n = makeNode(DropStmt);
-					n->removeType = $2;
-					n->names =  makeList1(makeString($3));
+					RemoveFuncStmt *n = makeNode(RemoveFuncStmt);
+					n->funcname = $3;
+					n->args = $4;
 					$$ = (Node *)n;
 				}
 		;
 
-remove_type:  TYPE_P							{  $$ = DROP_TYPE_P; }
-		| INDEX									{  $$ = DROP_INDEX; }
-		| RULE									{  $$ = DROP_RULE; }
-		;
-
-
 RemoveAggrStmt:  DROP AGGREGATE name aggr_argtype
 				{
 						RemoveAggrStmt *n = makeNode(RemoveAggrStmt);
@@ -2591,17 +2572,6 @@ aggr_argtype:  Typename							{ $$ = $1; }
 		| '*'									{ $$ = NULL; }
 		;
 
-
-RemoveFuncStmt:  DROP FUNCTION func_name func_args
-				{
-					RemoveFuncStmt *n = makeNode(RemoveFuncStmt);
-					n->funcname = $3;
-					n->args = $4;
-					$$ = (Node *)n;
-				}
-		;
-
-
 RemoveOperStmt:  DROP OPERATOR all_Op '(' oper_argtypes ')'
 				{
 					RemoveOperStmt *n = makeNode(RemoveOperStmt);

src/backend/tcop/utility.c

@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/tcop/utility.c,v 1.97 2000/10/18 16:16:06 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/tcop/utility.c,v 1.98 2000/10/22 23:32:41 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -47,21 +47,22 @@
 
 
 /*
- *
+ * Error-checking support for DROP commands
  */
 
 struct kindstrings {
 	char kind;
+	char *indef_article;
 	char *name;
 	char *command;
 };
 
 static struct kindstrings kindstringarray[] = {
-	{ RELKIND_RELATION, "table", "TABLE" },
-	{ RELKIND_SEQUENCE, "sequence", "SEQUENCE" },
-	{ RELKIND_VIEW, "view", "VIEW" },
-	{ RELKIND_INDEX, "index", "INDEX" },
-	{ '\0', "", "" }
+	{ RELKIND_RELATION, "a", "table", "TABLE" },
+	{ RELKIND_SEQUENCE, "a", "sequence", "SEQUENCE" },
+	{ RELKIND_VIEW, "a", "view", "VIEW" },
+	{ RELKIND_INDEX, "an", "index", "INDEX" },
+	{ '\0', "a", "???", "???" }
 };
 
 
@@ -81,36 +82,44 @@ DropErrorMsg(char* relname, char wrongkind, char rightkind)
 			break;
 	Assert(wentry->kind != '\0');
 	
-	elog(ERROR, "%s is not a %s. Use 'DROP %s' to remove a %s",
-			relname, rentry->name, wentry->command, wentry->name);
+	elog(ERROR, "\"%s\" is not %s %s. Use DROP %s to remove %s %s",
+		 relname, rentry->indef_article, rentry->name,
+		 wentry->command, wentry->indef_article, wentry->name);
 }
 
 static void
-CheckClassKind(char *name, char rightkind)
+CheckDropPermissions(char *name, char rightkind)
 {
-	HeapTuple	tuple;
 	struct kindstrings *rentry;
+	HeapTuple	tuple;
 	Form_pg_class classform;
 
+	for (rentry = kindstringarray; rentry->kind != '\0'; rentry++)
+		if (rentry->kind == rightkind)
+			break;
+	Assert(rentry->kind != '\0');
+
 	tuple = SearchSysCacheTuple(RELNAME,
 								PointerGetDatum(name),
 								0, 0, 0);
-
 	if (!HeapTupleIsValid(tuple))
-	{
-		for (rentry = kindstringarray; rentry->kind != '\0'; rentry++)
-			if (rentry->kind == rightkind)
-				break;
-		Assert(rentry->kind != '\0');
-		elog(ERROR, "%s \"%s\" is nonexistent", rentry->name, name);
-	}
+		elog(ERROR, "%s \"%s\" does not exist", rentry->name, name);
 
 	classform = (Form_pg_class) GETSTRUCT(tuple);
 
 	if (classform->relkind != rightkind)
 		DropErrorMsg(name, classform->relkind, rightkind);
+
+	if (!pg_ownercheck(GetUserId(), name, RELNAME))
+		elog(ERROR, "you do not own %s \"%s\"",
+			 rentry->name, name);
+
+	if (!allowSystemTableMods && IsSystemRelationName(name))
+		elog(ERROR, "%s \"%s\" is a system %s",
+			 rentry->name, name, rentry->name);
 }
 
+
 /* ----------------
  *		general utility function invoker
  * ----------------
@@ -204,7 +213,6 @@ ProcessUtility(Node *parsetree,
 			/*
 			 * Let AlterTableCreateToastTable decide if this
 			 * one needs a secondary relation too.
-			 *
 			 */
 			CommandCounterIncrement();
 			AlterTableCreateToastTable(((CreateStmt *)parsetree)->relname,
@@ -217,51 +225,32 @@ ProcessUtility(Node *parsetree,
 				List	   *args = stmt->names;
 				List	   *arg;
 
-				foreach(arg, args) {
+				set_ps_display(commandTag = "DROP");
 
+				foreach(arg, args)
+				{
 					relname = strVal(lfirst(arg));
-					if (!allowSystemTableMods && IsSystemRelationName(relname))
-						elog(ERROR, "class \"%s\" is a system catalog",
-							 relname);
-
-					set_ps_display(commandTag = "DROP");
 
 					switch(stmt->removeType)
 					{
 						case DROP_TABLE:
-							CheckClassKind(relname, RELKIND_RELATION);
-							if (!pg_ownercheck(GetUserId(), relname, RELNAME))
-								elog(ERROR, "you do not own table \"%s\"",
-									 relname);
+							CheckDropPermissions(relname, RELKIND_RELATION);
 							RemoveRelation(relname);
-
 							break;
 
 						case DROP_SEQUENCE:
-							CheckClassKind(relname, RELKIND_SEQUENCE);
-							if (!pg_ownercheck(GetUserId(), relname, RELNAME))
-								elog(ERROR, "you do not own sequence \"%s\"",
-									 relname);
+							CheckDropPermissions(relname, RELKIND_SEQUENCE);
 							RemoveRelation(relname);
-
 							break;
 
 						case DROP_VIEW:
-							CheckClassKind(relname, RELKIND_VIEW);
-							if (!pg_ownercheck(GetUserId(), relname, RELNAME))
-								elog(ERROR, "you do not own view \"%s\"",
-									 relname);
+							CheckDropPermissions(relname, RELKIND_VIEW);
 							RemoveView(relname);
-
 							break;
 
 						case DROP_INDEX:
-							CheckClassKind(relname, RELKIND_INDEX);
-							if (!pg_ownercheck(GetUserId(), relname, RELNAME))
-								elog(ERROR, "%s: %s", relname, 
-										aclcheck_error_strings[ACLCHECK_NOT_OWNER]);
+							CheckDropPermissions(relname, RELKIND_INDEX);
 							RemoveIndex(relname);
-
 							break;
 
 						case DROP_RULE:
@@ -279,11 +268,18 @@ ProcessUtility(Node *parsetree,
 							break;
 
 						case DROP_TYPE_P:
+							/* RemoveType does its own permissions checks */
 							RemoveType(relname);
 							break;
 					}
-				}
 
+					/*
+					 * Make sure subsequent loop iterations will see results
+					 * of this one; needed if removing multiple rules for
+					 * same table, for example.
+					 */
+					CommandCounterIncrement();
+				}
 			}
 			break;