On Tue, Mar 20, 2012 at 1:49 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:
> Hi,
>
> I guess I sent v17 a little early considering that we now already have
> v18 including support for CREATE TABLE AS and SELECT INTO, thanks to the
> work of Andres and Tom.
>
> There was some spurious tags in the sgml files in v17 that I did clean
> up too.
There are spurious whitespace changes in the documentation. Some of
these are of the following form:
- return_arr
+ return_arr
create_trigger.sgml adds a stray blank line as well.
I think that the syntax for enabling or disabling a command trigger
should not use the keyword SET. So just:
ALTER COMMAND TRIGGER name ENABLE [ ALWAYS | REPLICA ];
ALTER COMMAND TRIGGER name DISABLE;
That way is more parallel with the existing syntax for ordinary triggers.
+ The name to give the new trigger. This must be distinct from the name
+ of any other trigger for the same table. The name cannot be
+ schema-qualified.
"For the same table" is a copy-and-pasteo.
- /* Look up object address. */
+ /* Look up object address. */
Spurious diff.
I think that get_object_address_cmdtrigger should be turned into a
case within get_object_address_unqualified; I can't see a reason for
it to have its own function.
+ elog(ERROR, "could not find tuple for command trigger
%u", trigOid);
The standard phrasing is "cache lookup failed for command trigger %u".
+/*
+ * Functions to execute the command triggers.
+ *
+ * We call the functions that matches the command triggers definitions in
+ * alphabetical order, and give them those arguments:
+ *
+ * command tag, text
+ * objectId, oid
+ * schemaname, text
+ * objectname, text
+ *
+ */
This will not survive pgindent, unless you surround it with ------ and
-----. More generally, it would be nice if you could pgindent the
whole patch and then fix anything that breaks in such a way that it
will survive the next pgindent run.
+ * if (CommandFiresTriggers(cmd)
Missing paren.
+ /* before or instead of command trigger might have cancelled
the command */
Leftovers.
@@ -169,7 +169,7 @@ ExplainQuery(ExplainStmt *stmt, const char *queryString, ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("EXPLAIN option
BUFFERS
requires ANALYZE")));
-
+ /* if the timing was not set explicitly, set default value */ es.timing = (timing_set) ? es.timing :
es.analyze;
Spurious diff.
All the changes to ruleutils.c appear spurious. Ditto the change to
src/test/regress/sql/triggers.sql.
In the pg_dump support, it seems you're going to try to execute an
empty query if this is run against a pre-9.2 server. It seems like
you should just return, or something like that. What do we do in
comparable cases elsewhere?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
