Re: Event Triggers: adding information

Hi,

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Sorry for the delay - I'm looking at this now.

Thanks very much!

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

It's hard to devise exactly what kind of refactoring we need to do
before trying to write a patch that benefits from it, in my experience.
In some cases the refactoring will make things more complex, not less.

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

That's a good idea. I only started quite late in that patch to work on
the ObjectID piece of information, that's why I didn't split it before
doing so.

We might want to commit the other parts of the new infrastructure and
information first then get back on ObjectID and command string, what do
you think? Do you want me to prepare a patch that way, or would you
rather hack your way around the ObjectID APIs then I would rebase the
current patch?

Also, keep in mind we want the ObjectID in all CREATE, ALTER and DROP
statements, so my current patch is still some bricks shy of a load… (I
added ObjectID only in the commands I added rewrite support for, apart
from DROP).

> 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

I hear you. That feature is still required for any automatic consumption
of the command string because you want to resolve schema names, index
and constraint names, type name lookups and some more.

I think that this feature is also not an option for in-core logical
replication to support DDL at all. What alternative do you propose?

> 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

I had a more complete test suite last rounds and you did oppose to it
for good reasons. I'm ok to revisit that now, and I think the test case
should look like this:

- install an auditing command trigger that will insert any DDL command
string into a table with a sequence ordering

- select * from audit

- \d… of all objects created

- drop schema cascade

- DO $$ loop for sql in select command from audit do execute sql …

- \d… of all objects created again

Then any whacking of the grammar should alter the output here and any
case that's not supported will fail too.

We might be able to have a better way of testing the feature here, but I
tried to stay into the realms of what I know pg_regress able to do.

> 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

The way to solve that, I think, as Tom said, is to only rewrite the
command string when the objects exist in the catalogs. That's why we now
have ddl_command_trace which is an alias to either start or end
depending on whether we're doing a DROP or a CREATE or ALTER operation.

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

I'm not saying the cost of doing things that way are easy to swallow,
I'm saying that I don't see any other way to get that feature reliably
enough for in-core logical replication to just work with DDLs. Do you?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support