Re: New Event Trigger: table_rewrite

On Thu, Nov 20, 2014 at 10:37 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>>> 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.
>>
>> That's precisely why we have an event trigger here, I think --- for some
>> subcommands, it's not easy to determine whether a rewrite happens or
>> not. (I think SET TYPE is the one). I don't think we want to document
>> precisely under what condition a rewrite takes place.
>
> Yeah, the current documentation expands to the following sentence, as
> browsed in
>
> http://www.postgresql.org/docs/9.3/interactive/sql-altertable.html
>
> As an exception, if the USING clause does not change the column
> contents and the old type is either binary coercible to the new type
> or an unconstrained domain over the new type, a table rewrite is not
> needed, but any indexes on the affected columns must still be rebuilt.
>
> I don't think that "might or might not" is less helpful in the context
> of the Event Trigger, because the whole point is that the event is only
> triggered in case of a rewrite. Of course we could cross link the two
> paragraphs or something.
>
>>> 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.
>
> Right, being consistent trumps personal preferences, changed in the
> attached.
>
>> Yes please. <nitpick> Another thing in that sample code is "not current_hour
>> between 1 and 6". That reads strange to me. It should be equally
>> correct to spell it as "current_hour not between 1 and 6", which seems
>> more natural. </>
>
> True, fixed in the attached.
The status of this patch was not updated on the commit fest app, so I
lost track of it. Sorry for not answering earlier btw.

The following things to note about v5:
1) There are still mentions of VACUUM, ANALYZE and CLUSTER:
@@ -264,6 +275,10 @@ check_ddl_tag(const char *tag)
obtypename = tag + 6;
else if (pg_strncasecmp(tag, "DROP ", 5) == 0)
obtypename = tag + 5;
+ 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;
2) There are a couple of typos and incorrect styling, like "if(". Nothing huge..
Cleanup is done in the attached.

In any case, all the issues mentioned seem to have been addressed, so
switching this patch to ready for committer.
Regards,
--
Michael