On Sat, Nov 8, 2014 at 1:05 PM, Andres Freund <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
