On Tue, Mar 20, 2012 at 1:49 PM, Dimitri Fontaine <dimi...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers