Re: Event Triggers reduced, v1

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 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?

Thank you very much for that, yes it's included now. So you have 3
attachments here, the whole new patch revision (v1.7), the incremental
patch to go from 1.6 to 1.7 and the incremental patch that should apply
cleanly on top of your cleanups.

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

Done.

> 2. I don't think that this code properly handles older server
> versions.

Fixed.

> 3. The way you're generating evtfname is unsafe if either the schema

Fixed using regproc cast, both in pg_dump and in psql.

> 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.

Oh, sure, done.

> 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.

Done.

> evtowner seems to have missed the documentation bus.

I'm told it finally did catch it.

> 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>.

I tried to arrange something here, I'm not quite sure about the current
result but it's already much better than the previous version.
Articulating ideas that way also allows to begin that command / events
support matrix, as you can see in the attached.

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

[…ANY command set…]

It's a vestige from a long time ago, I removed that and some other
verbiage now.

> 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.

Indeed, command tag is always available. The magic trigger variables
might not all be, though, that was what this text was alluding to, but
that case is already covered in the specific PL docs.

The part that we will certainly have to re-install later is when we add
new event "integration points" that we can only implement in some of the
supported commands, not all of them. Think about command_end and CLUSTER
or other commands managing the transaction themselves (concurrently).

But that's not at all relevant to what's in this reduced v1 patch.

> 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:

All of that happens in InitEventContext(). We can see in there that we
wouldn't gain much, the great majority of this code is dealing with
cases where we have no symmetry at all. Also I don't think that the
nodeTag macro is expensive, nor is a 2-levels nested switch statement.

So while I would enjoy seeing that part simplified, I don't think your
angle of attack would provide us much progress here…

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support