Re: Event Triggers reduced, v1

On Tue, Jul 3, 2012 at 11:52 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> in it. That's more or less what Dimitri already has in his latest
>> patch, except that after looking it over I'm inclined to think that
>> we'd be better off storing the keys as text and translating to
>> internal ID numbers when we read and cache the table, rather than
>> storing the ID numbers in the table. That will make the catalog
>> contents easier to read, and more importantly will mean that a change
>> to the internal ID numbers need not be initdb-forcing.
>
> Well that's what I had in previous versions of the patch, where the code
> was dealing directly with command tags. If we store text in catalogs and
> given that we already have the command tag as text, I suppose we would
> get back to only managing tags as text in the cache key and the rest of
> the code.

Yeah, I'm of two minds on that. I thought that it made sense to use
integer identifiers internally for speed, but now I'm worried that the
effort to translate back and forth between strings and integers is
going to end up being more than any speed we might save. But I'm
still not certain which way is best: maybe your original idea of using
strings everywhere will prove to be the winner, but on the other hand
I'm not convinced that the code as you have it written today is as
efficient as it could be.

At any rate, whether or not we end up using strings or integers inside
the backend-local caches, I'm inclined to think that it's better to
store strings in the catalog, so we'd end up with something like this:

CATALOG(pg_event_trigger,3466)
{
NameData evtname; /* trigger's name */
int16 evtevent; /* trigger's event */
Oid evtfoid; /* OID of function to be
char evtenabled; /* trigger's firing configuratio
* session_repli
#ifdef CATALOG_VARLEN
text evttags[1]; /* command TAGs this event trigger targe
#endif
}

If there's no filter on tags, then I think we should let evttags = NULL.

If in the future we have filtering that's not based on tags, we'd add,
e.g. "text evtshushiness[1]" if we were going to filter based on
smushiness level. We'd set evtsmushiness = NULL if there's no
filtering by smushiness, or else an array of the smushiness levels
that fire that trigger.

Does that work for you?

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