Hi Dimitri, Hi all,
On Tuesday, November 08, 2011 06:47:13 PM Dimitri Fontaine wrote:
> As proposed by Jan Wieck on hackers and discussed in Ottawa at the
> Clusters Hackers Meeting, the most (only?) workable way to ever have DDL
> triggers is to have "command triggers" instead.
> Rather than talking about catalogs and the like, it's all about firing a
> registered user-defined function before or after processing the utility
> command, or instead of processing it. This naturally involves a new
> catalog (pg_cmdtrigger) and some new subcommands of CREATE and DROP
> TRIGGER, and some support code for calling the function at the right
> time.
Great ;)
fwiw I think thats the way forward as well.
The patch obviously isn't thought to be commitable yet, so I am not going to
do a very detailed code level review. Attached is a patch with low level
targeted comments and such I noticed while reading it.
At this state the important things are highlevel so I will try to concentrate
on those:
== the trigger function ==
I don't like the set of options parsed to the trigger functions very much. To
cite an example of yours those currently are:
* cmd_string text
* cmd_nodestring text
* schemaname text
* relname text
I think at least a
* command_tag text
is missing. Sure, you can disambiguate it by creating a function for every
command type but that seems pointlessly complex for many applications. And
adding it should be trivial as its already tracked ;)
Currently the examples refer to relname as relname which I dislike as that
seems to be too restricted to one use case. The code is already doing it
correctly (as objectname) though ;)
Why is there explicit documentation of not checking the arguments of said
trigger function? That seems to be a bit strange to me.
schemaname currently is mightily unusable because whether its sent or not
depends wether the table was created with a schemaname specified or not. That
doesn't seem to be a good approach.
That brings me to the next point:
== normalization of statements ==
Currently the normalization is a bit confusing. E.g. for:
CREATE SCHEMA barf;
SET search_path = barf;
CREATE TYPE bart AS ENUM ('a', 'b');
CREATE TABLE bar(a int, b bigint, c serial, d text, e varchar, "F" text, g
bart, h int4);
one gets
CREATE TABLE bar (a pg_catalog.int4,b pg_catalog.int8,c serial,d text,e
pg_catalog.varchar,F text,g bart,h int4);
Which points out two problems:
* inconsistent schema prefixing
* no quoting
Imo the output should fully schema qualify anything including operators. Yes,
thats rather ugly but anything else seems to be too likely to lead to
problems.
As an alternative it would be possible to pass the current search path but
that doesn't seem to the best approach to me;
Currently CHECK() constraints are not decodable, but inside those the same
quoting/qualifying rules should apply.
Then there is nice stuff like CREATE TABLE .... (LIKE ...);
What shall that return in future? I vote for showing it as the plain CREATE
TABLE where all columns are specified.
That touches another related topic. Dim's work in adding support for utility
cmd support in nodeToString and friends possibly will make the code somewhat
command trigger specific. Is there any other usage we envision?
== grammar ==
* multiple actions:
I think it would sensible to allow multiple actions on which to trigger to be
specified just as you can for normal triggers. I also would like an ALL option
* options:
Currently the grammar allows options to be passed to command triggers. Do we
want to keep that? If yes, with the same arcane set of datatypes as in data
triggers?
If yes it needs to be wired up.
* schema qualification:
An option to add triggers only to a specific schema would be rather neat for
many scenarios.
I am not totally sure if its worth the hassle of defining what happens in the
edge cases of e.g. moving from one into another schema though.
== locking ==
Currently there is no locking strategy at all. Which e.g. means that there is
no protection against two sessions changing stuff concurrently or such.
Was that just left out - which is ok - or did you miss that?
I think it would be ok to just always grab row level locks there.
== permissions ==
Command triggers should only be allowed for the database owner.
== recursion ==
I contrast to data triggers command triggers cannot change what is done. They
can either prevent it from running or not. Which imo is fine.
The question I have is whether it would be a good idea to make it easy to
prevent recursion.... Especially if the triggers wont be per schema.
== calling the trigger ==
Imo the current callsite in ProcessUtility isn't that good. I think it would
make more sense moving it into standard_ProcessUtility. It should be *after*
the check_xact_readonly check in there in my opinion.
I am also don't think its a good idea to run the
ExecBeforeOrInsteadOfCommandTriggers stuff there because thats allso the path
that COMMIT/BEGIN and other stuff take. Those are pretty "fast path".
I suggest making two switches in standard_ProcessUtility, one for the non-
command trigger stuff which returns on success and one after that. Only after
the first triggers are checked.
I wonder whether its the right choice to run triggers on untransformed input.
I.e. CREATE TABLE ... (LIKE ...) seems to only make sense to me after
transforming.
Also youre very repeatedly transforming the parstree into a string. It would
be better doing that only once instead of every trigger...
Ok, thats the first round of high level stuff...
Cool Work!
Some lower level annotations:
* many functions should be static but are not. Especially in cmdtrigger.c
* Either tgenable's datatype or its readfunc is wrong (watch the compiler ;))
* ruleutils.c:
* generic routine for IF EXISTS, CASCADE, ...
Greetings,
Andres
From 99a67fa63e5bcec6631c00253653717688bbdfde Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Thu, 1 Dec 2011 14:18:55 +0100
Subject: [PATCH 1/3] Fix dependency recording for command triggers
* the oid is only defined after heap_insert
* use the correct relationship as classid
---
src/backend/commands/cmdtrigger.c | 11 ++++-------
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/cmdtrigger.c b/src/backend/commands/cmdtrigger.c
index 5a4d370..c6cda29 100644
--- a/src/backend/commands/cmdtrigger.c
+++ b/src/backend/commands/cmdtrigger.c
@@ -157,16 +157,13 @@ CreateCmdTrigger(CreateCmdTrigStmt *stmt, const char *queryString)
tuple = heap_form_tuple(tgrel->rd_att, values, nulls);
- /* force tuple to have the desired OID */
- trigoid = HeapTupleGetOid(tuple);
-
- /*
- * Insert tuple into pg_trigger.
- */
simple_heap_insert(tgrel, tuple);
CatalogUpdateIndexes(tgrel, tuple);
+ /* remember oid for record dependencies */
+ trigoid = HeapTupleGetOid(tuple);
+
heap_freetuple(tuple);
heap_close(tgrel, RowExclusiveLock);
@@ -174,7 +171,7 @@ CreateCmdTrigger(CreateCmdTrigStmt *stmt, const char *queryString)
* Record dependencies for trigger. Always place a normal dependency on
* the function.
*/
- myself.classId = TriggerRelationId;
+ myself.classId = CmdTriggerRelationId;
myself.objectId = trigoid;
myself.objectSubId = 0;
--
1.7.6.409.ge7a85.dirty
From f48b887b5090823a621ff896b01412b044f02f64 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Thu, 1 Dec 2011 14:20:17 +0100
Subject: [PATCH 2/3] Fix file headers of new cmdtrigger.c file
---
src/backend/commands/cmdtrigger.c | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/cmdtrigger.c b/src/backend/commands/cmdtrigger.c
index c6cda29..733c798 100644
--- a/src/backend/commands/cmdtrigger.c
+++ b/src/backend/commands/cmdtrigger.c
@@ -1,13 +1,12 @@
/*-------------------------------------------------------------------------
*
- * trigger.c
- * PostgreSQL TRIGGERs support code.
+ * cmdtrigger.c
+ * PostgreSQL COMMAND TRIGGER support code.
*
- * Portions Copyright (c) 1996-2011, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
+ * Portions Copyright (c) 2011, PostgreSQL Global Development Group
*
* IDENTIFICATION
- * src/backend/commands/trigger.c
+ * src/backend/commands/cmdtrigger.c
*
*-------------------------------------------------------------------------
*/
--
1.7.6.409.ge7a85.dirty
From 016aedc955ccd4cb2eb6cdead6957bf7cb58cee8 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
