Re: Event Triggers reduced, v1

On Thu, Jul 5, 2012 at 5:31 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> [ new patch ]

Attached is a incremental patch with a bunch of minor cleanups,
including reverts of a few spurious white space changes. Could you
merge this into your version?

I have some concerns about pg_dump:

1. Can we spell out EvtTriggerInfo, getEvtTriggers, and dumpEvtTrigger
as EventTriggerInfo, getEventTriggers, and dumpEventTrigger?

2. I don't think that this code properly handles older server
versions. First, the version test in getEvtTriggers checks against
90200 instead of 90300. Second, when running against a too-old server
version, this is going to try to execute an empty query and then parse
the results. I think you want to restructure this code so that it
just plain old returns if the server is too old. See for example
getForeignDataWrappers.

3. The way you're generating evtfname is unsafe if either the schema
or the function name contains SQL characters. I think this should
probably be casting the function OID to regproc in lieu of rolling its
own formatting code. Also, the tags should probably be escaped using
quote_literal, just to be future-proof.

4. I think we should aim to generate all the SQL in upper case. Right
now "CREATE EVENT TRIGGER" and "EXECUTE PROCEDURE" are in upper case
but "when tag in" is in lower case.

In psql, I think we should similarly have listEventTriggers() rather
than listEvtTriggers(); here as in pg_dump I think you should cast the
evtfoid to regproc to get schema-qualification and escaping, in lieu
of the explicit join.

>> In terms of the schema itself, I think we are almost there, but:
>>
>> - I noticed while playing around with this that pg_dump emits a
>> completely empty owner field when dumping an event trigger. At first
>> I thought that was just an oversight, but then I realized there's a
>> deeper reason for it: pg_event_trigger doesn't have an owner field. I
>> think it should. The only other objects in the system that don't have
>> owners are text search parsers and text search templates (and casts,
>> sort of). It might seem redundant to have an owner even when
>> event-triggers are superuser-only, but we might want to try to relax
>> that restriction later. Note that foreign data wrappers, which are
>> also superuser-create-only, do have an owner. (Note that if we give
>> event triggers an owner, then we also need ALTER .. OWNER TO support
>> for them.)
>
> Damn, I had it on my TODO and Álvaro hinted me already, and I kept
> forgetting about it nonetheless. Fixed now.

evtowner seems to have missed the documentation bus.

With respect to the documentation, keep in mind that the synopsis is
going to show up in the command line help for \h. I'm thinking that
the full list of command tags is too long for that, and that we should
instead rearrange the page so that the list of supported commands is
outside the synopsis. The synposis is also somewhat misleading, I
think, in that "variable in (tag, ...)" conveys the idea that no
matter what the variable is, the items in parentheses will surely be
tags. I suggest that we say something like "filter_variable in
(filter_value, ...)" and then document in the text that
filter_variable must currently always be TAG, and that the legal
values for filter_value are dependent on the choice of
filter_variable, and the legal choices for TAG are those listed in the
following table: <splat>.

The documentation contains the following claim with which I'm
extremely uncomfortable:

+ <para>
+ Triggers on <literal>ANY</literal> command support more commands than
+ just this list, and will only provide the <literal>command
+ tag</literal> argument as <literal>NOT NULL</literal>. Supporting more
+ commands is made so that you can actually block <xref linkend="ddl">
+ commands in one go.
+ </para>

A minor issue is that there's no notion of ANY any more; it's just a
consequence of leaving out the WHEN clause. The bigger issue is that
I can't see any reason to do it that way. Surely if we're firing the
trigger at all, we can arrange to have the command tag properly filled
in so that we can filter by it.

This might be a crazy idea, but... it seems like it would be awfully
sweet if we could find a way to avoid having to translate between node
tags (i.e. constants beginning with T_* that are used to identify the
type of statement that is executing) and event tags (i.e. constants
beginning with E_* that are used to identify the type of statement
that is executing). Now obviously this is not quite possible because
in some cases the choice of E_* constant depends on not only the node
tag but also the type of object being operated on. However, it seems
like this is a surmountable obstacle: since we no longer need to store
the E_* constants in a system catalog, they don't really need to be
integers. For example, we could define something like this:

typedef struct
{
NodeTag nodetag;
ObjectType objecttype;
} NodeTagWithObjectType;

...and set the objecttype to a new OBJECT_DUMMY value or somesuch when
the NodeTag is such that the ObjectType isn't relevant. If we did
that, then all the E_* constants could go away, and InitEventContext()
would become a lot simpler and, presumably, faster. It would just
need to check whether it's got one of the node tags that needs the
object-type filled in. If so, it does that; if not, it just sets the
nodetag field to the statement's node-tag and the objecttype to
OBJECT_DUMMY, and it's done. Well, OK, it's probably not quite that
simple... but it still seems like we'd need explicit handling of a
smaller number of cases than presently.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company