From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Command Triggers patch v18 |
Date: | 2012-03-27 01:11:21 |
Message-ID: | CA+TgmoZo=O=bfn5fuLHvNCHDU+PgKf8YrU69Ca3Gvx_iyzpY5w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Mar 20, 2012 at 1:49 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Greg Stark | 2012-03-27 02:17:36 | Re: Cross-backend signals and administration (Was: Re: pg_terminate_backend for same-role) |
Previous Message | Tom Lane | 2012-03-27 00:19:21 | Re: Cross-backend signals and administration (Was: Re: pg_terminate_backend for same-role) |