From: | Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Event Triggers reduced, v1 |
Date: | 2012-07-05 21:31:32 |
Message-ID: | m2a9zd51zf.fsf@2ndQuadrant.fr |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
Please find attached v6 of the patch, fixing all your comments here.
Sorry about missing some obvious things in the making of this patch.
Given that I had to merge master again, I can't easily produce an
incremental patch on top of last version, so you only have the full
patch attached.
Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> - You only changed the definition of pg_event_triggers.evttags, not
> the documentation. I don't think we need pg_evttag_to_string aka
> pg_event_trigger_command_to_string any more either.
Done. Removed now useless exposed functions.
> - When you removed format_type_be_without_namespace, you didn't remove
> either the function prototype or the related changes in format_type.c.
Fixed.
> - I'm pretty sure that a previous review mentioned the compiler
> warning in evtcache.c, which seems not to be fixed. It doesn't look
> harmless, either.
I'm failing to reproduce it here, in -O0. […Trying with the -O2
default…] I got the error in -O2, fixed in the attached.
> - The definitions of EVTG_FIRED_* are still present in
> pg_event_trigger.h, even though they're longer used anywhere.
Fixed.
> - The comments in parsenodes.h still refer to command triggers rather
> than event triggers. event_trigger.h contains a reference to
> CommandTriggerData that should say EventTriggerData. plperl.sgml has
> vestiges of the old terminology as well.
Fixed. I only found a single such vestige in plperl.sgml though.
> 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.
> - It seems pretty clear at this point that the cache you've
> implemented in src/backend/utils/cache/evtcache.c is going to do all
> the heavy lifting of converting the stored catalog representation to a
> form that is suitable for quick access. Given that, I wonder whether
> we should go whole hog and change evtevent to a text field. This
> would simplify things for pg_dump and we could get rid of
> pg_evtevent_to_string, at a cost of doing marginally more work when we
> rebuild the event cache (but who really cares, given that you're
> reading the entire table every time you rebuild the cache anyway?).
Agreed, done that way (using a NameData fixed width field).
> - In the \dy output, command_start is referred to as the "condition",
> but the documentation calls it an "event" and the grammar calls it
> "event_name". "event" or "event_name" seems better, so I think this
> is just a matter of changing psql to match.
Fixed.
> - AlterEventTrigStmt defines tgenbled as char *; I think it should
> just be char. In gram.y, you can declare the enable_trigger
> production as <chr> (or <ival>?) rather than <str>. At any rate
> pstrdup(stmt->tgenabled)[0] doesn't look right; you don't need to copy
> a string to fetch the first byte.
D'oh. Sure. Done.
> - Why did you create a separate file pg_event_trigger_fn.h instead of
> just including that single prototype in pg_event_trigger.h?
To mimic some existing files, fixed.
> - The EVENTTRIGGEROID syscache is used only in RemoveEventTriggerById.
> I don't think that's a sufficient justification for eating up more
> memory for another system cache. I think you should just remove that
> cache and recode RemoveEventTriggerById to find the correct tuple via
> an index scan. See RemoveEventTriggerById for an example.
It's now used in more places (dependencies, alter owner), so thinking
that it's pulling its weight now with 3 call sites I've left it alone.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment | Content-Type | Size |
---|---|---|
event_triggers_v1.6.patch.gz | application/octet-stream | 44.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dimitri Fontaine | 2012-07-05 21:52:12 | Re: Schema version management |
Previous Message | Peter Eisentraut | 2012-07-05 21:30:52 | Re: Re: [COMMITTERS] pgsql: Fix mapping of PostgreSQL encodings to Python encodings. |