Re: Add CREATE support to event triggers - Mailing list pgsql-hackers

On Sat, Nov 8, 2014 at 1:05 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> There's nothing to keep you from exposing the parse trees to
>> C functions that can live in an extension, and you can do all of this
>> deparsing there.
>
> Not really. There's some core functions that need to be touched. Like
> most of the stuff in patches 1,2,3,5,16 does.

Patch 1 is fine.  We've done similar things in the past (cf.
c504513f83a9ee8dce4a719746ca73102cae9f13,
82b1b213cad3a69cf5f3dfaa81687c14366960fc).  I'd just commit that.

Patch 2 adds support for GRANT and REVOKE to the event trigger
mechanism.  I wonder if it's a bad idea to make the
ddl_command_start/end events fire for DCL.  We discussed a lot of
these issues when this patch originally went in, and I think it'd be
worth revisiting that discussion.

Patch 3 is the same kind of idea as patch 2, only for COMMENT.

Patch 5 depends on patch 4, which does a bunch of things.  I *think*
the upshot of patch 5 is that we're not currently firing event
triggers in some situations where we should, in which case +1 for
fixing that.  It would help if there were a real commit message,
and/or some more contents, and I think it could be more completely
disentangled from patch 4.

Patch 16 again contains almost no comments and no description of its
specific purpose, but it appears to be similar to patch 1, so probably
mostly uncontroversial.

> We could just integrate those parts, and be done with it. But would that
> actually be a good thing for the community? Then slony needs to do it
> and potentially others as well? Then auditing can't use it? Then
> potential schema tracking solutions can't use it?

Do you think Slony is really going to use this?  I guess we can let
the Slony guys speak for themselves, but I've been skeptical since day
one that this is the best way to do DDL replication, and I still am.
There are lots of ways that a replicated DDL statement can fail on the
replicas, and what are you going to do then?  It's too late to show
the user the error message, so you can throw it in a log someplace and
hope that somebody notices, but that's it.  It makes a lot more sense
to me to use some kind of a tool that applies the DDL in a coordinated
fashion on all nodes - or even just do it manually, since it might
very well be desirable to take the lock on different nodes at widely
different times, separated by a switchover.  I certainly think there's
a use-case for what you're trying to do here, but I don't think it'll
be right for everyone.

Certainly, if the Slony guys - or some other team building an
out-of-core replication solutions says, hey, we really want this in
core, that would considerably strengthen the argument for putting it
there.  But I haven't heard anyone say that yet - unlike logical
decoding, were we did have other people expressing clear interest in
using it.

> There've been people for a long while asking about triggers on catalogs
> for that purpose. IIRC Jan was one of them.

My impression, based on something Christopher Brown said a few years
ago, is that Slony's DDL trigger needs are largely satisfied by the
existing event trigger stuff.  It would be helpful to get confirmation
as to whether that's the case.

>> On the flip side, the *cost* of pushing it into core is that
>> it now becomes the PostgreSQL's community problem to update the
>> deparsing code every time the grammar changes.  That cost-benefit
>> trade-off does not look favorable to me, especially given the absence
>> of any kind of robust testing framework.
>
> I agree that there should be testing in this.
>
> But I think you're quite overstating the effort of maintaining
> this. Looking at gram.y between the stamping of 9.3 devel and 9.4 devel
> for commits that'd require DDL deparsing changes:
> * ALTER TABLE .. ALTER CONSTRAINT: About 7 lines for the deparse code,
>   out of a ~350 line patch
> * REFRESH MATERIALIZED VIEW ... CONCURRENTLY: About 1 line for deparse,
>   out of ~700
> * WITH CHECK OPTION for views: About 3 lines for deparse, out of ~1300
> * REPLICA IDENTITY: About 24 lines for deparse, out of ~950
> * ALTER TABLESPACE MOVE: About 20 lines for deparse, out of
>   ~340. Although that patch was essentially scrapped afterwards. And
>   rewritten differently.
> * CREATE TABLESPACE ... WITH ...:  Not supported by event triggers right
>   now as it's a global object.
>
> That's really not a whole lot.

Those are pretty minor syntax patches, though.  I think it's more
helpful to look at things like the row-level security stuff, or
materialized views per se, or DDL support for collations.  In any
case, the additional coding burden concerns me less than the
additional testing burden - but there's another email further down
that's more to that specific point, so I'll stop here.

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