On Thu, Nov 20, 2014 at 2:57 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Fixed in the attached version of the patch.
Thanks! Things are moving nicely for this patch. Patch compiles and
passes check-world. Some minor comments about the latest version:
1) Couldn't this paragraph be reworked?
<para>
+ The <literal>table_rewrite</> event occurs just before a table is going to
+ get rewritten by the commands <literal>ALTER TABLE</literal>. While other
+ control statements are available to rewrite a table,
+ like <literal>CLUSTER</literal> and <literal>VACUUM</literal>,
+ the <literal>table_rewrite</> event is currently only triggered by
+ the <literal>ALTER TABLE</literal> command, which might or might not need
+ to rewrite the table.
+ </para>
CLUSTER and VACUUM are not part of the supported commands anymore, so
I think that we could replace that by the addition of a reference
number in the cell of ALTER TABLE for the event table_rewrite and
write at the bottom of the table a description of how this event
behaves with ALTER TABLE. Note as well that "might or might not" is
not really helpful for the user.
2) The examples of SQL queries provided are still in lower case in the
docs, that's contrary to the rest of the docs where upper case is used
for reserved keywords.
+ <para>
+ Here's an example implementing such a policy.
+<programlisting>
+create or replace function no_rewrite()
+ returns event_trigger
+ language plpgsql as
[...]
3) This reference can be completely removed:
/*
* Otherwise, command should be CREATE, ALTER, or DROP.
+ * Or one of ANALYZE, CLUSTER, VACUUM.
*/
4) In those places as well CLUSTER and VACUUM should be removed:
+ else if (pg_strncasecmp(tag, "ANALYZE", 7) == 0 ||
+ pg_strncasecmp(tag, "CLUSTER", 7) == 0 ||
+ pg_strncasecmp(tag, "VACUUM", 6) == 0)
+ return EVENT_TRIGGER_COMMAND_TAG_OK;
else
And here:
+ if (pg_strcasecmp(tag, "ALTER TABLE") == 0 ||
+ pg_strcasecmp(tag, "CLUSTER") == 0 ||
+ pg_strcasecmp(tag, "VACUUM") == 0 ||
+ pg_strcasecmp(tag, "ANALYZE") == 0 )
+ return EVENT_TRIGGER_COMMAND_TAG_OK
I am noticing that the points raised by Alvaro previously are fixed.
Regards,
--
Michael