Re: Event Triggers: adding information

On Wed, Jan 16, 2013 at 4:16 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Ok. Now I want to talk about our process a little. That's a 2 paragraphs
> diversion, after that it's getting back to technical matters.
>
> There's a difference between "it's not the way I would have done it" and
> "the author didn't think about what he's doing". That's also the reason
> why it's very hard to justify sending a polished enough patch as a non
> commiter.

There's no rule that anyone's got to agree with my opinion on
anything, but the idea that a patch should do one thing and do it well
is not a novel concept on this mailing list, however much you may feel
otherwise. This patch:

- adds ddl_command_trace and ddl_command_end events
- causes those events to be called not only for actual SQL commands
but also for things that happen, at present, to go through the same
code path
- adds additional magic variables to PL/pgsql to expose new
information not previously exposed
- adds CONTEXT as an additional event-trigger-filter variable, along with TAG

In my mind, that's four or five separate patches. You don't have to
agree, but that's what I think, and I'm not going to apologize for
thinking it. Moreover, I think you will struggle to find examples of
patches committed in the last three years that made as many different,
only loosely-related changes as you're proposing for this one.

As for the patch not being well-thought-out, I'm sticking by that,
too. Alvaro's comments about lock escalations should be enough to
tickle your alarm bells, but there are plenty of other problems here,
too.

> I think you might want to review the use case behing ddl_command_trace,
> that has been asked by who users wanted to see their use case supported
> in some easier way than just what you're talking about here.

That argument carries no water with me. You're asking every
PostgreSQL user in the universe to carry the overhead of a feature
that 90% of them will not use. That is mighty expensive syntactic
sugar. I am not talking about code-complexity, either. It is pretty
obvious that there is run-time overhead of having this feature. I am
not aware of any case where we have accepted run-time overhead for a
feature not mandated by the SQL standard. Given the rest of what you
have here, it is quite simple to arrange for the same function to be
called after a create or alter and before a drop. Being able to do
that in one command instead of two is not a sufficient reason to add
another event type.

>> This gets right back to an argument Dimitri and I have been having
>> since v1 of this patch, which is whether these are command triggers or
>> event triggers. I think we ought to support both, but they are not
>> the same thing. I think the design you are proposing is just fine for
>> an event called sql_drop, but it makes no sense for an event called
>> ddl_command_end, which needs to be called once per DDL statement - at
>> the end. Not once per object dropped.
>
> What I think you're missing here is the proposal flying around to have
> drop operation get back to ProcessUtility so that C hooks and event
> triggers both can have at it. It's been debated on the list earlier, way
> before we talked about event triggers, also as a way to clean up things,
> IIRC.

I'm very much opposed to that proposal, as I am to your proposal to
expose internal and generated events to users. Recursing into
ProcessUtility is a nasty, messy hook that is responsible for subtle
bugs and locking problems in our current DDL implementation. We
should be trying to refactor that to clean it up, not exposing it as a
user-visible detail. I do NOT want a future refactoring effort to
clean up race conditions and duplicate name lookups in the DDL code to
be blocked because someone is relying on the details of the existing
implementation in their event trigger implementation.

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