From 016aedc955ccd4cb2eb6cdead6957bf7cb58cee8 Mon Sep 17 00:00:00 2001 From: Andres Freund 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