Re: Event Triggers: adding information

Hi,

On 2012-12-21 09:48:22 -0500, Robert Haas wrote:
> On Wed, Dec 12, 2012 at 4:47 PM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)fr> wrote:
> > The previously attached patch already needs a rebase since Tom fixed the
> > single user mode where you're not supposed to access potentially
> > corrupted system indexes. Please find attached a new version of the
> > patch that should applies cleanly to master (I came to trust git).
>
> Sorry for the delay - I'm looking at this now.
>
> My first observation (a.k.a. gripe) is this patch touches an awful lot
> of code all over the backend. We just did a big old refactoring to
> try to get all the rename and alter owner commands to go through the
> same code path, but if this patch is any indication it has not done us
> a whole lot of good. The whole idea of all that work is that when
> people wanted to make systematic changes to those operations (like
> involving sepgsql, or returning the OID) there would not be 47 places
> to change. Apparently, we aren't there yet. Maybe we need some more
> refactoring of that stuff before tackling this?

Its hard to do such refactorings without very concrete use-cases, so I
think its not unlikely that this patch points out some things that can
be committed independently.
ISTM that a good part of the "return oid;" changes are actually such a
part.

> Does anyone object to the idea of making every command that creates a
> new SQL object return the OID of an object, and similarly for RENAME /
> ALTER TO? If so, maybe we ought to go through and do those things
> first, as separate patches, and then rebase this over those changes
> for simplicity. I can probably do that real soon now, based on this
> patch, if there are no objections, but I don't want to do the work and
> then have someone say, ack, what have you done?

FWIW I am all for it.

> I'm basically implacably opposed to the ddl_rewrite.c part of this
> patch. I think that the burden of maintaining reverse-parsing code
> for all the DDL we support is an unacceptable level of maintenance
> overhead for this feature. It essentially means that every future
> change to gram.y that affects any of the supported commands will
> require a compensating change to ddl_rewrite.c. That is a whole lot
> of work and it is almost guaranteed that future patch authors will
> sometimes fail to do it correctly.

Thats - unsurprisingly - where I have a different opinion. ruleutils.c
does that for normal SQL and while DDL is a bit bigger in the grammar
than the DQL support I would say the data structures of of the latter
are far more complex than for DDL.
If you look at what changes were made to ruleutils.c in the last years
the rate of bugs isn't, as far as I can see, higher than in other areas
of the code.
The ruleutils.c precedent also means that patch authors *have* to be
aware of it already. Not too many features get introduced that only
change DDL.

> At an absolute bare minimum I
> think we would need some kind of comprehensive test suite for this
> functionality, as opposed to the less than 60 lines of new test cases
> this patch adds which completely fail to test this code in any way at
> all, but really I think we should just not have it.

Absolutely aggreed.

> It WILL get
> broken (if it isn't already) and it WILL slow down future development
> of SQL features. It also WILL have edge cases where it doesn't work
> properly, such as where the decision of the actual index name to use
> is only decided at execution time and cannot be inferred from the
> parse tree.

That obviousl needs to be solved, but I think the idea of invoking this
command trigger whenever its appropriate (which I think was actually
your idea?) should take care of most of the problems around this.

> I know that you feel that all of these are tolerable costs, but I 100%
> disgaree.

I think that missing DDL support is one of the major weaknesses of all
potential "logical" replication solutions. Be it slonly, londiste, BDR,
or even something what some day may be in core.
I just don't see any more realistic way to fix that besides supporting
something like this.

> I predict that if we commit this, it will
> becomes a never-ending and irrecoverable font of trouble.

Why? Its really not that different from whats done currently?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services