Re: Event Triggers: adding information

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?

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?

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. 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. 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. I know that you feel that all of these are tolerable
costs, but I 100% disgaree. I predict that if we commit this, it will
becomes a never-ending and irrecoverable font of trouble.

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