0003-Add-some-review-comments.patch

From 016aedc955ccd4cb2eb6cdead6957bf7cb58cee8 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 1 Dec 2011 17:30:48 +0100
Subject: [PATCH 3/3] Add some review comments

---
 src/backend/catalog/dependency.c  |    2 +-
 src/backend/commands/cmdtrigger.c |   25 +++++++++++++++++++------
 src/backend/nodes/readfuncs.c     |    2 +-
 src/backend/utils/adt/ruleutils.c |   11 +++++++++--
 4 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 42701f3..5e53e08 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -2843,7 +2843,7 @@ getObjectDescription(const ObjectAddress *object)
 
 				trig = (Form_pg_cmdtrigger) GETSTRUCT(tup);
 
-				appendStringInfo(&buffer, _("trigger %s on %s"),
+				appendStringInfo(&buffer, _("command trigger %s on %s"),
 								 NameStr(trig->ctgname),
 								 NameStr(trig->ctgcommand));
 
diff --git a/src/backend/commands/cmdtrigger.c b/src/backend/commands/cmdtrigger.c
index 733c798..3780521 100644
--- a/src/backend/commands/cmdtrigger.c
+++ b/src/backend/commands/cmdtrigger.c
@@ -129,6 +129,8 @@ CreateCmdTrigger(CreateCmdTrigStmt *stmt, const char *queryString)
 						 errdetail("Commands cannot have both AFTER and INSTEAD OF triggers.")));
             break;
 		}
+		default:
+			elog(ERROR, "unknown trigger type for COMMAND TRIGGER");
 	}
 
 	if (ctgtype == CMD_TRIGGER_FIRED_BEFORE && funcrettype != BOOLOID)
@@ -143,6 +145,8 @@ CreateCmdTrigger(CreateCmdTrigStmt *stmt, const char *queryString)
 				 errmsg("function \"%s\" must return type \"void\"",
 						NameListToString(stmt->funcname))));
 
+	//FIXME: check argument types?
+
 	/*
 	 * Build the new pg_trigger tuple.
 	 */
@@ -259,8 +263,8 @@ AlterCmdTrigger(AlterCmdTrigStmt *stmt)
 	Form_pg_cmdtrigger cmdForm;
 	char        tgenabled = pstrdup(stmt->tgenabled)[0]; /* works with gram.y */
 
-	tgrel = heap_open(CmdTriggerRelationId, AccessShareLock);
-
+	tgrel = heap_open(CmdTriggerRelationId, AccessShareLock);//FIXME: wrong lock level?
+	//FIXME: need a row level lock here
 	ScanKeyInit(&skey[0],
 				Anum_pg_cmdtrigger_ctgcommand,
 				BTEqualStrategyNumber, F_NAMEEQ,
@@ -315,6 +319,7 @@ RenameCmdTrigger(List *name, const char *trigname, const char *newname)
 
 	rel = heap_open(CmdTriggerRelationId, RowExclusiveLock);
 
+	//FIXME: need a row level lock here
 	/* newname must be available */
 	check_cmdtrigger_name(command, newname, rel);
 
@@ -432,7 +437,7 @@ check_cmdtrigger_name(const char *command, const char *trigname, Relation tgrel)
 
 	tuple = systable_getnext(tgscan);
 
-	elog(NOTICE, "check_cmdtrigger_name(%s, %s)", command, trigname);
+	elog(DEBUG1, "check_cmdtrigger_name(%s, %s)", command, trigname);
 
 	if (HeapTupleIsValid(tuple))
 		ereport(ERROR,
@@ -464,7 +469,8 @@ check_cmdtrigger_name(const char *command, const char *trigname, Relation tgrel)
  * nodeToString() output.
  *
  */
-
+//FIXME: Return a List here.
+//FIXME: add abort-after-first for CreateCmdTrigger?
 static RegProcedure *
 list_triggers_for_command(const char *command, char type)
 {
@@ -500,7 +506,7 @@ list_triggers_for_command(const char *command, char type)
 		{
 			if (count == size)
 			{
-				size += 10;
+				size += 10;//FIXME: WTF?
 				procs = (Oid *)repalloc(procs, size);
 			}
 			procs[count++] = cmd->ctgfoid;
@@ -560,7 +566,7 @@ call_cmdtrigger_procedure(RegProcedure proc, CommandContext cmd,
  * Instead Of triggers, not both.
  *
  * Instead Of triggers have to run before the command and to cancel its
- * execution , hence this API where we return the number of InsteadOf trigger
+ * execution, hence this API where we return the number of InsteadOf trigger
  * procedures we fired.
  */
 int
@@ -591,6 +597,8 @@ ExecBeforeOrInsteadOfCommandTriggers(Node *parsetree, const char *cmdtag)
 	return nb;
 }
 
+//FIXME: This looks like it should be static
+//FIXME: why not return the number of calls here as well?
 bool
 ExecBeforeCommandTriggers(Node *parsetree, const char *cmdtag,
 						  MemoryContext per_command_context)
@@ -602,6 +610,7 @@ ExecBeforeCommandTriggers(Node *parsetree, const char *cmdtag,
 	RegProcedure proc;
 	int cur= 0;
 	bool cont = true;
+	//FIXME: add assert for IsA(parsetree, parsetree)
 
 	/*
 	 * Do the functions evaluation in a per-command memory context, so that
@@ -631,6 +640,7 @@ ExecBeforeCommandTriggers(Node *parsetree, const char *cmdtag,
 /*
  * return the count of triggers we fired
  */
+//FIXME: This looks like it should be static
 int
 ExecInsteadOfCommandTriggers(Node *parsetree, const char *cmdtag,
 							 MemoryContext per_command_context)
@@ -641,6 +651,7 @@ ExecInsteadOfCommandTriggers(Node *parsetree, const char *cmdtag,
 													CMD_TRIGGER_FIRED_INSTEAD);
 	RegProcedure proc;
 	int cur = 0;
+	//FIXME: add assert for IsA(parsetree, parsetree)
 
 	/*
 	 * Do the functions evaluation in a per-command memory context, so that
@@ -673,6 +684,8 @@ ExecAfterCommandTriggers(Node *parsetree, const char *cmdtag)
 	RegProcedure proc;
 	int cur = 0;
 
+	//FIXME: add assert for IsA(parsetree, parsetree)
+
 	/*
 	 * Do the functions evaluation in a per-command memory context, so that
 	 * leaked memory will be reclaimed once per command.
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 2b1a76d..1505683 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -384,6 +384,7 @@ _readConstraint(void)
 	token = pg_strtok(&length); /* skip :constraint */
 	token = pg_strtok(&length); /* get field value */
 
+	//FIXME: what is going here?
 	if (strncmp(token, "NULL", 4) == 0)
 		local_node->contype = CONSTR_NULL;
 	else if (strncmp(token, "NOT_NULL", 8) == 0)
@@ -1428,7 +1429,6 @@ _readRangeTblEntry(void)
 	READ_DONE();
 }
 
-
 /*
  * parseNodeString
  *
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 83775bf..2a14281 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7332,6 +7332,9 @@ flatten_reloptions(Oid relid)
  * Functions that ouputs a COMMAND given a Utility parsetree
  *
  * FIXME: First some tools that I couldn't find in the sources.
+ * FIXME: replace conversion to rangevar and back to string with
+ *  NameListToQuotedString
+ * FIXME: missing quoting
  */
 static char *
 RangeVarToString(RangeVar *r)
@@ -7506,8 +7509,8 @@ _rwViewStmt(CommandContext cmd, ViewStmt *node)
 	viewParse = parse_analyze((Node *) copyObject(node->query),
 							  "(unavailable source text)", NULL, 0);
 
-	appendStringInfo(&buf, "CREATE %s %s AS ",
-					 node->replace? "OR REPLACE VIEW": "VIEW",
+	appendStringInfo(&buf, "CREATE %sVIEW %s AS ",
+					 node->replace? "OR REPLACE": "",
 					 RangeVarToString(node->view));
 
 	get_query_def(viewParse, &buf, NIL, NULL, 0, 1);
@@ -7526,6 +7529,7 @@ _rwColQualList(StringInfo buf, List *constraints, const char *relname)
 	foreach(lc, constraints)
 	{
 		Constraint *c = (Constraint *) lfirst(lc);
+		Assert(IsA(c, Constraint));
 
 		if (c->conname != NULL)
 			appendStringInfo(buf, " CONSTRAINT %s", c->conname);
@@ -7897,10 +7901,13 @@ _rwAlterTableStmt(CommandContext cmd, AlterTableStmt *node)
  * work from the parsetree directly, that would be query->utilityStmt which is
  * of type Node *. We declare that a void * to avoid incompatible pointer type
  * warnings.
+ *
+ * FIXME: just add a cast + assert at the callsite ^^^?
  */
 void
 pg_get_cmddef(CommandContext cmd, void *parsetree)
 {
+	//FIXME: at least add an assert for type
 	cmd->nodestr = nodeToString(parsetree);
 	/* elog(NOTICE, "nodeToString: %s", cmd->nodestr); */
 	stringToNode(cmd->nodestr);
-- 
1.7.6.409.ge7a85.dirty