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

On 2014-11-08 17:49:30 -0500, Robert Haas wrote:
> 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.

Well, it doesn't generally support it for all GRANT statement, but just
for ones that are database local. I think that mostly skirts the
problems from the last round of discussion. But I only vaguely remember
them.

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

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

There was a fair amount noise about it at one of the past cluster
hackers thingies.

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

Well, I've yet to hear anything that's realistic otherwise.

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

Sure. And absolutely the same is true for DML. And the lack of DDL
integration makes it happen really rather frequently...

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

I agree that it's not the right.

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

As I said, there was clear interest at at least two of the cluster
hackers meetings...

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

Oh, that's contrary to what I remember, but yes, it'd be interesting to
hear about htat.

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

Sure, but these are all the ones from the stamping of 9.3devel to
9.4devel. I wanted to look at a release cycle, and that seemed the
easiest way to do it. I didn't choose that cycle, because I knew it was
"light" on ddl, I chose it because it was the last complete one.

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

I can't imagine that writing the relatively straight forward stuff that
deparsing requires is a noticeable part of a patch like RLS. The actual
coding in anything bigger really is the minor part - especially the
relatively trivial bits. And for prototyping you can just leave it out,
just as frequently done today for pg_dump (which usually is noticeably
more complex).
I can't imagine the MATERIALIZED VIEW stuff to be much different. The
CREATE MATERIALIZED VIEW stuff is basically CREATE VIEW with three extra
clauses. I really can't see that mattering that much in the course of a
4.5k line patch.

The same with collations. The first patch adding collations to
table/index columns, just needs to add print the collation name - that's
it. The patch for the collation DDL support needs to add CREATE
COLLATION (~27 lines, including newlines and function header). DROP is
handled generically. Most of ALTER is as well, and SET SCHEMA can't be
very hard.

Greetings,

Andres Freund