Event Triggers: adding information

Hi,

Please find attached v3 of the patch to add information to event
triggers, including command string. This work has mostly been part of
the original goal and its design is the result of much reviewing here,
in particular from Robert and Tom (rewriting a command before is done is
called magic, not code, and that's why we have ddl_command_trace now).
Many thanks.

I hope that Robert will be in a position to continue reviewing and
taking care of that patch series. He's clearly interested but really
busy these days, so if another commiter wants to see for himself, please
don't be shy!

The current patch provides the following features:

- New events: ddl_command_end and ddl_command_trace

- New information in the TG_VARIABLE:
object name, schema name, object id, object kind,
operation (CREATE|ALTER|DROP), context (GENERATED|TOPLEVEL|…)
normalized command string

- New event filtering on CONTEXT (see after sig for an example)

+ create event trigger regress_event_trigger_end on ddl_command_end
+ when context in ('toplevel', 'generated', 'query', 'subcommand')
+ execute procedure test_event_trigger();

- Documentation of listed features

- pg_dump support of the new features (context support)

The command string normalization is a necessary feature and takes most
of this patch footprint. The main use case of this feature is in-core
logical replication support, the main non-core users will be the usual
replication suspects (Slony, Londiste, Bucardo) and the audit systems.
We might be able to also support some advanced Extension capabilities if
we get "command diverting" support in next commit fest, and we could
already deprecate the extension whitelisting code I had to do for 9.1
and 9.2.

As we need to support all of CREATE, ATLER and DROP cases, I can't see
another way than to work from a very loose notion of the parse tree. Up
to now I've seen 3 cases worth reporting:

- simple commands, where you can rely on the parsetree as is, and that
need no transform stage (create extension, drop object, alter
sequence, text search *, most commands really)

- medium complex commands, which transform the parsetree in some ways
but for which it still is easy to use it directly if you take care
of only using the transformed parse tree (create table, create
domain, create view — oh, the work was already done)

- complex commands where you need to hack them to return the actual
parse tree like structure they prepared out of the transformed parse
tree so that you can get to kmow what they've just done, and that's
only ALTER TABLE really, up to now

So my guess is that it will get real easier from now, and I will add
some more de parsing support with stats to have some ideas about that.

I've been told that the maintenance cost is going to be so great as to
limit our abilities to continue adding features to existing DDLs and
coming up with new ones. I have two answers: first, I don't suppose
we're going to include logical replication without DDL support in core
and I don't see another realistic way to implement it, second, let's
have some data to think about the case at hand.

For example, I added CREATE SCHEMA objectid and command normalisation
and here's the footprint:

git diff --stat
src/backend/commands/schemacmds.c | 6 ++++--
src/backend/tcop/utility.c | 4 ++--
src/backend/utils/adt/ddl_rewrite.c | 31 +++++++++++++++++++++++++++++++
src/include/commands/schemacmds.h | 2 +-
4 files changed, 38 insertions(+), 5 deletions(-)

For the whole DefineStmt command set (CREATE AGGREGATE, OPERATOR, TYPE,
TEXT SEARCH *, COLLATION), we're talking about:

11 files changed, 186 insertions(+), 70 deletions(-)
95 lines of those in ddl_rewrite.c

For the normalisation of CREATE CONVERSATION command string, this time
it's

4 files changed, 40 insertions(+), 5 deletions(-)

And CREATE DOMAIN, which is more involved as you need to take care of
rewriting DEFAULT and CHECK constraints, we have

4 files changed, 163 insertions(+), 3 deletions(-)

The other commands I need to be working on if we want more data are
ALTER kind of commands, and I've already worked on the most complex one
of those, namely ALTER TABLE. I don't expect most DROP commands to offer
any difficulty here, as I already have support for DropStmt.

The question for DropStmt is what to do about object specific
information when a single command can have as many targets as you want
to? Current patch is filling-in information for the "main" target which
would be the first object in the sequence, but that's a very weak
position and I would easily agree to just leave the fields NULL in that
case.

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

A mildly interesting example that you can have now:

create schema baz
authorization dim
create table distributors
(did serial primary key,
name varchar(40),
f2 text check (upper(f2) = f2),
unique(name) with (fillfactor=70)
)
with (fillfactor=70);

I added the blank lines to help the reader in that console paste:

NOTICE: snitch event: ddl_command_end, context: GENERATED, tag: CREATE SEQUENCE, operation: CREATE, type: SEQUENCE
NOTICE: oid: 41633, schema: baz, name: distributors_did_seq
NOTICE: command: CREATE SEQUENCE baz.distributors_did_seq;

NOTICE: snitch event: ddl_command_end, context: SUBCOMMAND, tag: CREATE TABLE, operation: CREATE, type: TABLE
NOTICE: oid: 41635, schema: baz, name: distributors
NOTICE: command: CREATE TABLE baz.distributors (did integer, name pg_catalog.varchar, f2 text, CHECK ((upper(f2) = f2))) WITH (fillfactor=70);

NOTICE: snitch event: ddl_command_end, context: GENERATED, tag: CREATE INDEX, operation: CREATE, type: INDEX
NOTICE: oid: 41643, schema: baz, name: distributors_pkey
NOTICE: command: CREATE UNIQUE INDEX distributors_pkey ON baz.distributors USING btree (did);

NOTICE: snitch event: ddl_command_end, context: GENERATED, tag: CREATE INDEX, operation: CREATE, type: INDEX
NOTICE: oid: 41645, schema: baz, name: distributors_name_key
NOTICE: command: CREATE UNIQUE INDEX distributors_name_key ON baz.distributors USING btree (name) WITH (fillfactor=70);

NOTICE: snitch event: ddl_command_end, context: GENERATED, tag: ALTER SEQUENCE, operation: ALTER, type: SEQUENCE
NOTICE: oid: 41633, schema: baz, name: distributors_did_seq
NOTICE: command: ALTER SEQUENCE baz.distributors_did_seq OWNED BY baz.distributors.did;

NOTICE: snitch event: ddl_command_end, context: TOPLEVEL, tag: CREATE SCHEMA, operation: CREATE, type: SCHEMA
NOTICE: oid: 41632, schema: <NULL>, name: baz
NOTICE: command: CREATE SCHEMA baz AUTHORIZATION dim;
CREATE SCHEMA

Hi again,

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> The current patch provides the following features:
>
> - New events: ddl_command_end and ddl_command_trace
>
> - New information in the TG_VARIABLE:
> object name, schema name, object id, object kind,
> operation (CREATE|ALTER|DROP), context (GENERATED|TOPLEVEL|…)
> normalized command string
>
> - New event filtering on CONTEXT (see after sig for an example)
>
> + create event trigger regress_event_trigger_end on ddl_command_end
> + when context in ('toplevel', 'generated', 'query', 'subcommand')
> + execute procedure test_event_trigger();
>
> - Documentation of listed features
>
> - pg_dump support of the new features (context support)

Still the same, plus some more command rewriting support so that I have
some more data points to offer about the code maintainance burden we all
want to limit on that feature.

> For example, I added CREATE SCHEMA objectid and command normalisation
> and here's the footprint:
> 4 files changed, 38 insertions(+), 5 deletions(-)
>
> For the whole DefineStmt command set (CREATE AGGREGATE, OPERATOR, TYPE,
> TEXT SEARCH *, COLLATION), we're talking about:
> 11 files changed, 186 insertions(+), 70 deletions(-)
> 95 lines of those in ddl_rewrite.c
>
> For the normalisation of CREATE CONVERSATION command string, this time
> 4 files changed, 40 insertions(+), 5 deletions(-)
>
> And CREATE DOMAIN, which is more involved as you need to take care of
> rewriting DEFAULT and CHECK constraints, we have
> 4 files changed, 163 insertions(+), 3 deletions(-)

The whole set of RenameStmt commands:
32 files changed, 382 insertions(+), 119 deletions(-)

Then after that, sharing a lot of the previous code, support for both
AlterOwnerStmt and AlterObjectSchemaStmt rewriting:
1 file changed, 102 insertions(+), 17 deletions(-)

As you can guess I separated away the EventTriggerTargetOid tracking:
20 files changed, 96 insertions(+), 57 deletions(-)

Again, those data point are not commented here, it's only to get some
better idea of what we are talking about as the cost of maintaining the
new command normalisation feature that's brewing in this patch.

I would like that we commit to that idea and the current implementation
of it, and include the set of commands I already had time to prepare in
the current submission, knowing that I will continue adding commands
support up to the next commitfest with the goal to have a full coverage.

I also will be working on more features (complete PL support, some kind
of command diverting, a "table_rewrite" event, some ready to use event
triggers functions).

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

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

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

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

On Fri, Dec 21, 2012 at 11:35 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> 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.

Perhaps. I have a feeling there's some more simplification that can
be done here, somehow.

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

That's fine. I've committed that part. I think the remainder of the
patch is really several different features that ought to be split
apart and considered independently. We may want some but not others,
or some may be ready to go in but not others.

> 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 shall rely on you to provide those bricks which are still missing.

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

I was thinking that we might need to go beyond what pg_regress can do
in this case. For example, one idea would be to install an audit
trigger that records all the DDL that happens during the regression
tests. Then, afterwards, replay that DDL into a new database. Then
do a schema-only dump of the old and new databases and diff the dump
files. That's a little wacky by current community standards but FWIW
EDB has a bunch of internal tests that we run to check our proprietary
stuff; some of them work along these lines and it's pretty effective
at shaking out bugs.

In this particular case, it would significantly reduce the maintenance
burden of putting a proper test framework in place, because we can
hope that the regression tests create (and leave around) at least one
object of any given type, and that any new DDL features will be
accompanied by similar regression tests. We already rely on the
regression database for pg_upgrade testing, so if it's not complete,
it impacts pg_upgrade test coverage as well.

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

Hmm. I have to study that more, maybe. I certainly agree that if you
can look at the catalogs, you should be able to reliably reconstruct
what happened - which isn't possible just looking at the parse tree.
However, it feels weird to me to do something that's partly based on
the parse tree and partly based on the catalog contents. Moreover,
the current pre-create hook isn't the right place to gather
information from the catalogs anyway as it happens before locks have
been taken.

Now, there's another thing that is bothering me here, too. How
complete is the support you've implemented in this patch? Does it
cover ALL CREATE/ALTER/DROP commands, including all options to and all
variants of those commands? Because while I'm not very sanguine about
doing this at all, it somehow feels like doing it only halfway would
be even worse.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> 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.
>
> That's fine. I've committed that part. I think the remainder of the
> patch is really several different features that ought to be split
> apart and considered independently. We may want some but not others,
> or some may be ready to go in but not others.

Thank you for this partial commit, and Simon and Andres to fill in the
gaps. I should mention that the missing header parts were all in my
patch, and that headers hacking is proving suprisingly uneasy.

I've been having several rounds of problems with the gcc tool chain when
modifying headers. Worst case has been a successful compiling followed
by random errors. Then gdb was not giving any clue (botched memory when
returned from a function, palloc'ed memory not freed yet). After
spending more time on it that I care to admit, I did a `make
maintainer-clean`, built again and the problem disappeared all by
itself.

The gcc tool chain is not my favorite developer environment.

>> 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 shall rely on you to provide those bricks which are still missing.

Cool, will prepare a separate patch implementing that API now, and base
the following patches on top of that. My thinking is to do as following:

- API patch to get Oid from all CREATE/ALTER commands
- API patch for getting the Oid(s) in (before) DROP commands
- Event Trigger Infos patch, depending on the previous ones
- Command String Rewrite and its publishing in Event Triggers

Of course for the DROP parts, we'd better have the Oid and use it before
the command runs through completion or it will not be usable from the
event trigger (the target is ddl_command_start in that case).

>> 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.
>
> I was thinking that we might need to go beyond what pg_regress can do
> in this case. For example, one idea would be to install an audit
> trigger that records all the DDL that happens during the regression
> tests. Then, afterwards, replay that DDL into a new database. Then
> do a schema-only dump of the old and new databases and diff the dump
> files. That's a little wacky by current community standards but FWIW
> EDB has a bunch of internal tests that we run to check our proprietary
> stuff; some of them work along these lines and it's pretty effective
> at shaking out bugs.

Are you in a position to contribute those parts to the community?
Implementing them again then having to support two different variants of
them does not look like the best use of both our times.

> Hmm. I have to study that more, maybe. I certainly agree that if you
> can look at the catalogs, you should be able to reliably reconstruct
> what happened - which isn't possible just looking at the parse tree.

Well in fact we're looking at the parsetree once edited to host the
exact data that goes into the catalogs. In most cases that data is
easily available to further consumption, or it's possible to use the
transform API without touching catalogs again.

In some places I'm doing the same processing twice, which I don't like
very much, but it's only happening if an Event Trigger is going to fire
so I though we could optimize that later. The aim here has been to limit
the changes to review, it looks like we have enough of them already.

Concerned parts are dealing with raw and cooked expressions for CHECK
and DEFAULT and WHERE clauses (create table, create index, alter table,
create constraint, create domain).

> However, it feels weird to me to do something that's partly based on
> the parse tree and partly based on the catalog contents. Moreover,
> the current pre-create hook isn't the right place to gather
> information from the catalogs anyway as it happens before locks have
> been taken.

So all the information is derived from the parse tree. The trick is that
this information is only valid once it has hit the catalogs (think of a
CHECK constraint in a CREATE TABLE statement, referencing some column
attribute, same with a DEFAULT expression depending on another col).

The case where we should certainly prefer looking at the catalog caches
are when we want to know the actual schema where the object has been
created. The current patch is deriving that information mostly from the
parse tree, using the first entry of the search_path when the schema is
not given in the command. It's ok because we are still in the same
transaction and no command has been able to run in between the user
command and our lookup, but I could easily get convinced to look up the
catalogs instead, even more so once we have the OID of the object easily
available in all places.

> Now, there's another thing that is bothering me here, too. How
> complete is the support you've implemented in this patch? Does it
> cover ALL CREATE/ALTER/DROP commands, including all options to and all
> variants of those commands? Because while I'm not very sanguine about
> doing this at all, it somehow feels like doing it only halfway would
> be even worse.

This patch is implementing complete support for all commands for most of
the information, and partial support for some information that comes
from the parse tree analysis. The goal for this commit fest is to agree
on how to do it, then finish the whole command set for the next one.

As decided at the hackers meeting, now is the time to address anything
controversial, once done we can "return with feedback". So that's my
goal for this commit fest, concerning the Normalized Command String.

Given your review, I think the way forward is pretty clear now, and my
plan is as following:

- commit Oid related API changes in this commit fest
- commit Event Trigger Information changes in this commit fest
- agree on how to tackle exposing the normalized command string

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> 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 shall rely on you to provide those bricks which are still missing.

Please find attached a patch to change most functions called from
standard_ProcessUtility() to return an Oid (passes `make
maintainer-clean; configure; make install check`). Most of them only,
because it only make sense for functions touching an object that exists
in the catalogs and have a distinct Oid.

That completes ALTER and CREATE ObjectID support, I did nothing about
the DROP case in the attached. The way I intend to solve that problem is
using get_object_address() and do an extra lookup from within the Event
Trigger code path.

Yes that's an extra lookup, the only alternative that I see would be
another giant refactoring so that all and any DROP support function is
split in a lookup and lock part first, then the actual DROP. I don't see
that as a particular win either, as the advantage of doing a separate
(extra) lookup in the ddl_command_start Event Trigger is that the actual
DROP code then will check that the target still exists.

The attached patch is known to miss support for ExecCreateTableAs
because I wanted to keep the ball rolling and couldn't figure out where
to find the target relation id in time. If you feel like filling that
gap that would be awesome, if not I will take care of that later, either
in a version v1 of that patch, or in a follow-up patch, or embedded into
the next patch to come, the cleaned up Event Trigger Info patch without
Normalized Command String support. As you see fit.

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

Hi,

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Cool, will prepare a separate patch implementing that API now, and base
> the following patches on top of that. My thinking is to do as following:
>
> - API patch to get Oid from all CREATE/ALTER commands
> - API patch for getting the Oid(s) in (before) DROP commands
> - Event Trigger Infos patch, depending on the previous ones
> - Command String Rewrite and its publishing in Event Triggers
>
> Of course for the DROP parts, we'd better have the Oid and use it before
> the command runs through completion or it will not be usable from the
> event trigger (the target is ddl_command_start in that case).

Please find attached two versions of the same patch implementing added
information for Event Triggers, without any support for command strings
normalizing.

- event_trigger_infos.5.patch.gz
applies on top of current's master branch

- event_trigger_infos_partial.5.patch.gz
applies on top of oid-api.0.patch.gz applied to current's master
branch

I guess you're going to use the partial one, but if you want to run a
quick testing the aggregated patch might make sense.

The way I've been adding schema and object names is to do extra lookups
to the system caches (or the catalogs when we don't have caches), so
that the information is know to be correct in all cases. That means I
had to add a CommandCounterIncrement(), and that the information is only
available when the object actually exists (use ddl_command_trace).

Also, in the DROP case, the extra lookup that happens before the DROP
command is done with a ShareUpdateExclusiveLock, so that the object
doesn't disappear from concurrent activity while we run the User Defined
Trigger. Should we be concerned about deadlock possibilities here when
later the transaction will need to upgrade its Lock?

Given lock upgrade concerns, I guess there's not so much we can do apart
from taking an AccessExclusiveLock from the get go in the case of a DROP
Event Trigger at ddl_command_start, right?

> Are you in a position to contribute those parts to the community?
> Implementing them again then having to support two different variants of
> them does not look like the best use of both our times.

I didn't include new testing in that patch version, waiting for your
answer.

> As decided at the hackers meeting, now is the time to address anything
> controversial, once done we can "return with feedback". So that's my
> goal for this commit fest, concerning the Normalized Command String.
>
> Given your review, I think the way forward is pretty clear now, and my
> plan is as following:
>
> - commit Oid related API changes in this commit fest
> - commit Event Trigger Information changes in this commit fest
> - agree on how to tackle exposing the normalized command string

Including this email now, the first two items are getting addressed
(pending review then commit, at least that's the goal here). Let's not
forget the third point: we need to reach an agreement on how to address
the Command String Normalisation, too.

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

On 21 December 2012 14:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

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

Event triggers are a new feature with many powerful uses: replication,
online upgrade, data transport.

New features require extra code. To decide how big a maintenance
burden, we need to look at whether we want the feature, and if we have
it, how much code it would be to maintain it.

pg_dump needs to dump what is there, so dumping "current state"
information is required. That can be accessed from catalog, but
requires significant extra code to bring that all back together into
DDL text. pg_dump is useful for schema backups and pg_upgrade. This
code already exists in core.

Replication needs to see "delta" information, so we can track changes
as they happen. Trying to deduce exact deltas by diffing two "current
state" dumps is close to impossible. The only way is a "delta" stream.
This would be used by replication and on-line upgrade. This code can
be added to core by the patches under discussion.

The code is broadly comparable between "current state" and "delta".
It's basically just assembling the text of DDL statements from
available information. In one case from the catalog, in the other case
from the command parse/execution trees. It's not especially hard to
code, nor will it be fiddly or difficult to understand - just
developing normalised text versions of the change in progress.
Normalisation is important and useful; we spent time in 9.2 doing
things the right way for pg_stat_statements.

In both cases, the code really should live in core. So we can tie the
code directly to each release. Realistically, doing things outside
core means there are often holes or problems that cannot be easily
solved. That seems almost certain to be the case here.

pg_dump maintenance is required but its not a huge burden. pg_dump
hasn't slowed down the addition of new DDL features. It just requires
a little extra code, mostly very simple.

The PostgreSQL project needs rewrite support for DDL, so it will come
to exist inside, or outside core. Knowing it is there, being able to
rely on that, means we'll make faster progress towards the new
features we are building. The approach proposed here has been
discussed over many years, minuted in detail at cluster hackers
meetings: "DDL Triggers".

My thoughts are that the approach here has been well thought through
and is exactly what we need. It *is* extra code, but not complex code,
and it is code designed to bring us closer to our goals, not to hamper
the development of new DDL statements. With the right tests, the
changes required would be easy to spot and add code for them. It looks
to me that we already have a good suggestion on exactly how to do that
(from Robert).

So overall, it certainly is worth thinking hard about whether we
should include the rewrite code. Balancing the costs and the benefits,
I think we should include.

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

On Tue, Dec 25, 2012 at 10:34 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Thank you for this partial commit, and Simon and Andres to fill in the
> gaps. I should mention that the missing header parts were all in my
> patch, and that headers hacking is proving suprisingly uneasy.

Apologies to all about the missed bits. I believe that I had them in
my version of the patch as well, but I think the changes ended up
unstaged in my repository rather than folded into the patch. I'm
usually smarter than that, but, obviously, not this time.

>> I was thinking that we might need to go beyond what pg_regress can do
>> in this case. For example, one idea would be to install an audit
>> trigger that records all the DDL that happens during the regression
>> tests. Then, afterwards, replay that DDL into a new database. Then
>> do a schema-only dump of the old and new databases and diff the dump
>> files. That's a little wacky by current community standards but FWIW
>> EDB has a bunch of internal tests that we run to check our proprietary
>> stuff; some of them work along these lines and it's pretty effective
>> at shaking out bugs.
>
> Are you in a position to contribute those parts to the community?
> Implementing them again then having to support two different variants of
> them does not look like the best use of both our times.

If I thought there were some useful code, I would try to see if we
could contribute it, but I'm pretty sure there isn't. We have a bunch
of pg_dump tests, but their intended purpose is to verify that our
proprietary stuff dumps and reloads properly, which is not what we
need here. The reason I mentioned it was just to make the point that
it is possible to have a large suite of tests that goes beyond what is
possible with pg_regress, and that this can be a useful way of finding
out whether you've broken things. In our case, we actually cheat
pretty hard by using things like \! pg_dump in the .sql files, so it
actually ends up going through pg_regress after all, technically
anyway. I'm not quite sure what to think about that approach from a
community point of view. It certainly ends up reducing the number of
new things that you need to invent in order to do new kinds of
testing, but it's still not as flexible as I would like - for example,
I don't see a way to do the exact kind of testing we need here in a
cross-platform way using that infrastructure.

I'm tempted to propose that we define a Perl-based testing API for
"additional regression tests" that can't be handled using pg_regress.
Like: we put a bunch of Perl modules in a certain directory, and then
write a script that goes through and do

require $module;
$module->regress;

...on each one, based on some schedule file. If a test passes or
fails, it calls some API that we provide to report the result. That
way, whatever tests we write for this effort can potentially become
the core of a larger test suite, and by basing it on Perl we add no
new tool dependencies and can (hopefully) run on both Linux and
Windows. I'm open up to other ideas, of course.

> The case where we should certainly prefer looking at the catalog caches
> are when we want to know the actual schema where the object has been
> created. The current patch is deriving that information mostly from the
> parse tree, using the first entry of the search_path when the schema is
> not given in the command. It's ok because we are still in the same
> transaction and no command has been able to run in between the user
> command and our lookup, but I could easily get convinced to look up the
> catalogs instead, even more so once we have the OID of the object easily
> available in all places.

I haven't looked at the code, but it seems to me that there have got
to be edge cases where that will fail. For one thing, we only do
pretty minimal locking on namespaces, so I'm not at all sure that
someone couldn't (for example) rename the old namespace out of the
way, thus causing one search_path lookup to find the first schema in
the search_path and the other lookup to find the second schema in the
search_path. Frankly, I think we have some hazards of that type in
the DDL code as it stands, so it's not like nobody's ever made that
mistake before, but it would be nice not to propagate it.

Really, the only safe way to do these things is to have any given name
lookup done in one and only one place. We're some distance from there
and there and I don't know that this patch should be required to carry
the burden of nailing down all the loose ends in that area, but I
think it's reasonable to insist that it shouldn't do anything which
makes it impossible for someone else to nail down those loose ends,
which is still fairly high on my personal to-do list.

The other danger here is - what exactly do you mean by "no command has
been able to run between the user command and our lookup"? Because
you can do stupid things with functions like set_config(). This would
only be safe if no user-provided expressions can possibly get
evaluated between point A and point B, and that strikes me as the sort
of thing that could easily be false unless this is all done VERY close
to the start of processing.

A belated Merry Christmas to you as well.

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

On Tue, Dec 25, 2012 at 1:42 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> 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 shall rely on you to provide those bricks which are still missing.
>
> Please find attached a patch to change most functions called from
> standard_ProcessUtility() to return an Oid (passes `make
> maintainer-clean; configure; make install check`). Most of them only,
> because it only make sense for functions touching an object that exists
> in the catalogs and have a distinct Oid.

OK, I committed this.

> That completes ALTER and CREATE ObjectID support, I did nothing about
> the DROP case in the attached. The way I intend to solve that problem is
> using get_object_address() and do an extra lookup from within the Event
> Trigger code path.

An extra lookup that occurs always, or only when event triggers are in
use? A re-resolution of the name, or some other kind of lookup?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> If I thought there were some useful code, I would try to see if we
> could contribute it, but I'm pretty sure there isn't. We have a bunch

Oh. Too bad.

[... description of the tool ...]
> I don't see a way to do the exact kind of testing we need here in a
> cross-platform way using that infrastructure.

Well, do we have \! in psql in windows at all? does it work in a similar
way to the unix one? I just don't know.

> I'm tempted to propose that we define a Perl-based testing API for
> "additional regression tests" that can't be handled using pg_regress.

I know that our build requirements are pretty stable for very good
reasons and I appreciate that. Now, I don't do perl. I'll happily use
whatever you come up with, though.

>> command and our lookup, but I could easily get convinced to look up the
>> catalogs instead, even more so once we have the OID of the object easily
>> available in all places.
>
> I haven't looked at the code, but it seems to me that there have got
> to be edge cases where that will fail. For one thing, we only do

In between that email and the latest patch I've removed any processing
of the parse tree, because without the ddl_rewrite module it didn't make
much sense anyway, and I still want to be sure we have ObjectID, Name
and Schema as a base information set.

> Really, the only safe way to do these things is to have any given name
> lookup done in one and only one place. We're some distance from there
> and there and I don't know that this patch should be required to carry
> the burden of nailing down all the loose ends in that area, but I
> think it's reasonable to insist that it shouldn't do anything which
> makes it impossible for someone else to nail down those loose ends,
> which is still fairly high on my personal to-do list.

The current version of the patch now does its own lookup, and only does
so at a time when the object exists in the catalogs. So I think we're
now good on that front in the current Event Triggers patch, even if it's
not refactoring all and any DDL to be able to add hooks in between
lookups and locks, for example.

Also, several cases appear to need an extra lookup anyway, such as ALTER
OBJECT … RENAME TO … or ALTER OBJECT … SET SCHEMA … when you want to
feed information to a ddl_command_end Event Trigger…

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> OK, I committed this.

Thanks. Please find attached a rebased patch, v6. I think it's looking
more like what you would expect now:

COLUMNS=70 git diff --stat postgres/master..
doc/src/sgml/event-trigger.sgml | 196 +++-
doc/src/sgml/plpgsql.sgml | 95 +-
doc/src/sgml/ref/create_event_trigger.sgml | 20 +-
src/backend/catalog/objectaddress.c | 22 +-
src/backend/commands/event_trigger.c | 870 ++++++++++++++++-
src/backend/commands/trigger.c | 39 +
src/backend/commands/typecmds.c | 2 +-
src/backend/tcop/utility.c | 385 +++++---
src/backend/utils/cache/evtcache.c | 15 +
src/bin/pg_dump/pg_dump.c | 21 +-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/psql/describe.c | 9 +-
src/include/catalog/objectaddress.h | 21 +
src/include/catalog/pg_event_trigger.h | 2 +
src/include/commands/event_trigger.h | 47 +-
src/include/commands/trigger.h | 1 +
src/include/utils/evtcache.h | 6 +-
src/pl/plpgsql/src/pl_comp.c | 48 +
src/pl/plpgsql/src/pl_exec.c | 57 +-
src/pl/plpgsql/src/plpgsql.h | 6 +
src/test/regress/expected/event_trigger.out | 53 +-
src/test/regress/sql/event_trigger.sql | 51 +-
22 files changed, 1700 insertions(+), 267 deletions(-)

>> That completes ALTER and CREATE ObjectID support, I did nothing about
>> the DROP case in the attached. The way I intend to solve that problem is
>> using get_object_address() and do an extra lookup from within the Event
>> Trigger code path.
>
> An extra lookup that occurs always, or only when event triggers are in
> use? A re-resolution of the name, or some other kind of lookup?

Only when event triggers are in use, and when the object is known to
exists in the catalogs (ddl_command_start for a DROP operation, or
ddl_command_end for a CREATE or ALTER operation). And it's only a name
resolution using the catcache when it exists, or a catalog index scan
when we have to. The key we have is the OID.

The OID is filled in from utility.c in all CREATE and ALTER operations
that we are interested into, and in the case of a DROP operation we do
either a RangeVarGetRelid for relations or get_object_address() for the
other object kinds.

Given the OID, we use the ObjectProperty[] structure to know which cache
or catalog to scan in order to get the name and namespace of the object.

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

On 12/29/2012 08:41 AM, Dimitri Fontaine wrote:
> Well, do we have \! in psql in windows at all? does it work in a similar
> way to the unix one? I just don't know.
>
>

Of course we do, Why ever would you think we don't? The command must be
written in the Windows shell language, but the behaviour is the same. If
system() and popen() didn't work on Windows we'd be in serious trouble
having a Windows port at all.

cheers

andrew

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> The other danger here is - what exactly do you mean by "no command has
> been able to run between the user command and our lookup"? Because
> you can do stupid things with functions like set_config(). This would
> only be safe if no user-provided expressions can possibly get
> evaluated between point A and point B, and that strikes me as the sort
> of thing that could easily be false unless this is all done VERY close
> to the start of processing.

To me, the largest single risk of the whole event triggers feature is
precisely that it will result in random user-provided code getting
executed in fairly random places, thus breaking assumptions of this type
that may be hard or impossible to fix. But given that allowing that
is more or less exactly the point of the feature, I'm not sure why
you're blaming the patch for it. It should have been rejected on day
one if you're not willing to have that type of risk.

regards, tom lane

On Sat, 2012-12-29 at 08:11 -0500, Robert Haas wrote:
> On Tue, Dec 25, 2012 at 1:42 PM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)fr> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >>> 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 shall rely on you to provide those bricks which are still missing.
> >
> > Please find attached a patch to change most functions called from
> > standard_ProcessUtility() to return an Oid (passes `make
> > maintainer-clean; configure; make install check`). Most of them only,
> > because it only make sense for functions touching an object that exists
> > in the catalogs and have a distinct Oid.
>
> OK, I committed this.

There is a new compiler warning coming from this, I believe:

copy.c: In function ‘DoCopy’:
copy.c:835:2: error: ‘relid’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
return relid;
^
copy.c:753:6: note: ‘relid’ was declared here
Oid relid;
^

Hi,

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> There is a new compiler warning coming from this, I believe:

I don't get this warning here, at least not in -O2, and I did forget to
make maintainer-clean then rebuild with -O0 just in case gcc is now
complaining about something else. I wish this situation could be fixed.

Now, this warning needs to be fixed, too, and here's a patch for that.

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

On Sun, 2012-12-30 at 13:18 +0100, Dimitri Fontaine wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > There is a new compiler warning coming from this, I believe:
>
> I don't get this warning here, at least not in -O2, and I did forget to
> make maintainer-clean then rebuild with -O0 just in case gcc is now
> complaining about something else. I wish this situation could be fixed.

I get this warning in Debian squeeze and wheezy with fairly standard
build options. I don't think -O0 will give you any more warnings.

> Now, this warning needs to be fixed, too, and here's a patch for that.

Thanks, I fixed it slightly differently.

Dimitri Fontaine escribió:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > OK, I committed this.
>
> Thanks. Please find attached a rebased patch, v6. I think it's looking
> more like what you would expect now:

I gave this patch a look. I'm not done with it; I mostly gave the
utility.c changes some love so that the end result is not as cluttered.
I did this by creating a couple of macros that call the Start() thing,
then set the OID, then call the End() thing. This made pretty obvious
the places that were missing to set the object OID; I changed the CREATE
TABLE AS code to return it, for instance. The patch I attach applies on
top of Dimitri's v6 patch. With these changes, it's much easier to
review the big standard_ProcessUtility() switch and verify cases that
are being handled correctly. (A few places cannot use the macros I
defined because they use more complex arrangements. This is fine, I
think, because they are few enough that can be verified manually.)

I also noticed that ALTER DEFAULT PRIVILEGES was trying to fire event
triggers, even though we don't support GRANT and REVOKE; it seems to me
that the three things out to be supported together. I'm told David
Fetter would call this "DCL support", and we're not supporting DCL at
this stage. So DEFAULT PRIVILEGES ought to be not supported.

I also observed that the SECURITY LABEL commands does not fire event
triggers; I think it should. But then maybe this can be rightly called
DCL as well, so perhaps it's all right to leave it out.

DROP OWNED is not firing event triggers. There is a comment therein
that says "we don't fire them for shared objects", but this is wrong:
this command is dropping local objects, not shared, so it seems
necessary to me that triggers are fired.

I also noticed that some cases such as DROP <whatever> do not report the
OIDs of all the objects that are being dropped, only one of them. This
seems bogus to me; if you do "DROP TABLE foo,bar" then both tables
should be reported.

> Given the OID, we use the ObjectProperty[] structure to know which cache
> or catalog to scan in order to get the name and namespace of the object.

The changes to make ObjectPropertyType visible to code outside
objectaddress.c look bogus to me. I think we should make this new code
use the interfaces we already have.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera escribió:

> I gave this patch a look. I'm not done with it; I mostly gave the
> utility.c changes some love so that the end result is not as cluttered.
> I did this by creating a couple of macros that call the Start() thing,
> then set the OID, then call the End() thing. This made pretty obvious
> the places that were missing to set the object OID; I changed the CREATE
> TABLE AS code to return it, for instance. The patch I attach applies on
> top of Dimitri's v6 patch.

... and here's the attachment.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> I gave this patch a look. I'm not done with it; I mostly gave the
> utility.c changes some love so that the end result is not as cluttered.

Thanks Álvaro!

> I did this by creating a couple of macros that call the Start() thing,
> then set the OID, then call the End() thing. This made pretty obvious
> the places that were missing to set the object OID; I changed the CREATE
> TABLE AS code to return it, for instance. The patch I attach applies on
> top of Dimitri's v6 patch. With these changes, it's much easier to
> review the big standard_ProcessUtility() switch and verify cases that
> are being handled correctly. (A few places cannot use the macros I
> defined because they use more complex arrangements. This is fine, I
> think, because they are few enough that can be verified manually.)

That sounds great. I'm not at ease with creating avdanved C macros and
I'm very happy you did it :)

> I also noticed that ALTER DEFAULT PRIVILEGES was trying to fire event
> triggers, even though we don't support GRANT and REVOKE; it seems to me
> that the three things out to be supported together. I'm told David
> Fetter would call this "DCL support", and we're not supporting DCL at
> this stage. So DEFAULT PRIVILEGES ought to be not supported.

Agreed. Do you want me to edit my patch to remove support for that
command and the other arrangement we are talking about, or do you want
to continue having at it?

> I also observed that the SECURITY LABEL commands does not fire event
> triggers; I think it should. But then maybe this can be rightly called
> DCL as well, so perhaps it's all right to leave it out.

I would agree with calling that a DCL.

> DROP OWNED is not firing event triggers. There is a comment therein
> that says "we don't fire them for shared objects", but this is wrong:
> this command is dropping local objects, not shared, so it seems
> necessary to me that triggers are fired.

I think the best way to address DROP OWNED is the same as to address
DROP CASCADE: have the inner code (doDeletion, I think) prepare another
DropStmt and give control back to ProcessUtility() with a context of
PROCESS_UTILITY_CASCADE.

The good news is that the unified DropStmt support allows us to think
about that development, even if it still is a non trivial refactoring.

I would like to hear that we can consider the current patch first then
refactor the drop cascade operations so that we automatically have full
support for DROP OWNED.

> I also noticed that some cases such as DROP <whatever> do not report the
> OIDs of all the objects that are being dropped, only one of them. This
> seems bogus to me; if you do "DROP TABLE foo,bar" then both tables
> should be reported.

When we have the DROP CASCADE refactoring, we could maybe implement drop
on a list of objects as getting back to ProcessUtility() also, with yet
another context (SUBCOMMAND or maybe GENERATED would do here).

The other way to address the information problem would be to expose it
as a relation (resultset, setof record, cursor, you name it) variable to
PLpgSQL.

Again, I hope we can decide on the approach now, commit what we have and
expand on it on the next commit fest.

>> Given the OID, we use the ObjectProperty[] structure to know which cache
>> or catalog to scan in order to get the name and namespace of the object.
>
> The changes to make ObjectPropertyType visible to code outside
> objectaddress.c look bogus to me. I think we should make this new code
> use the interfaces we already have.

We need to move the new fonction get_object_name_and_namespace() into
the objectaddress.c "module" then, and decide about returning yet
another structure (because we have two values to return) or to pass that
function a couple of char **. Which devil do you prefer?

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

Dimitri Fontaine escribió:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:

> >> Given the OID, we use the ObjectProperty[] structure to know which cache
> >> or catalog to scan in order to get the name and namespace of the object.
> >
> > The changes to make ObjectPropertyType visible to code outside
> > objectaddress.c look bogus to me. I think we should make this new code
> > use the interfaces we already have.
>
> We need to move the new fonction get_object_name_and_namespace() into
> the objectaddress.c "module" then, and decide about returning yet
> another structure (because we have two values to return) or to pass that
> function a couple of char **. Which devil do you prefer?

I made some changes to this, and I think the result (attached) is
cleaner overall.

Now, this review is pretty much unfinished as far as I am concerned;
mainly I've been trying to figure out how it all works and improving
some stuff as I go, and I haven't looked at the complete patch yet. We
might very well doing some things quite differently; for example I'm not
really sure of the way we handle CommandTags, or the way we do object
lookups at the event trigger stage, only to have it repeated later when
the actual deletion takes place. In particular, since event_trigger.c
does not know the lock strength that's going to be taken at deletion, it
only grabs ShareUpdateExclusiveLock; so there is lock escalation here
which could lead to deadlocks. This might need to be rethought. I
added a comment somewhere, noting that perhaps it's ProcessUtility's job
to set the object classId (or at least the ObjectType) at the same time
the TargetOid is being set, rather than have event_trigger.c figure it
out from the parse node. And so on. I haven't looked at plpgsql or
pg_dump yet, either.

We've been discussing on IM the handling of DROP in general. The
current way it works is that the object OID is reported only if the
toplevel command specifies to drop a single object (so no OID if you do
DROP TABLE foo,bar); and this is all done by standard_ProcessUtility.
This seems bogus to me, and it's also problematic for things like DROP
SCHEMA, as well as DROP OWNED BY (which, as noted upthread, is not
handled at all but should of course be). I think the way to do it is
have performDeletion and performMultipleDeletions (dependency.c) call
into the event trigger code, by doing something like this:

1. look up all objects to drop (i.e. resolve the list of objects
specified by the user, and complete with objects dependent on those)

2. iterate over this list, firing DROP at-start event triggers

3. do the actual deletion of objects (i.e. deleteOneObject, I think)

4. iterate again over the list firing DROP at-end event triggers

We would have one or more event triggers being called with a context of
TOPLEVEL (for objects directly mentioned in the command), and then some
more calls with a context of CASCADE. Exactly how should DROP OWNED BY
be handled is not clear; perhaps we should raise one TOPLEVEL event for
the objects directly owned by the role? Maybe we should just do CASCADE
for all objects? This is not clear yet.

Thoughts?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Jan 9, 2013 at 11:58 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> I made some changes to this, and I think the result (attached) is
> cleaner overall.
>
> Now, this review is pretty much unfinished as far as I am concerned;
> mainly I've been trying to figure out how it all works and improving
> some stuff as I go, and I haven't looked at the complete patch yet. We
> might very well doing some things quite differently; for example I'm not
> really sure of the way we handle CommandTags, or the way we do object
> lookups at the event trigger stage, only to have it repeated later when
> the actual deletion takes place. In particular, since event_trigger.c
> does not know the lock strength that's going to be taken at deletion, it
> only grabs ShareUpdateExclusiveLock; so there is lock escalation here
> which could lead to deadlocks. This might need to be rethought. I
> added a comment somewhere, noting that perhaps it's ProcessUtility's job
> to set the object classId (or at least the ObjectType) at the same time
> the TargetOid is being set, rather than have event_trigger.c figure it
> out from the parse node. And so on. I haven't looked at plpgsql or
> pg_dump yet, either.

I think this points to a couple of problems: this patch isn't
well-enough thought out, and it's got several features jammed into a
single patch. This should really be split up into several patches and
each one submitted separately.

I think that ddl_command_trace is an unnecessary frammish that should
be ripped out entirely. It is easy enough to accomplish the same
thing with ddl_command_start and ddl_command_end, and so we're just
increasing the number of cases we have to support in the future for no
real benefit.

> We've been discussing on IM the handling of DROP in general. The
> current way it works is that the object OID is reported only if the
> toplevel command specifies to drop a single object (so no OID if you do
> DROP TABLE foo,bar); and this is all done by standard_ProcessUtility.
> This seems bogus to me, and it's also problematic for things like DROP
> SCHEMA, as well as DROP OWNED BY (which, as noted upthread, is not
> handled at all but should of course be). I think the way to do it is
> have performDeletion and performMultipleDeletions (dependency.c) call
> into the event trigger code, by doing something like this:
>
> 1. look up all objects to drop (i.e. resolve the list of objects
> specified by the user, and complete with objects dependent on those)
>
> 2. iterate over this list, firing DROP at-start event triggers
>
> 3. do the actual deletion of objects (i.e. deleteOneObject, I think)
>
> 4. iterate again over the list firing DROP at-end event triggers

This gets right back to an argument Dimitri and I have been having
since v1 of this patch, which is whether these are command triggers or
event triggers. I think we ought to support both, but they are not
the same thing. I think the design you are proposing is just fine for
an event called sql_drop, but it makes no sense for an event called
ddl_command_end, which needs to be called once per DDL statement - at
the end. Not once per object dropped.

> We would have one or more event triggers being called with a context of
> TOPLEVEL (for objects directly mentioned in the command), and then some
> more calls with a context of CASCADE. Exactly how should DROP OWNED BY
> be handled is not clear; perhaps we should raise one TOPLEVEL event for
> the objects directly owned by the role? Maybe we should just do CASCADE
> for all objects? This is not clear yet.

TOPLEVEL is supposed to correspond to a complete SQL statement entered
by the user:

typedef enum
{
PROCESS_UTILITY_TOPLEVEL, /* toplevel
interactive command */
PROCESS_UTILITY_QUERY, /* a complete query,
but not toplevel */
PROCESS_UTILITY_SUBCOMMAND, /* a piece of a query */
PROCESS_UTILITY_GENERATED /* internally
generated node query node */
} ProcessUtilityContext;

IMHO, "DROP OWNED BY" probably shouldn't fire ddl_command_start/end.
I excluded it from ddl_command_start in the initial commit because
it's such a weird case, and it didn't seem to cleanly fit in the
framework. Now, that could be changed, but surely that should be a
separate patch if we're going to do it rather than lumped in with a
bunch of other dubious changes, and more than that, it just
underscores, again, the fact that we're pouring a lot of effort into
fleshing out ddl_command_start/end instead of doing what we probably
should be doing, which is adding events that are actually targeting
what we want to do, like sql_drop.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I think this points to a couple of problems: this patch isn't
> well-enough thought out, and it's got several features jammed into a
> single patch. This should really be split up into several patches and
> each one submitted separately.

Ok. Now I want to talk about our process a little. That's a 2 paragraphs
diversion, after that it's getting back to technical matters.

There's a difference between "it's not the way I would have done it" and
"the author didn't think about what he's doing". That's also the reason
why it's very hard to justify sending a polished enough patch as a non
commiter.

And then this patch is like the next one in a long series that is
lasting for about 2 years now, and spliting it is just more work for
everybody, and then you take the risk that the next commiter who looks
at the patch prefers to see a complete enough view of the goal you're
trying to reach.

> I think that ddl_command_trace is an unnecessary frammish that should
> be ripped out entirely. It is easy enough to accomplish the same
> thing with ddl_command_start and ddl_command_end, and so we're just
> increasing the number of cases we have to support in the future for no
> real benefit.

I think you might want to review the use case behing ddl_command_trace,
that has been asked by who users wanted to see their use case supported
in some easier way than just what you're talking about here.

> This gets right back to an argument Dimitri and I have been having
> since v1 of this patch, which is whether these are command triggers or
> event triggers. I think we ought to support both, but they are not
> the same thing. I think the design you are proposing is just fine for
> an event called sql_drop, but it makes no sense for an event called
> ddl_command_end, which needs to be called once per DDL statement - at
> the end. Not once per object dropped.

What I think you're missing here is the proposal flying around to have
drop operation get back to ProcessUtility so that C hooks and event
triggers both can have at it. It's been debated on the list earlier, way
before we talked about event triggers, also as a way to clean up things,
IIRC.

I agree that if we keep the code as it is to implement cascading
behavior, a sql_drop event makes more sense.

> TOPLEVEL is supposed to correspond to a complete SQL statement entered
> by the user:

Having the DROP command get back to ProcessUtility would allow us to
have the nested events as GENERATED, but CASCADE would allow an easier
way to just match that if that's what users are interested into.

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

On Wed, Jan 16, 2013 at 4:16 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Ok. Now I want to talk about our process a little. That's a 2 paragraphs
> diversion, after that it's getting back to technical matters.
>
> There's a difference between "it's not the way I would have done it" and
> "the author didn't think about what he's doing". That's also the reason
> why it's very hard to justify sending a polished enough patch as a non
> commiter.

There's no rule that anyone's got to agree with my opinion on
anything, but the idea that a patch should do one thing and do it well
is not a novel concept on this mailing list, however much you may feel
otherwise. This patch:

- adds ddl_command_trace and ddl_command_end events
- causes those events to be called not only for actual SQL commands
but also for things that happen, at present, to go through the same
code path
- adds additional magic variables to PL/pgsql to expose new
information not previously exposed
- adds CONTEXT as an additional event-trigger-filter variable, along with TAG

In my mind, that's four or five separate patches. You don't have to
agree, but that's what I think, and I'm not going to apologize for
thinking it. Moreover, I think you will struggle to find examples of
patches committed in the last three years that made as many different,
only loosely-related changes as you're proposing for this one.

As for the patch not being well-thought-out, I'm sticking by that,
too. Alvaro's comments about lock escalations should be enough to
tickle your alarm bells, but there are plenty of other problems here,
too.

> I think you might want to review the use case behing ddl_command_trace,
> that has been asked by who users wanted to see their use case supported
> in some easier way than just what you're talking about here.

That argument carries no water with me. You're asking every
PostgreSQL user in the universe to carry the overhead of a feature
that 90% of them will not use. That is mighty expensive syntactic
sugar. I am not talking about code-complexity, either. It is pretty
obvious that there is run-time overhead of having this feature. I am
not aware of any case where we have accepted run-time overhead for a
feature not mandated by the SQL standard. Given the rest of what you
have here, it is quite simple to arrange for the same function to be
called after a create or alter and before a drop. Being able to do
that in one command instead of two is not a sufficient reason to add
another event type.

>> This gets right back to an argument Dimitri and I have been having
>> since v1 of this patch, which is whether these are command triggers or
>> event triggers. I think we ought to support both, but they are not
>> the same thing. I think the design you are proposing is just fine for
>> an event called sql_drop, but it makes no sense for an event called
>> ddl_command_end, which needs to be called once per DDL statement - at
>> the end. Not once per object dropped.
>
> What I think you're missing here is the proposal flying around to have
> drop operation get back to ProcessUtility so that C hooks and event
> triggers both can have at it. It's been debated on the list earlier, way
> before we talked about event triggers, also as a way to clean up things,
> IIRC.

I'm very much opposed to that proposal, as I am to your proposal to
expose internal and generated events to users. Recursing into
ProcessUtility is a nasty, messy hook that is responsible for subtle
bugs and locking problems in our current DDL implementation. We
should be trying to refactor that to clean it up, not exposing it as a
user-visible detail. I do NOT want a future refactoring effort to
clean up race conditions and duplicate name lookups in the DDL code to
be blocked because someone is relying on the details of the existing
implementation in their event trigger implementation.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> There's a difference between "it's not the way I would have done it" and
> "the author didn't think about what he's doing". That's also the reason
> why it's very hard to justify sending a polished enough patch as a non
> commiter.

> And then this patch is like the next one in a long series that is
> lasting for about 2 years now, and spliting it is just more work for
> everybody, and then you take the risk that the next commiter who looks
> at the patch prefers to see a complete enough view of the goal you're
> trying to reach.

What was discussed at the last dev meeting was assigning a committer to
each large patch to start with, which would reduce the risk of the
goalposts moving that way. It seems to me that Robert's at least
unofficially taken that role for event triggers. You should be happy,
because if I were reviewing it I'd likely bounce the whole thing.
I'm not convinced this will *ever* be a stable feature that doesn't
create more problems than it fixes.

> What I think you're missing here is the proposal flying around to have
> drop operation get back to ProcessUtility so that C hooks and event
> triggers both can have at it.

I've seen no such proposal, and it seems like a nonstarter just from the
idea. dependency.c doesn't have a syntax tree to describe each object
that it finds to drop; creating one, and then doing a lookup to re-find
the object, is just going to make drops hugely slower and buggier. Not
to mention the large amount of code that would have to be added and
maintained. Not to mention that the objects dependency.c works with
aren't necessarily all that interesting from the user's level --- for
instance, do you really want to see each column default expression
dropped individually? Not to mention that the permissions
considerations are different from a standalone DROP.

The bigger picture there is that it's taken us years, and multiple major
iterations, to get cascaded drops to work properly and reliably. I'm
disinclined to rip that code open and rewrite it completely; let alone
add hooks that might inject random operations at any point in the
process.

regards, tom lane

On Wed, Jan 16, 2013 at 6:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> What was discussed at the last dev meeting was assigning a committer to
> each large patch to start with, which would reduce the risk of the
> goalposts moving that way. It seems to me that Robert's at least
> unofficially taken that role for event triggers. You should be happy,
> because if I were reviewing it I'd likely bounce the whole thing.
> I'm not convinced this will *ever* be a stable feature that doesn't
> create more problems than it fixes.

And speaking of the goalposts moving...

I don't think that's the problem, here. Rather, I think the problem
is that the design is ardently refusing to move. It might be a slight
overstatement to say that every review I've ever posted for this patch
has complained about design decisions that expose implementation
details to the user that we might want to change later, but not by
much. And yet, two years on, we've got proposals on the table to
artificially force *more* things through ProcessUtility(). There's no
particularly consistency to which things do and don't go through that
function today, and no reason whatsoever to try to force everything to
go through there. I agree with everything you say in the portion of
the email I didn't quote, and I'm pretty sure I've made similar points
more than once in the past.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> - adds ddl_command_trace and ddl_command_end events
> - causes those events to be called not only for actual SQL commands
> but also for things that happen, at present, to go through the same
> code path
> - adds additional magic variables to PL/pgsql to expose new
> information not previously exposed
> - adds CONTEXT as an additional event-trigger-filter variable, along with TAG
>
> In my mind, that's four or five separate patches. You don't have to
> agree, but that's what I think, and I'm not going to apologize for
> thinking it. Moreover, I think you will struggle to find examples of
> patches committed in the last three years that made as many different,
> only loosely-related changes as you're proposing for this one.

So, say I do that. Now we have 5 patches to review. They all are against
the same set of files, so there's a single possible ordering to apply
them, and so to review them. When we reach to an agreement out-of-order
(we will), it takes a sensible amount of time to rebuild the patch set
in a different order. I don't think we have the time to do that.

Now, if that's what it takes, I'll spend time on it. In which exact
order do you want to be reviewing and applying that series of patches?

> As for the patch not being well-thought-out, I'm sticking by that,
> too. Alvaro's comments about lock escalations should be enough to
> tickle your alarm bells, but there are plenty of other problems here,
> too.

Here's the comment in the code where the lock problems are discussed:

/*
* Fill in the EventTriggerTargetOid for a DROP command and a
* "ddl_command_start" Event Trigger: the lookup didn't happen yet and we
* still want to provide the user with that information when possible.
*
* We only do the lookup if the command contains a single element, which is
* often forced to be the case as per the grammar (see the drop_type:
* production for a list of cases where more than one object per command is
* allowed).
*
* We take no exclusive lock here, the main command will have to do the
* name lookup all over again and take appropriate locks down after the
* User Defined Code runs anyway, and the commands executed by the User
* Defined Code will take their own locks. We only lock the objects here so
* that they don't disapear under us in another concurrent transaction,
* hence using ShareUpdateExclusiveLock. XXX this seems prone to lock
* escalation troubles.
*/

My thinking is that the user code can do anything, even run a DROP
command against the object that we are preparing ourselves to be
dropping. I don't think we can bypass doing the lookup twice.

The way not to have to do any lookup in our code is not to report the
OID of the Object being dropped. Then, the event trigger code will do
that lookup as its first thing, certainly. The lookup will happen.

RAISE NOTICE 'drop table % (%)', tg_objectname::regclass,
tg_objectname::regclass::oid;

That is, at least, the angle I considered when crafting this patch. I'm
happy to change that angle if needed and adjust to things I missed and
you will tell me about.

>> I think you might want to review the use case behing ddl_command_trace,
>> that has been asked by who users wanted to see their use case supported
>> in some easier way than just what you're talking about here.
>
> […]
> Being able to do
> that in one command instead of two is not a sufficient reason to add
> another event type.

That feature was proposed in past october (with a patch) and received no
comment, so I went on with it. The performance costs of this patch is
another couple of cache lookups when doing a DDL command, which I saw as
acceptable.

http://www.postgresql.org/message-id/m28vbgcc30.fsf@2ndQuadrant.fr

I'm not so attached to it that I will put the rest of the patch in
danger with it, I'm fine about removing that part if we have a good
reason to. And yes, you (politely) saying "I don't like it" is a good
enough reason to.

Our process for a non-commiter proposal seems to me that you have to
send a patch showing what you're talking about for a thread to form and
to get some comments on the idea, either rejecting it or reaching to a
consensus about it. So the opening of that thread was in october and the
discussion happens now: it's very fine, and I could be happy and
thankfull about this fact with some subtle changes in the form of the
messages.

> I'm very much opposed to that proposal, as I am to your proposal to
> expose internal and generated events to users. Recursing into
> ProcessUtility is a nasty, messy hook that is responsible for subtle
> bugs and locking problems in our current DDL implementation. We

We are publishing 2 pieces of information tied to the implementation in
the current patch:

- the parse tree, only available to C code, and without any API
stability promises

- the fact that somme commands are running nested commands: serial does
a create sequence, create schema create table issues a create table.

My understanding of your proposal not to call them ddl_command_* events
is that you are exposing exactly the same set of implementation
dependent information to the users, just under another name.

What I mean is that if a user wants to log any sequence creation, he
will have to either know that some of them happen within the CONTEXT
named 'GENERATED', or to know that some of them happen as ddl_command_*
events and some of them happen as sequence_for_serial_* events, say.

In other words, while I can follow your general approach to things, I
don't see any practical impact that makes me think it's a much cleaner
proposal in terms of not publishing the implementation details, because
I don't think that it is possible to both give the users the information
they need and hide those implementation details.

> should be trying to refactor that to clean it up, not exposing it as a
> user-visible detail. I do NOT want a future refactoring effort to
> clean up race conditions and duplicate name lookups in the DDL code to
> be blocked because someone is relying on the details of the existing
> implementation in their event trigger implementation.

That problem is going to happen in some way or another. It's not about
avoiding the problem completely, but choosing a set of compromises. I'm
yet to understand what your set of compromises is in detailed enough
practical implementation terms (not just "it's another set of events")
and how it's a better compromise over all (I trust your judgement on
that, I still want to understand).

After all, the whole point of the Event Trigger patch is to expose some
level of implementation details.

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> What was discussed at the last dev meeting was assigning a committer to
> each large patch to start with, which would reduce the risk of the
> goalposts moving that way. It seems to me that Robert's at least
> unofficially taken that role for event triggers. You should be happy,
> because if I were reviewing it I'd likely bounce the whole thing.

As Robert, I don't think the problem is about moving goal posts here,
it's more about how we want to spend our time. I don't want to be
refactoring 5 branches each time we reach on a new consensus only to
realise that of course the inter-dependent set of patches just need to
be redone from scratch because we will be applying another one first.

As for the nested commands design, see above for why I'm not already
buying into his counter proposal. Hint: I don't think it solves the
problem Robert is complaining about. Details: I certainly don't
understand what problem he is complaining about really, and I know I've
been trying to get that information before. It must be something
obvious.

> I'm not convinced this will *ever* be a stable feature that doesn't
> create more problems than it fixes.

I'm still open to hearing about another way to implement

- ddl support for logical replication

- extensions to ddl behaviour

I want to be able to install an extension that will change the way some
DDL are run, and I know that what I want to do here will never get
accepted for the core version of PostgreSQL. That's a very fine position
that I understand well enough to work on a patch series where the only
goal is that I can do my hacking outside of PostgreSQL core.

> I've seen no such proposal, and it seems like a nonstarter just from the
> idea. dependency.c doesn't have a syntax tree to describe each object
> that it finds to drop; creating one, and then doing a lookup to re-find
> the object, is just going to make drops hugely slower and buggier. Not

We wouldn't want to be doing it that way, of course. One idea was to
create a new parse node (CascadeDropStmt, say) that gets the OID of the
object to drop, and only does another schema/name lookup when there's an
event trigger to fire.

So the only obvious part of that proposal is that the problem is complex
enough and the solution not ready yet. That's also why that problem is
not addressed in any patch I sent to this day. The status of that part
of the problem is still to find together a good design to implement.

That part of the discussion really should live in a separate thread.

> to mention the large amount of code that would have to be added and
> maintained. Not to mention that the objects dependency.c works with
> aren't necessarily all that interesting from the user's level --- for
> instance, do you really want to see each column default expression
> dropped individually? Not to mention that the permissions
> considerations are different from a standalone DROP.

No, we don't want to see all the details. Yes, we will need to come up
with a list of the ones we want to see. As I'm currently working on the
DDL part of the Event Triggers, my current thinking is to only include
objects that we could have been DROPing with a TOPLEVEL command.

> The bigger picture there is that it's taken us years, and multiple major
> iterations, to get cascaded drops to work properly and reliably. I'm
> disinclined to rip that code open and rewrite it completely; let alone
> add hooks that might inject random operations at any point in the
> process.

Fair enough. If that's the consensus, there's nothing to change in my
current patch, or maybe a paragraph or two in the docs to set users
expectations.

Maybe we should ask the logical replication guys their opinion about how
to solve DROP CASCADE support for them, so that we all agree that not
doing anything is a good enough answer.

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

On Thu, Jan 17, 2013 at 5:18 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Now, if that's what it takes, I'll spend time on it. In which exact
> order do you want to be reviewing and applying that series of patches?

Let's agree on which things we even want to do first. Here's my take:

- adds ddl_command_trace and ddl_command_end events

I think ddl_command_end is OK and I'm willing to commit that if
extracted as its own patch. I think ddl_command_trace is unnecessary
syntactic sugar.

- causes those events to be called not only for actual SQL commands
but also for things that happen, at present, to go through the same
code path

I think this is a bad idea, not only because, as I said before, it
exposes internal implementation details, but also because it's
probably not safe. As Noah (I believe, or maybe Tom), observed in a
previous post, we've gone to a lot of work to make sure that nothing
you do inside a trigger can crash the server, corrupt the catalogs,
trigger elog() messages, etc. That applies in spades to DDL.

Despite Tom's skepticism, I'm pretty sure that it's safe for us to
execute trigger function calls either completely before or completely
after we execute the DDL command. It should be no different than
executing them as separate commands. But doing something in the
middle of a command is something else altogether. Consider, for
example, that ALTER TABLE does a bunch of crap internally to itself,
and then recurses into ProcessUtility to do some more stuff. This is
a terrible design, but it's what we've got. Are you absolutely sure
that the intermediate state that exists after that first bunch of
stuff is done and before those other things are done is a safe place
to execute arbitrary user-provided code? I'm not. And the more
places we add that recurse into ProcessUtility, or indeed, add event
triggers in general, the more places we're going to add new failure
modes.

Fixing that is part of the price of adding a new event trigger and I'm
OK with that. But I'm not OK with baking into the code the assumption
that any current or future callers of ProcessUtility() should be
prepared for arbitrary code execution. The code wasn't written with
that in mind, and it will not end well.

- adds additional magic variables to PL/pgsql to expose new
information not previously exposed

I'm not sure that I agree with the implementation of this, but I think
I'm OK with the concept. I will review it when it is submitted in a
form not intertwined with other things.

- adds CONTEXT as an additional event-trigger-filter variable, along with TAG

I'm OK with adding new event-trigger-filter variables, provided that
they are done without additional run-time overhead for people who are
not using event triggers; I think it's nice that we can support that.
But I think this particular proposal is DOA because nothing except
TOPLEVEL is actually safe.

>> As for the patch not being well-thought-out, I'm sticking by that,
>> too. Alvaro's comments about lock escalations should be enough to
>> tickle your alarm bells, but there are plenty of other problems here,
>> too.
>
> Here's the comment in the code where the lock problems are discussed:
>
> /*
> * Fill in the EventTriggerTargetOid for a DROP command and a
> * "ddl_command_start" Event Trigger: the lookup didn't happen yet and we
> * still want to provide the user with that information when possible.
> *
> * We only do the lookup if the command contains a single element, which is
> * often forced to be the case as per the grammar (see the drop_type:
> * production for a list of cases where more than one object per command is
> * allowed).
> *
> * We take no exclusive lock here, the main command will have to do the
> * name lookup all over again and take appropriate locks down after the
> * User Defined Code runs anyway, and the commands executed by the User
> * Defined Code will take their own locks. We only lock the objects here so
> * that they don't disapear under us in another concurrent transaction,
> * hence using ShareUpdateExclusiveLock. XXX this seems prone to lock
> * escalation troubles.
> */
>
> My thinking is that the user code can do anything, even run a DROP
> command against the object that we are preparing ourselves to be
> dropping. I don't think we can bypass doing the lookup twice.

Sure, but there's also the issue of getting the right answer. For
example, suppose you are planning to drop a function. You do a name
lookup on the name and call the event trigger. Meanwhile, someone
else renames the function and creates a new one with the same name.
After the event trigger returns, the actual drop code latches onto the
one with the new name. Now, the event trigger ran with OID A and the
actual object dropped was one with OID B. It's possible that KaiGai's
RENAME refactoring in 9.3 has strengthened the locking enough that
this particular failure mode is no longer possible in that specific
scenario, but I am fairly confident it would have been an issue in
9.2, and it may still be. In general, the only way to be sure that
things like that can't happen is to only do the name lookup once. Our
locking on non-table objects is still pretty flim-flammy, and there
are a lot of concurrent scenarios where things that seem like they
shouldn't change under you actually can.

There's another issue here, too, which is that this change
reintroduces a failure mode that I spent a lot of time cleaning up
during the 9.2 cycle - namely, taking locks on a table before
permissions have been checked, thus providing new methods for
unprivileged users to block operations on tables they don't have
rights to.

>> I'm very much opposed to that proposal, as I am to your proposal to
>> expose internal and generated events to users. Recursing into
>> ProcessUtility is a nasty, messy hook that is responsible for subtle
>> bugs and locking problems in our current DDL implementation. We
>
> We are publishing 2 pieces of information tied to the implementation in
> the current patch:
>
> - the parse tree, only available to C code, and without any API
> stability promises

I'm OK with that. People who program in C are taking their life in
their hands anyway as far as new versions are concerned.

> - the fact that somme commands are running nested commands: serial does
> a create sequence, create schema create table issues a create table.
>
> My understanding of your proposal not to call them ddl_command_* events
> is that you are exposing exactly the same set of implementation
> dependent information to the users, just under another name.

What I'm proposing is that we have a separate set of events that get
invoked every time an object is created, altered, or dropped,
regardless of the cause. Yes, the name will be different, but the
point is not to only change the name, but rather to also change the
definition. There are two separate things you might want here:

- to get a callback every time a command is executed (e.g. so you can
log it, check permissions, or reject the entire statement)
- to get a callback every time a certain kind of SQL object is
created, altered, or destroyed (e.g. so you can replicate those
operations on another server)

For example, suppose a user executes the command "DROP OWNED BY fred".
For a logging hook, you want the first thing: one callback, saying,
hey, the user is trying to run DROP OWNED BY fred. You log it once,
and go home. For a replication hook, you want the second thing: one
callback per object, with details about each one, so you can drop them
on the remote side. Both things are useful, but they are different.

As a further example, suppose that in 9.4 (or 9.17) we add a command
"DROP TABLES IN SCHEMA fred WHERE name LIKE 'bob%'". Well, the
logging trigger still just works (because it's only writing the
statement, without caring about its contents). And the replication
trigger still just works too (because it will still get a callback for
every table that gets dropped, even though it knows nothing about the
new syntax). That's very powerful. Of course, there will still be
cases where things have to change, but you can minimize it by clean
definitions.

It pains me that I've evidently failed to communicate this concept
clearly despite a year or more of trying. Does that make sense? Is
there some way I can make this more clear? The difference seems very
clear-cut to me, but evidently I'm not conveying it well.

> That problem is going to happen in some way or another. It's not about
> avoiding the problem completely, but choosing a set of compromises. I'm
> yet to understand what your set of compromises is in detailed enough
> practical implementation terms (not just "it's another set of events")
> and how it's a better compromise over all (I trust your judgement on
> that, I still want to understand).
>
> After all, the whole point of the Event Trigger patch is to expose some
> level of implementation details.

I don't really agree with that. I think the point is to expose what
the system is doing to the DBA. I'm OK with exposing the fact that
creating a table with a serial column also creates a sequence. There
is no problem with that - it's all good. What we DON'T want to do is
set up a system where the fact that it creates a sequence is exposed
because that happens to go through ProcessUtility() and the fact that
it creates a constraint is not because that happens not to go through
ProcessUtility(). That is not the sort of quality that our users have
come to expect from PostgreSQL.

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

On 2013-01-17 11:15:24 -0500, Robert Haas wrote:
> As a further example, suppose that in 9.4 (or 9.17) we add a command
> "DROP TABLES IN SCHEMA fred WHERE name LIKE 'bob%'". Well, the
> logging trigger still just works (because it's only writing the
> statement, without caring about its contents). And the replication
> trigger still just works too (because it will still get a callback for
> every table that gets dropped, even though it knows nothing about the
> new syntax). That's very powerful. Of course, there will still be
> cases where things have to change, but you can minimize it by clean
> definitions.
>
> It pains me that I've evidently failed to communicate this concept
> clearly despite a year or more of trying. Does that make sense? Is
> there some way I can make this more clear? The difference seems very
> clear-cut to me, but evidently I'm not conveying it well.

Well, I didn't manage to follow the topic with the level of detail I
would have liked to/should have so its very well possible that I missed
a proposal of how to implement that concept in a way you like?
With some squinting you can actually see the proposed ddl_trace callsite
as exactly that kind of replication trigger, but with a worse name...

Without piggy-backing on the internal commands generated - which
currently seems to be achieved by passing everything through
ProcessUtility, but could work in a quite different way - and
reconstructing sql from that I don't immediately see how you want to do
this?

Greetings,

Andres Freund

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> - adds ddl_command_trace and ddl_command_end events
>
> I think ddl_command_end is OK and I'm willing to commit that if
> extracted as its own patch. I think ddl_command_trace is unnecessary
> syntactic sugar.

Ok. Will prepare a non controversial patch for ddl_command_end. After
having played with the patch, I happen to quite like ddl_command_trace,
but I won't try to protect it from its death any more than that.

> - causes those events to be called not only for actual SQL commands
> but also for things that happen, at present, to go through the same
> code path
>
> I think this is a bad idea, not only because, as I said before, it
> exposes internal implementation details, but also because it's
> probably not safe. As Noah (I believe, or maybe Tom), observed in a
> previous post, we've gone to a lot of work to make sure that nothing
> you do inside a trigger can crash the server, corrupt the catalogs,
> trigger elog() messages, etc. That applies in spades to DDL.

I naively though that doing CommandCounterIncrement() before running the
event trigger would go a long enough way towards making the operation
safe when the current code is calling back into ProcessUtility().

> Despite Tom's skepticism, I'm pretty sure that it's safe for us to
> execute trigger function calls either completely before or completely
> after we execute the DDL command. It should be no different than
> executing them as separate commands. But doing something in the
> middle of a command is something else altogether. Consider, for
> example, that ALTER TABLE does a bunch of crap internally to itself,
> and then recurses into ProcessUtility to do some more stuff. This is
> a terrible design, but it's what we've got. Are you absolutely sure
> that the intermediate state that exists after that first bunch of
> stuff is done and before those other things are done is a safe place
> to execute arbitrary user-provided code? I'm not. And the more
> places we add that recurse into ProcessUtility, or indeed, add event
> triggers in general, the more places we're going to add new failure
> modes.

While I hear you, I completely fail to understand which magic is going
to make your other idea safer than this one. Namely, calling an event
trigger named sql_drop rather than ddl_command_start from the same place
in the code is certainly not helping at all.

If your answer to that is how to implement it (e.g. keeping a queue of
events for which to fire Event Triggers once we reach a safe point in
the code), then why couldn't we do exactly the same thing under the
other name?

> Fixing that is part of the price of adding a new event trigger and I'm
> OK with that. But I'm not OK with baking into the code the assumption
> that any current or future callers of ProcessUtility() should be
> prepared for arbitrary code execution. The code wasn't written with
> that in mind, and it will not end well.

Again, while I agree with the general principle, it happens that the
calls to the arbitrary code are explicit in ProcessUtility(), so that if
the place is known to be unsafe it's easy enough to refrain from calling
the user code. We do that already for CREATE INDEX CONCURRENTLY at
ddl_command_end.

> - adds additional magic variables to PL/pgsql to expose new
> information not previously exposed
>
> I'm not sure that I agree with the implementation of this, but I think
> I'm OK with the concept. I will review it when it is submitted in a
> form not intertwined with other things.

Fair enough.

> - adds CONTEXT as an additional event-trigger-filter variable, along with TAG
>
> I'm OK with adding new event-trigger-filter variables, provided that
> they are done without additional run-time overhead for people who are
> not using event triggers; I think it's nice that we can support that.
> But I think this particular proposal is DOA because nothing except
> TOPLEVEL is actually safe.

Yeah, let's first see about that.

>> My thinking is that the user code can do anything, even run a DROP
>> command against the object that we are preparing ourselves to be
>> dropping. I don't think we can bypass doing the lookup twice.
>
> Sure, but there's also the issue of getting the right answer. For
> example, suppose you are planning to drop a function. You do a name
> lookup on the name and call the event trigger. Meanwhile, someone
> else renames the function and creates a new one with the same name.
> After the event trigger returns, the actual drop code latches onto the
> one with the new name. Now, the event trigger ran with OID A and the
> actual object dropped was one with OID B. It's possible that KaiGai's

Is your scenario even possible when we take a ShareUpdateExclusiveLock
on he object before running the Event Trigger, in order to protect
against exactly that scenario, as said in the comment? Maybe another
lock should be taken. Which one?

> There's another issue here, too, which is that this change
> reintroduces a failure mode that I spent a lot of time cleaning up
> during the 9.2 cycle - namely, taking locks on a table before
> permissions have been checked, thus providing new methods for
> unprivileged users to block operations on tables they don't have
> rights to.

Aren't Event Triggers superuser only? Do your concern means that we
should force Event Triggers Functions to be SECURITY DEFINER?

>> - the parse tree, only available to C code, and without any API
>> stability promises
>
> I'm OK with that. People who program in C are taking their life in
> their hands anyway as far as new versions are concerned.

Cool.

> What I'm proposing is that we have a separate set of events that get
> invoked every time an object is created, altered, or dropped,
> regardless of the cause. Yes, the name will be different, but the
> point is not to only change the name, but rather to also change the
> definition. There are two separate things you might want here:

In the rest of your email, I can't decide what kind of information you
want those new events to give to the Event Trigger User Code. The other
thing is, as I said before, that I can't see why those new events would
suddenly make the call point safer.

Other than that, I must say that your proposal is sound to me and that I
like it too. I just fail to understand how it makes any difference other
than in naming things, really.

Why would a command that the system executes for you has to be
considered such a different beast from the same command when you happen
to have sent explicitely? A GENERATED context sounds better to me than a
"sql_create" event as a user interface, obviously, and implementing the
later has the *exact* same set of challenges to address, as far as I
understand.

> For example, suppose a user executes the command "DROP OWNED BY fred".
> For a logging hook, you want the first thing: one callback, saying,
> hey, the user is trying to run DROP OWNED BY fred. You log it once,
> and go home. For a replication hook, you want the second thing: one

Maybe it's only me, but I'm really annoyed that there's currently no way
to get the list of objects actually dropped other than parsing the logs.
So I know that I would want my audit trigger to see them all.

> callback per object, with details about each one, so you can drop them
> on the remote side. Both things are useful, but they are different.

Sure. And you have exactly that feature in my patch, where by default
CONTEXT is TOPLEVEL and you see only the main command, but you can also
register against both TOPLEVEL and GENERATED, say CASCADE in the future,
and have all the details. Or only register against CASCADE, too.

> As a further example, suppose that in 9.4 (or 9.17) we add a command
> "DROP TABLES IN SCHEMA fred WHERE name LIKE 'bob%'". Well, the
> logging trigger still just works (because it's only writing the
> statement, without caring about its contents). And the replication
> trigger still just works too (because it will still get a callback for
> every table that gets dropped, even though it knows nothing about the
> new syntax). That's very powerful. Of course, there will still be
> cases where things have to change, but you can minimize it by clean
> definitions.

Your use case here is exactly the reason why we proposed to have the
main DROP command implemented via ProcessUtility, so that an Event
Trigger called with a CASCADE context works the same way whatever the
TOPLEVEL command looks like, and always against a single object.

> It pains me that I've evidently failed to communicate this concept
> clearly despite a year or more of trying. Does that make sense? Is
> there some way I can make this more clear? The difference seems very
> clear-cut to me, but evidently I'm not conveying it well.

Communication is hard, and apparently even more so for computer-people,
or they wouldn't prefer to spend their jobs talking to a computer rather
than talking to people. I've heard that so many times that it almost
could make it true, sometimes…

>> After all, the whole point of the Event Trigger patch is to expose some
>> level of implementation details.
>
> I don't really agree with that. I think the point is to expose what
> the system is doing to the DBA. I'm OK with exposing the fact that
> creating a table with a serial column also creates a sequence. There
> is no problem with that - it's all good. What we DON'T want to do is
> set up a system where the fact that it creates a sequence is exposed
> because that happens to go through ProcessUtility() and the fact that
> it creates a constraint is not because that happens not to go through
> ProcessUtility(). That is not the sort of quality that our users have
> come to expect from PostgreSQL.

I see what you mean. I don't see any CREATE CONSTRAINT command in the
following reference, though.

http://www.postgresql.org/docs/9.2/static/sql-commands.html

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

On 17 January 2013 16:15, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> As a further example, suppose that in 9.4 (or 9.17) we add a command
> "DROP TABLES IN SCHEMA fred WHERE name LIKE 'bob%'". Well, the
> logging trigger still just works (because it's only writing the
> statement, without caring about its contents). And the replication
> trigger still just works too (because it will still get a callback for
> every table that gets dropped, even though it knows nothing about the
> new syntax). That's very powerful. Of course, there will still be
> cases where things have to change, but you can minimize it by clean
> definitions.
>
> It pains me that I've evidently failed to communicate this concept
> clearly despite a year or more of trying. Does that make sense? Is
> there some way I can make this more clear? The difference seems very
> clear-cut to me, but evidently I'm not conveying it well.
>
>> That problem is going to happen in some way or another. It's not about
>> avoiding the problem completely, but choosing a set of compromises. I'm
>> yet to understand what your set of compromises is in detailed enough
>> practical implementation terms (not just "it's another set of events")
>> and how it's a better compromise over all (I trust your judgement on
>> that, I still want to understand).
>>
>> After all, the whole point of the Event Trigger patch is to expose some
>> level of implementation details.
>
> I don't really agree with that. I think the point is to expose what
> the system is doing to the DBA. I'm OK with exposing the fact that
> creating a table with a serial column also creates a sequence. There
> is no problem with that - it's all good. What we DON'T want to do is
> set up a system where the fact that it creates a sequence is exposed
> because that happens to go through ProcessUtility() and the fact that
> it creates a constraint is not because that happens not to go through
> ProcessUtility(). That is not the sort of quality that our users have
> come to expect from PostgreSQL.

"The point", i.e. the main use case, is to replicate the DDL in a useful form.

Discussions and reasoning need to focus on the main use case, not on
weird futures or qualitative points. Yes, the discussion has gone on
for years and it really should come to a useful conclusion sometime
soon.

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

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 17 January 2013 16:15, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> It pains me that I've evidently failed to communicate this concept
>> clearly despite a year or more of trying. Does that make sense? Is
>> there some way I can make this more clear? The difference seems very
>> clear-cut to me, but evidently I'm not conveying it well.

> "The point", i.e. the main use case, is to replicate the DDL in a useful form.

> Discussions and reasoning need to focus on the main use case, not on
> weird futures or qualitative points.

I have to completely disagree with that. If we don't want PostgreSQL
to soon subside into an unfixable morass, as I think Brooks puts it,
we must *not* simply patch things in a way that expediently provides
an approximation of some desired feature. We have to consider, and put
substantial weight on, having a clean and maintainable system design.

This is particularly the case if we're talking about a feature that
is going to expose backend internals to users to any degree. We're
either going to be constrained to not change those internals any more,
or expect to break users' applications when we do change them, and
neither result is very appealing. Especially not when we're talking
about the structure of Postgres' DDL code, which can most charitably
be described as "it just grew", not as something that has any clear
architecture to it.

Now if we could quantify these things, that would be great, but
AFAICS "qualitative points" is all we've got to go on.

I think that we're not realistically going to be able to introduce
event triggers in very many of the places we'd like to have them
without first doing a lot of fundamental refactoring. And that is
hard, dangerous, slow work that tends to break things in itself.
As we've been repeatedly reminded in connection with KaiGai-san's
refactoring patches.

So my opinion is that most of what we'd like to have here is material
for 9.4 or 9.5 or even further out. If we can get the event trigger
catalog infrastructure and some *very* basic events, like
end-of-toplevel-command, into place for 9.3, we'll have done well.

regards, tom lane

On Thu, Jan 17, 2013 at 12:06 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Ok. Will prepare a non controversial patch for ddl_command_end.

Thanks. I will make a forceful effort to review that in a timely
fashion when it's posted.

>> I think this is a bad idea, not only because, as I said before, it
>> exposes internal implementation details, but also because it's
>> probably not safe. As Noah (I believe, or maybe Tom), observed in a
>> previous post, we've gone to a lot of work to make sure that nothing
>> you do inside a trigger can crash the server, corrupt the catalogs,
>> trigger elog() messages, etc. That applies in spades to DDL.
>
> I naively though that doing CommandCounterIncrement() before running the
> event trigger would go a long enough way towards making the operation
> safe when the current code is calling back into ProcessUtility().

It's something, but I'd be shocked if it covers all the bases.

Let me try to give a concrete example of how I think another firing
point could be made to work along the lines I'm suggesting. I'm not
going to use DROP as an example because I think upon reflection that
will be a particularly challenging case to make work correctly.
Instead, let me use the example of ALTER.

Goal: Every time an ALTER command is used on object *that actually
exists*, we will call some user-defined function and pass the object
type, the OID of the object, and some details about what sort of
alteration the user has requested.

Where shall we put the code to do this? Clearly, ProcessUtility is
too early, because at that point we have not done the name lookup,
locked the object, or checked permissions. So the OID of the target
object is not and cannot be known at that point. We cannot do an
extra name lookup at that point without taking a lock, because the
answer might change, or, due to non-MVCC catalog access, the lookup
might fail to find an object altogether. And we can't take a lock
without checking permissions first. And the permissions-checking
logic can't be done inside ProcessUtility(), because it's object-type
specific and we don't want to duplicate it.

So, instead, what we need to do is go into each function that
implements ALTER, and modify it so that after it does the dance where
we check permissions, take locks, and look up names, we call the
trigger function. That's a bit of a nuisance, because we probably
have a bunch of call sites to go fix, but in theory it should be
doable. The problem of course, is that it's not intrinsically safe to
call user-defined code there. If it drops the object and creates a
new one, say, hilarity will probably ensue.

But that should be a fixable problem. The only things we've done at
this point are check permissions, lock, and look up names. I think we
can assume that if the event trigger changes permissions, it's safe
for the event trigger to ignore that. The trigger cannot have
released the lock, so we're OK there. The only thing it can really
have done that's problematic is change the results of the name lookup,
say by dropping or renaming the object. But that's quite simple to
protect against: after firing the trigger, we do a
CommandCounterIncrement() and repeat the name lookup, and if we get a
different answer, then we bail out with an error and tell the user
they're getting far too cute. Then we're safe.

Now, there is a further problem: all of the information about the
ALTER statement that we want to provide to the trigger may not be
available at that point. Simple cases like ALTER .. RENAME won't
require anything more than that, but things like ALTER TABLE .. ADD
FOREIGN KEY get tricky, because while at this point we have a solid
handle on the identity of the relation to which we're adding the
constraint, we do NOT yet have knowledge of or a lock on the TARGET
relation, whose OID could still change on us up to much later in the
computation. To get reliable information about *that*, we'll need to
refactor the sequence of events inside ALTER TABLE.

Hopefully you can see what I'm driving toward here. In a design like
this, we can pretty much prove - after returning from the event
trigger - that we're in a state no different from what would have been
created by executing a series of commands - lock table, then SELECT
event_trigger_func(), then the actual ALTER in sequence. Maybe
there's a flaw in that analysis - that's what patch review is for -
but it sounds pretty darn tight to me.

I could write more, but I have to take the kids to dance class, and
this email may be too long already. Does that help at all to clarify
the lines along which I am thinking? Note that the above is clearly
not ddl_command_start but something else, and the different firing
point and additional safety cross-checks are what enable us to pass
additional information to the trigger without fear of breaking either
the trigger or the world.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Goal: Every time an ALTER command is used on object *that actually
> exists*, we will call some user-defined function and pass the object
> type, the OID of the object, and some details about what sort of
> alteration the user has requested.

Ok, in current terms of the proposed patch, it's a ddl_command_end event
trigger, and you can choose when to fire it in utility.c. The current
place is choosen to be just after the AlterTable() call, because that
sounded logical if you believe in CommandCounterIncrement.

> So, instead, what we need to do is go into each function that
> implements ALTER, and modify it so that after it does the dance where
> we check permissions, take locks, and look up names, we call the
> trigger function. That's a bit of a nuisance, because we probably
> have a bunch of call sites to go fix, but in theory it should be
> doable. The problem of course, is that it's not intrinsically safe to
> call user-defined code there. If it drops the object and creates a
> new one, say, hilarity will probably ensue.

You're trying to find a dangerous point when to fire the trigger here,
rather than trying to solve the problem you're describing. For the
problem you're describing, either you want the Object Specific
Information and the trigger fires at ddl_command_end, or you don't and
you can register your trigger at ddl_command_start.

If you want something else, well, it won't ship in 9.3.

> Now, there is a further problem: all of the information about the
> ALTER statement that we want to provide to the trigger may not be
> available at that point. Simple cases like ALTER .. RENAME won't
> require anything more than that, but things like ALTER TABLE .. ADD
> FOREIGN KEY get tricky, because while at this point we have a solid
> handle on the identity of the relation to which we're adding the
> constraint, we do NOT yet have knowledge of or a lock on the TARGET
> relation, whose OID could still change on us up to much later in the
> computation. To get reliable information about *that*, we'll need to
> refactor the sequence of events inside ALTER TABLE.

The only valid answer here has already been given by Tom. You can only
provide the information if it's available in the catalogs. So again,
it's a ddl_command_end event. It can not happen before.

> Hopefully you can see what I'm driving toward here. In a design like
> this, we can pretty much prove - after returning from the event
> trigger - that we're in a state no different from what would have been
> created by executing a series of commands - lock table, then SELECT
> event_trigger_func(), then the actual ALTER in sequence. Maybe
> there's a flaw in that analysis - that's what patch review is for -
> but it sounds pretty darn tight to me.

The only case when we need to do a lookup BEFORE actually running the
command is when that command is a DROP, because that's the only timing
when the information we want is still in the catalogs.

So that's the only case where we do a double object lookup, and we take
a ShareUpdateExclusiveLock lock on the object when doing so, so that we
can't lose it from another concurrent activity.

> Note that the above is clearly
> not ddl_command_start but something else, and the different firing
> point and additional safety cross-checks are what enable us to pass
> additional information to the trigger without fear of breaking either
> the trigger or the world.

That's the reason why we are only going to have ddl_command_start and
ddl_command_end in 9.3, and also the reason why the extra information is
only provided when this very information still exists in the catalogs at
the moment when we want to expose it.

And that's the reason why ddl_command_trace is attractive too: you won't
ever get Object OID and Name and Schema from the event where the object
does not exists, and maybe you want an easy way to tell the system
you're interested into an event *with* the information, and be done,
don't think.

Again, I don't care much about keeping ddl_command_trace because it's
not interesting much for implementing DDL support in logical
replication and my other use cases, but still, I though I would explain
it now that we're talking about it.

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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> I have to completely disagree with that. If we don't want PostgreSQL
> to soon subside into an unfixable morass, as I think Brooks puts it,
> we must *not* simply patch things in a way that expediently provides
> an approximation of some desired feature. We have to consider, and put
> substantial weight on, having a clean and maintainable system design.

When in Ottawa next may, I will have to buy you a glass of your favorite
wine and explain to you my main use case and vision. You will then
realize that all this time that I've been spending on Event Triggers is
a sideway to build something else entirely, because I know that it's the
only way to get the feature I want in core PostgreSQL.

So yes, I know about the clean and maintainable system design
constraint. I've a lot to learn still, and I appreciate your help very
much. Rest assured that the path you describe is the one I'm taking.

> I think that we're not realistically going to be able to introduce
> event triggers in very many of the places we'd like to have them
> without first doing a lot of fundamental refactoring.

We're only talking about ddl_command_start and ddl_command_end now. The
table_rewrite event is still on the horizon, but is not realistic to get
in 9.3 anymore, I think :(

So we're talking about adding some call points only in utility.c, only
before or after a ddl command is run, including nested sub-commands.

> So my opinion is that most of what we'd like to have here is material
> for 9.4 or 9.5 or even further out. If we can get the event trigger
> catalog infrastructure and some *very* basic events, like
> end-of-toplevel-command, into place for 9.3, we'll have done well.

My feeling is that I'm sending patches that only implement the *very*
basic events here, and nothing more. Most of the heat of this thread
came from a discussion about a feature that's very hard to design and
that is not in my patch series to review.

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

On 17 January 2013 20:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On 17 January 2013 16:15, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> It pains me that I've evidently failed to communicate this concept
>>> clearly despite a year or more of trying. Does that make sense? Is
>>> there some way I can make this more clear? The difference seems very
>>> clear-cut to me, but evidently I'm not conveying it well.
>
>> "The point", i.e. the main use case, is to replicate the DDL in a useful form.
>
>> Discussions and reasoning need to focus on the main use case, not on
>> weird futures or qualitative points.
>
> I have to completely disagree with that. If we don't want PostgreSQL
> to soon subside into an unfixable morass, as I think Brooks puts it,
> we must *not* simply patch things in a way that expediently provides
> an approximation of some desired feature. We have to consider, and put
> substantial weight on, having a clean and maintainable system design.

So why does focusing on a use case make this turn into an unfixable
morass? The two things are completely unrelated.

If the patch is anywhere close to an unfixable morass, let's just
reject it now. But Robert's arguments were much more tenuous than
that.

My comments were in response to this

> I don't really agree with that. I think the point is to expose what
> the system is doing to the DBA. I'm OK with exposing the fact that
> creating a table with a serial column also creates a sequence. There
> is no problem with that - it's all good. What we DON'T want to do is
> set up a system where the fact that it creates a sequence is exposed
> because that happens to go through ProcessUtility() and the fact that
> it creates a constraint is not because that happens not to go through
> ProcessUtility(). That is not the sort of quality that our users have
> come to expect from PostgreSQL.

The above functionality is sufficient to allow DDL replication. What
else do we want to do that requires some other capability?

Discussing things in terms of what we will actually use a feature for
is how we know we've got it right. And if it does that, we don't need
anything else.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Let me try to give a concrete example of how I think another firing
> point could be made to work along the lines I'm suggesting.
> [ snip description of how an event trigger might safely be fired just
> after identification and locking of the target object for an ALTER ]

Reading that and reflecting on my gripe about the lack of any clear
architecture for our DDL code, I had the beginnings of an idea.

Perhaps it would improve matters if we refactored DDL processing so that
there were separate "parse analysis" and "execution" phases, where parse
analysis is (perhaps among other responsibilities) responsible for
identifying and locking all objects to be used in the command. I note
that locking the referenced tables is the responsibility of the parse
analysis step in DML processing, so there's solid precedent for this.
Also, we have some of this approach already for certain commands such
as CREATE TABLE, cf parse_utilcmd.c.

If we did that, then it'd be feasible to fire event triggers after the
parse analysis step, and the rechecking that Robert describes could be
encapsulated as "redo the parse analysis and see if the result changed".

It's not clear to me just how this ought to extend to the cascaded-DROP
or inherited-table-ALTER cases, but hey, it's only the beginnings of
an idea.

regards, tom lane

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> I think that we're not realistically going to be able to introduce
>> event triggers in very many of the places we'd like to have them
>> without first doing a lot of fundamental refactoring.

> We're only talking about ddl_command_start and ddl_command_end now. The
> table_rewrite event is still on the horizon, but is not realistic to get
> in 9.3 anymore, I think :(

> So we're talking about adding some call points only in utility.c, only
> before or after a ddl command is run, including nested sub-commands.

Well, that's already a problem, because as Robert keeps saying, what
goes through utility.c and what doesn't is pretty random right at the
moment, and we shouldn't expose that behavior to users for fear of not
being able to change it later.

I think we could possibly ship event triggers defined as start and end
of a *top level* command and have them working reliably in 9.3. If you
want users to be looking at subcommands, there is a lot more work to do
there than we have any chance of getting done for 9.3 (if it is to ship
in 2013, that is).

Alternatively, if you want to get something into 9.3 that has not
necessarily got a long-term-stable API, I'd be inclined to suggest that
we forget about a SQL-level event trigger facility for now, and just put
in some hook call points. It's pretty well established that we're
willing to change the API seen by hook functions across major releases.

TBH this might be the best thing anyway if your long-term goals have to
do with replication, because it'd be a lot cheaper to get to the point
where you could write the replication code and see if it all actually
works. I'm fairly concerned that we might spend many man-months getting
event triggers with definitions A,B,C into the system, only to find out
later that what is really needed for logical replication is definitions
D,E,F.

regards, tom lane

Simon Riggs <simon(at)2ndquadrant(dot)com> writes:
> On 17 January 2013 20:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> My comments were in response to this

>> I don't really agree with that. I think the point is to expose what
>> the system is doing to the DBA. I'm OK with exposing the fact that
>> creating a table with a serial column also creates a sequence. There
>> is no problem with that - it's all good. What we DON'T want to do is
>> set up a system where the fact that it creates a sequence is exposed
>> because that happens to go through ProcessUtility() and the fact that
>> it creates a constraint is not because that happens not to go through
>> ProcessUtility(). That is not the sort of quality that our users have
>> come to expect from PostgreSQL.

> The above functionality is sufficient to allow DDL replication. What
> else do we want to do that requires some other capability?

I think you and Robert are talking past each other. Whether it is or
is not sufficient for DDL replication is not what he is concerned about;
what he's worried about is whether we can refactor the backend in future
without causing a user-visible change in the events that event triggers
see. I think that that is an entirely valid concern, and it's not going
to go away on the strength of "but this is enough to let us do
replication".

The other point I'd make here is what I just said to Dimitri: it's far
from proven, AFAIK, that this actually *is* sufficient to allow DDL
replication. I'd be a lot happier if we had a working proof of that
before we lock down a SQL-level facility that we're going to have a hard
time changing the details of. Maybe we should just introduce some
C-level hooks and let you guys go off and code replication using the
hooks, before we put in a lot of expensive infrastructure that might
turn out to be Not Quite The Right Thing. After we *know* the hooks
are in the right places and supply the right information, we can look at
replacing them with some more formalized facility.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Well, that's already a problem, because as Robert keeps saying, what
> goes through utility.c and what doesn't is pretty random right at the
> moment, and we shouldn't expose that behavior to users for fear of not
> being able to change it later.

It didn't feel that random to me. In most cases, the rule is that if the
system wants to execute a command that you could have typed as a user in
the prompt, then it hacks together a parsenode and call ProcessUtility.

The main exception to that rule is the CASCADE implementation, for
reasons that you detailed in that thread earlier.

> I think we could possibly ship event triggers defined as start and end
> of a *top level* command and have them working reliably in 9.3. If you
> want users to be looking at subcommands, there is a lot more work to do
> there than we have any chance of getting done for 9.3 (if it is to ship
> in 2013, that is).

By default, you only get top level in what I've been cooking. I think
the other contexts are needed for the logical replication, and maybe it
would be good enough if they are restricted to either only internal code
or event triggers coded in C. What do you think?

> Alternatively, if you want to get something into 9.3 that has not
> necessarily got a long-term-stable API, I'd be inclined to suggest that
> we forget about a SQL-level event trigger facility for now, and just put
> in some hook call points. It's pretty well established that we're
> willing to change the API seen by hook functions across major releases.

Or just a hook. That would want to have about the exact same amount of
information as the Event Trigger anyway, so I'm thinking we could maybe
do that the same way as the parsetree exposure?

Another idea would be yet another GUC, ala allow_system_table_mods, so
that only intrepid users can have at it. Well, as long as the logical
replication use case is covered, I'm done here, so I want to hear from
Simon and Andres on that (and maybe the Slony and Londiste guys, etc),
and from you to triage what is possible and what is crazy.

> TBH this might be the best thing anyway if your long-term goals have to
> do with replication, because it'd be a lot cheaper to get to the point
> where you could write the replication code and see if it all actually
> works. I'm fairly concerned that we might spend many man-months getting
> event triggers with definitions A,B,C into the system, only to find out
> later that what is really needed for logical replication is definitions
> D,E,F.

I'm worried about that too, and that's why I'm trying to remain general
and quite transparent about the way the system actually works.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> Well, that's already a problem, because as Robert keeps saying, what
>> goes through utility.c and what doesn't is pretty random right at the
>> moment, and we shouldn't expose that behavior to users for fear of not
>> being able to change it later.

> It didn't feel that random to me. In most cases, the rule is that if the
> system wants to execute a command that you could have typed as a user in
> the prompt, then it hacks together a parsenode and call ProcessUtility.

Uh, no, there's no such "rule". In some places it was convenient to do
that so people did it --- but often it's easier to just call the
appropriate function directly, especially if you have any special
locking or permissions requirements. I don't think there's any great
consistency to it.

To be a little more concrete, I looked through backend/catalog and
backend/commands, which ought to pretty much cover all the places where
commands do things that could be considered subcommands. I find three
uses of ProcessUtility:

extension.c: uses ProcessUtility to handle non-DML commands read from an
extension script.

schemacmds.c: CreateSchemaCommand uses ProcessUtility for any subcommand
of a CREATE SCHEMA statement. This is really just direct execution of
something the user typed, it's not the system "creating" a subcommand.

trigger.c: ConvertTriggerToFK uses ProcessUtility to execute a consed-up
ALTER TABLE ADD FOREIGN KEY command in place of a series of legacy CREATE
CONSTRAINT TRIGGER commands. Now this is the very definition of ugly,
and shouldn't drive our design decisions --- but I think that any user
event trigger would be darn surprised to find this being emitted,
especially when the two preceding CREATE CONSTRAINT TRIGGERs hadn't done
any such thing, and indeed hadn't created any constraint trigger.

AFAICT, everything else in those directories is using direct calls and
not going through utility.c. So the only other cases where a trigger in
ProcessUtility would trap subcommands are where those subcommands are
things that parse_utilcmd.c generated during expansion of a CREATE TABLE
or such. And that whole area is something that I feel strongly is
implementation detail we don't particularly want to expose to users.
For instance, the fact that a UNIQUE clause in a CREATE TABLE works that
way and not at some lower level is nothing but implementation artifact.

[ pokes around a bit more... ] Having now actually looked at every call
point of ProcessUtility in the current code, I find myself agreeing very
much with Robert: we do *not* want to expose that exact set of events to
users. Except for the original top-level commands, it's almost entirely
implementation artifacts that can and will change from release to
release.

regards, tom lane

On Thu, Jan 17, 2013 at 4:43 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Goal: Every time an ALTER command is used on object *that actually
>> exists*, we will call some user-defined function and pass the object
>> type, the OID of the object, and some details about what sort of
>> alteration the user has requested.
>
> Ok, in current terms of the proposed patch, it's a ddl_command_end event
> trigger, and you can choose when to fire it in utility.c. The current
> place is choosen to be just after the AlterTable() call,

/me scratches my head.

Depends. What I described would fire before the command was executed,
not after, so it's not quite the same thing, but it might be just as
good for your purposes. It wasn't so much intended as "we should do
this exact thing" as much as "this is the kind of thought process that
you need to go through if you want to safely run an event trigger in
the middle of another command".

Now, maybe we don't actually need to do that to serve your purposes.
As already discussed, it seems like passing information to
ddl_command_end is for most purposes sufficient for what you need -
and we can do that without any special gymnastics, because that way it
runs completely after the command, not in the middle. If I understand
correctly, and I may not, there are basically two problems with that
approach:

(1) It's not exactly clear how to pass the information about the
statement through to the ddl_command_end trigger. I know you've
proposed a couple of things, but none of them seem completely
satisfying. At least not to me.

(2) If the method of transmission is "the OIDs of the affected
objects", which I believe is your current proposal, the
ddl_command_end trigger can't go look those objects up in the catalog,
because the catalog entries are already gone by that point.

It's possible we could solve both of those problems without running
event triggers in the middle of commands at all. In which case, my
whole example is moot for now. But I think it would be smart to get
ddl_command_end (without any additional information) committed first,
and then argue about these details, so that we at least make some
definable progress here.

> because that
> sounded logical if you believe in CommandCounterIncrement.

I'm somewhat bemused by this comment. I don't think
CommandCounterIncrement() is an article of faith. If you execute a
command to completion, and do a CommandCounterIncrement(), then
whatever you do next will look just like a new command, so it should
be safe to run user-provided code there. But if you stop in the
middle of a command, do a CommandCounterIncrement(), run some
user-provided code, and then continue on, the
CommandCounterIncrement() by itself protects you from nothing. If the
code is not expecting arbitrary operations to be executed at that
point, and you execute them, you get to keep both pieces. Indeed,
there are some places in the code where inserting a
CommandCounterIncrement() *by itself* could be unsafe. I don't
believe that's a risk in ProcessUtility(), but that doesn't mean there
aren't any risks in ProcessUtility().

>> So, instead, what we need to do is go into each function that
>> implements ALTER, and modify it so that after it does the dance where
>> we check permissions, take locks, and look up names, we call the
>> trigger function. That's a bit of a nuisance, because we probably
>> have a bunch of call sites to go fix, but in theory it should be
>> doable. The problem of course, is that it's not intrinsically safe to
>> call user-defined code there. If it drops the object and creates a
>> new one, say, hilarity will probably ensue.
>
> You're trying to find a dangerous point when to fire the trigger here,

Yes, I am. I'm not asking you to implement what I proposed there -
I'm just giving an example of how to make a dangerous place safe.
You've ALSO found a dangerous point when to fire the trigger. The
difference is that my example dangerous point is probably fixable to
be safe, whereas your actual dangerous point is probably unfixable,
because there are many current paths of execution that go through that
function in an extremely ad-hoc fashion, and there may be more in the
future. ddl_command_start and ddl_command_end are safe enough *when
restricted to toplevel commands*, but that's about as far as it goes.
Perhaps there are other special cases that are also safe, but just
throwing everything into the pot with no analysis and no
future-proofing certainly isn't.

>> Now, there is a further problem: all of the information about the
>> ALTER statement that we want to provide to the trigger may not be
>> available at that point. Simple cases like ALTER .. RENAME won't
>> require anything more than that, but things like ALTER TABLE .. ADD
>> FOREIGN KEY get tricky, because while at this point we have a solid
>> handle on the identity of the relation to which we're adding the
>> constraint, we do NOT yet have knowledge of or a lock on the TARGET
>> relation, whose OID could still change on us up to much later in the
>> computation. To get reliable information about *that*, we'll need to
>> refactor the sequence of events inside ALTER TABLE.
>
> The only valid answer here has already been given by Tom. You can only
> provide the information if it's available in the catalogs. So again,
> it's a ddl_command_end event. It can not happen before.

I don't see why you say that. It's perfectly possible to have that
information available at the right time; the work just hasn't been
done yet.

>> Hopefully you can see what I'm driving toward here. In a design like
>> this, we can pretty much prove - after returning from the event
>> trigger - that we're in a state no different from what would have been
>> created by executing a series of commands - lock table, then SELECT
>> event_trigger_func(), then the actual ALTER in sequence. Maybe
>> there's a flaw in that analysis - that's what patch review is for -
>> but it sounds pretty darn tight to me.
>
> The only case when we need to do a lookup BEFORE actually running the
> command is when that command is a DROP, because that's the only timing
> when the information we want is still in the catalogs.
>
> So that's the only case where we do a double object lookup, and we take
> a ShareUpdateExclusiveLock lock on the object when doing so, so that we
> can't lose it from another concurrent activity.

As noted, I don't think that's sufficient to really guarantee no shenanigans.

One idea would be to save off all the names of the objects actually
dropped at the time the drops are done, and then pass it to the event
trigger afterward. But frankly I think that's way beyond the scope of
what we should try to get done for 9.3.

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

On Thu, Jan 17, 2013 at 5:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Perhaps it would improve matters if we refactored DDL processing so that
> there were separate "parse analysis" and "execution" phases, where parse
> analysis is (perhaps among other responsibilities) responsible for
> identifying and locking all objects to be used in the command. I note
> that locking the referenced tables is the responsibility of the parse
> analysis step in DML processing, so there's solid precedent for this.
> Also, we have some of this approach already for certain commands such
> as CREATE TABLE, cf parse_utilcmd.c.
>
> If we did that, then it'd be feasible to fire event triggers after the
> parse analysis step, and the rechecking that Robert describes could be
> encapsulated as "redo the parse analysis and see if the result changed".
>
> It's not clear to me just how this ought to extend to the cascaded-DROP
> or inherited-table-ALTER cases, but hey, it's only the beginnings of
> an idea.

This is pretty much where I think we should be headed, long term. I
believe it would help with the previously-stated desire to have
EXPLAIN ALTER TABLE and similar, too. I am not sure it's necessary to
go this far to get some of what Dimitri wants, but it may be a
necessary prerequisite to getting all of it.

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

On 2013-01-17 23:48:23 +0100, Dimitri Fontaine wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> > Alternatively, if you want to get something into 9.3 that has not
> > necessarily got a long-term-stable API, I'd be inclined to suggest that
> > we forget about a SQL-level event trigger facility for now, and just put
> > in some hook call points. It's pretty well established that we're
> > willing to change the API seen by hook functions across major releases.
>
> Or just a hook. That would want to have about the exact same amount of
> information as the Event Trigger anyway, so I'm thinking we could maybe
> do that the same way as the parsetree exposure?
>
> Another idea would be yet another GUC, ala allow_system_table_mods, so
> that only intrepid users can have at it. Well, as long as the logical
> replication use case is covered, I'm done here, so I want to hear from
> Simon and Andres on that (and maybe the Slony and Londiste guys, etc),
> and from you to triage what is possible and what is crazy.

> > TBH this might be the best thing anyway if your long-term goals have to
> > do with replication, because it'd be a lot cheaper to get to the point
> > where you could write the replication code and see if it all actually
> > works. I'm fairly concerned that we might spend many man-months getting
> > event triggers with definitions A,B,C into the system, only to find out
> > later that what is really needed for logical replication is definitions
> > D,E,F.

I have no problem requiring C code to use the even data, be it via hooks
or via C functions called from event triggers. The problem I have with
putting in some hooks is that I doubt that you can find sensible spots
with enough information to actually recreate the DDL for a remote system
without doing most of the work for command triggers.

Greetings,

Andres Freund

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

On 01/18/2013 04:24 AM, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On 17 January 2013 16:15, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> It pains me that I've evidently failed to communicate this concept
>>> clearly despite a year or more of trying. Does that make sense? Is
>>> there some way I can make this more clear? The difference seems very
>>> clear-cut to me, but evidently I'm not conveying it well.
>> "The point", i.e. the main use case, is to replicate the DDL in a useful form.
>> Discussions and reasoning need to focus on the main use case, not on
>> weird futures or qualitative points.
> I have to completely disagree with that. If we don't want PostgreSQL
> to soon subside into an unfixable morass, as I think Brooks puts it,
> we must *not* simply patch things in a way that expediently provides
> an approximation of some desired feature. We have to consider, and put
> substantial weight on, having a clean and maintainable system design.
>
> This is particularly the case if we're talking about a feature that
> is going to expose backend internals to users to any degree. We're
> either going to be constrained to not change those internals any more,
> or expect to break users' applications when we do change them, and
> neither result is very appealing.
I actually wonder if - for now - breaking applications that use certain
limited well defined features is an acceptable outcome in exchange for
getting a usable first version in place. I'm approaching this from more
of an end user PoV: meeting user needs with the least possible pain to
users and to the Pg team.

A non-forward compatible feature wouldn't be OK to rely on in end user
applications, but it'd potentially be usable (if not ideal) for things
like replication tools, where practically anything is better than the
current situation with DDL. They could special case their integration
code by server version if they had to, adding version-specific triggers
and handling code. The user application wouldn't care about the changes,
they'd just need to make sure they upgraded Slony (or whatever) whenever
they upgraded to a new Pg major version.

I'm not suggesting that as a long term measure, but as a possible
stop-gap that'd:

- Allow replication to start using these features now, so they'd be
ready and more mature when the final version is available; and
- Permit prototyping of applications to use the new replication features

What I'm thinking about is packaging the client interface in an
extension that's explicitly documented as not cross-version compatible
and that must be explicitly loaded by named version in order to get the
feature. This would require a function-like interface for setup; if
that's not possible, an alternative might be a GUC that must be
explicitly set in postgresql.conf (only, not via SET) like
"enable_non_forward_compatible_features" or "enable_experimental_features".

I suspect that if this feature goes in, even in an ugly form, it'll help
guide the development of a final version concurrently with the cleanups
and refactoring that appears to be necessary.

Note that non-forward-compatible changes have been made repeatedly in
the system catalogs, despite the fact that end user applications are
known to use them via JDBC drivers and libpq, as well as directly. I
wonder if we can give this the same kind of compatibility requirements,
where it must not break within a given major version but may break
between them.

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

On 01/18/2013 09:33 AM, Andres Freund wrote:
> I have no problem requiring C code to use the even data, be it via
> hooks or via C functions called from event triggers. The problem I
> have with putting in some hooks is that I doubt that you can find
> sensible spots with enough information to actually recreate the DDL
> for a remote system without doing most of the work for command triggers.

Is that so much of a problem if it's OK to break it between major
versions? Maybe require compilation with -DPG_NON_FORWARD_COMPATIBLE to
make the hooks visible in the headers, so nobody can claim they didn't know?

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

On 2013-01-18 10:31:28 +0800, Craig Ringer wrote:
> On 01/18/2013 09:33 AM, Andres Freund wrote:
> > I have no problem requiring C code to use the even data, be it via
> > hooks or via C functions called from event triggers. The problem I
> > have with putting in some hooks is that I doubt that you can find
> > sensible spots with enough information to actually recreate the DDL
> > for a remote system without doing most of the work for command triggers.
>
> Is that so much of a problem if it's OK to break it between major
> versions? Maybe require compilation with -DPG_NON_FORWARD_COMPATIBLE to
> make the hooks visible in the headers, so nobody can claim they didn't know?

I can't follow the connection between what you quote from my mail and
what you are saying ;)

The problem I see is providing enough information to the hooks, not
cross version compatibility. If we go the hook route I don't have the
slightest problem of breaking them in the next releases.

Greetings,

Andres Freund

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jan 17, 2013 at 4:43 PM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)fr> wrote:
>> because that
>> sounded logical if you believe in CommandCounterIncrement.

> I'm somewhat bemused by this comment. I don't think
> CommandCounterIncrement() is an article of faith. If you execute a
> command to completion, and do a CommandCounterIncrement(), then
> whatever you do next will look just like a new command, so it should
> be safe to run user-provided code there. But if you stop in the
> middle of a command, do a CommandCounterIncrement(), run some
> user-provided code, and then continue on, the
> CommandCounterIncrement() by itself protects you from nothing. If the
> code is not expecting arbitrary operations to be executed at that
> point, and you execute them, you get to keep both pieces. Indeed,
> there are some places in the code where inserting a
> CommandCounterIncrement() *by itself* could be unsafe.

I think it's quite likely that we should *not* expect that we can safely
do CommandCounterIncrement at any random place. That isn't just "x++".
It causes relcache and catcache flushes for anything affected by the
command-so-far, since it's making the command's results visible --- and
the code may not be expecting the results to become visible yet, and
almost certainly isn't expecting cache entries to disappear under it.
So a CCI could break things whether or not the event trigger itself did
anything at all.

But this is just one part of the general problem that injecting random
code mid-command is risky as hell, and isn't likely to become less so.
Frankly I don't really wish to see that happen, and don't immediately
see good end-user reasons (as opposed to structure-of-the-current-code
reasons) to need it. I'd really like to get to a point where we can
define things as happening like this:

* collect information needed to interpret the DDL command
(lookup and lock objects, etc)
* fire "before" event triggers, if any (and if so, recheck info)
* do the command
* fire "after" event triggers, if any

This might require that the command execution save aside information
that would help us fire appropriate event triggers later. But I'd much
rather pay that overhead than fire triggers mid-command just because
that's where the info is currently available.

BTW, how badly do we need the "before" case at all? If we were to
restrict our ambitions to "after" triggers, it would perhaps be
relatively simple for DDL execution to create data structures noting
what it does as it goes along, and then use those to fire "after"
triggers at the end (and provide desired info to them too). Still
a lot of new code of course, but not very much risk involved.

[ reads further... ] Robert seems to have pretty much that same idea
down at the end:

> One idea would be to save off all the names of the objects actually
> dropped at the time the drops are done, and then pass it to the event
> trigger afterward.

regards, tom lane

On 01/18/2013 10:34 AM, Andres Freund wrote:
> On 2013-01-18 10:31:28 +0800, Craig Ringer wrote:
>> On 01/18/2013 09:33 AM, Andres Freund wrote:
>>> I have no problem requiring C code to use the even data, be it via
>>> hooks or via C functions called from event triggers. The problem I
>>> have with putting in some hooks is that I doubt that you can find
>>> sensible spots with enough information to actually recreate the DDL
>>> for a remote system without doing most of the work for command triggers.
>> Is that so much of a problem if it's OK to break it between major
>> versions? Maybe require compilation with -DPG_NON_FORWARD_COMPATIBLE to
>> make the hooks visible in the headers, so nobody can claim they didn't know?
> I can't follow the connection between what you quote from my mail and
> what you are saying ;)
... in which case I probably misunderstood part of the discussion. I'm
trying to skim things fairly quickly in order to keep up and continue
working through the CF backlog.

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> I have no problem requiring C code to use the even data, be it via hooks
> or via C functions called from event triggers. The problem I have with
> putting in some hooks is that I doubt that you can find sensible spots
> with enough information to actually recreate the DDL for a remote system
> without doing most of the work for command triggers.

"most of the work"? No, I don't think so. Here's the thing that's
bothering me about that: if the use-case that everybody is worried about
is replication, then triggers of any sort are the Wrong Thing, IMO.

The difference between a replication hook and a trigger is that a
replication hook has no business changing any state of the local system,
whereas a trigger *has to be expected to change the state of the local
system* because if it has no side-effects you might as well not bother
firing it. And the fear of having to cope with arbitrary side-effects
occuring in the midst of DDL is about 80% of my angst with this whole
concept.

If we're only interested in replication, let's put in some hooks whose
contract does not allow for side-effects on the local catalogs, and be
done. Otherwise we'll be putting in man-years of unnecessary (or at
least unnecessary for this use-case) work.

regards, tom lane

On 2013-01-17 21:48:01 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > I have no problem requiring C code to use the even data, be it via hooks
> > or via C functions called from event triggers. The problem I have with
> > putting in some hooks is that I doubt that you can find sensible spots
> > with enough information to actually recreate the DDL for a remote system
> > without doing most of the work for command triggers.

> "most of the work"? No, I don't think so. Here's the thing that's
> bothering me about that: if the use-case that everybody is worried about
> is replication, then triggers of any sort are the Wrong Thing, IMO.

> The difference between a replication hook and a trigger is that a
> replication hook has no business changing any state of the local system,
> whereas a trigger *has to be expected to change the state of the local
> system* because if it has no side-effects you might as well not bother
> firing it. And the fear of having to cope with arbitrary side-effects
> occuring in the midst of DDL is about 80% of my angst with this whole
> concept.

I *personally* would be *very* happy to get the capability to do two
things:
a) get enough information to replicate DDL to another system
b) raise an error that aborts the current action

Imo b) fits well enough with not allowing arbitrary side effects.

> If we're only interested in replication, let's put in some hooks whose
> contract does not allow for side-effects on the local catalogs, and be
> done. Otherwise we'll be putting in man-years of unnecessary (or at
> least unnecessary for this use-case) work.

Its a thing of perspective I guess. I can't imagine a hook-ey solution,
without quite a bit of work, that gets enough information to regenerate
SQL that performs the same action on another system. ISTM that the
refactoring to make that consistently "easy" is the hard part, but I
hope I am wrong.

Greetings,

Andres Freund

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-01-17 21:48:01 -0500, Tom Lane wrote:
>> If we're only interested in replication, let's put in some hooks whose
>> contract does not allow for side-effects on the local catalogs, and be
>> done. Otherwise we'll be putting in man-years of unnecessary (or at
>> least unnecessary for this use-case) work.

> Its a thing of perspective I guess. I can't imagine a hook-ey solution,
> without quite a bit of work, that gets enough information to regenerate
> SQL that performs the same action on another system. ISTM that the
> refactoring to make that consistently "easy" is the hard part, but I
> hope I am wrong.

The problem of how to get the information needed is real and large,
I agree. But that's not any easier for a trigger --- if anything,
it's probably harder, because then you not only need to *get* the
information but you have to figure out how to provide it in a way
that makes sense at SQL level.

regards, tom lane

On Thu, Jan 17, 2013 at 8:33 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> I have no problem requiring C code to use the even data, be it via hooks
> or via C functions called from event triggers. The problem I have with
> putting in some hooks is that I doubt that you can find sensible spots
> with enough information to actually recreate the DDL for a remote system
> without doing most of the work for command triggers.

It should be noted that the point of KaiGai's work over the last three
years has been to solve exactly this problem. Well, KaiGai wants to
check security rather than do replication, but he wants to be able
grovel through the entire node tree and make security decisions based
on stuff core PG doesn't care about, so in effect the requirements are
identical. Calling the facility "event triggers" rather than "object
access hooks" doesn't make the underlying problem any easier to solve.
I actually believe that the object access hook stuff is getting
pretty close to a usable solution if you don't mind coding in C, but
I've had trouble convincing anyone else of that.

I find it a shame that it hasn't been taken more seriously, because it
really does solve the same problem. sepgsql, for example, has no
trouble at all checking permissions for dropped objects. You can't
call procedural code from the spot where we've got that hook, but you
sure can call C code, with the usual contract that if it breaks you
get to keep both pieces. The CREATE stuff works fine too. Support
for ALTER is not all there yet, but that's because it's a hard
problem.

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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I'm somewhat bemused by this comment. I don't think
>> CommandCounterIncrement() is an article of faith. If you execute a
>> command to completion, and do a CommandCounterIncrement(), then
>> whatever you do next will look just like a new command, so it should
>> be safe to run user-provided code there. But if you stop in the

Exactly my thinking.

>> middle of a command, do a CommandCounterIncrement(), run some
>> user-provided code, and then continue on, the
>> CommandCounterIncrement() by itself protects you from nothing. If the

Yes. That's why I've been careful not to add CommandCounterIncrement()
when adding the Event Trigger facilities. The only one we added is so
that the next trigger sees what the previous one just did. That's all.

So the patch is not adding CCI calls to pretend that an existing place
in the code is now safe, it's relying on CCI as existing protocol in the
implementation to denote existing places in the code where calling user
code should be safe, because a logical block of a command just has been
done and is finished.

>> code is not expecting arbitrary operations to be executed at that
>> point, and you execute them, you get to keep both pieces. Indeed,
>> there are some places in the code where inserting a
>> CommandCounterIncrement() *by itself* could be unsafe.

I agree, it's pretty obvious. And that's why I'm not adding any.

> I think it's quite likely that we should *not* expect that we can safely
> do CommandCounterIncrement at any random place. That isn't just "x++".

Agreed. That's what the patch is doing, relying on the ones that are
already in the code, and being very careful not to add any new one.

> It causes relcache and catcache flushes for anything affected by the
> command-so-far, since it's making the command's results visible --- and
> the code may not be expecting the results to become visible yet, and
> almost certainly isn't expecting cache entries to disappear under it.
> So a CCI could break things whether or not the event trigger itself did
> anything at all.

Yes.

> But this is just one part of the general problem that injecting random
> code mid-command is risky as hell, and isn't likely to become less so.

Yes.

That's why there's nothing in the proposed patch that does that.

The user code is run either before the command starts, the event is
called ddl_command_start, and in the case of a DROP command is has to do
an extra lookup only to guarantee that we're not trying to inject user
code in a random point in the execution of the existing code.

Or the user code is run after the command is done, when it has called
itself CommandCounterIncrement() and is ready to resume execution to its
caller, and that event is called ddl_command_end.

> Frankly I don't really wish to see that happen, and don't immediately
> see good end-user reasons (as opposed to structure-of-the-current-code
> reasons) to need it. I'd really like to get to a point where we can
> define things as happening like this:
>
> * collect information needed to interpret the DDL command
> (lookup and lock objects, etc)
> * fire "before" event triggers, if any (and if so, recheck info)
> * do the command
> * fire "after" event triggers, if any

I think that if you do the lookup and lock of objects before running the
command, then you also want to do it again after the event trigger has
run, because you just don't know what it did.

That's why I implemented that way:

* no information is given to ddl_command_start event triggers

* information is collected while the code runs normally

* the ddl_command_end event has the information and only uses it if the
object still exists

Now, the extra complexity is that we also want the information at
ddl_command_start for a DROP command, so there's an explicit extra
lookup here in *only that case* and *only when some event triggers are
registered* (which is checked from a cache lookup), and only when
there's a single object targeted by the command.

And when doing that extra lookup we take a ShareUpdateExclusiveLock on
the object so that someone else won't drop it before we get to run the
Event Trigger User Function.

> This might require that the command execution save aside information
> that would help us fire appropriate event triggers later. But I'd much
> rather pay that overhead than fire triggers mid-command just because
> that's where the info is currently available.

And that's what is implemented in my patch. The information that's kept
aside is the OID of the object being the target of the command.

Last year, in the patch that didn't make it in 9.2, I used to keep the
whole EventTriggerData structure around and fill it in (it had another
name in the 9.2 era patch) from all the commands/* implementations.
Robert didn't like that, at all, because of the code foot print IIRC.

So I've been very careful in that version of the patch not to implement
things exactly as you're saying, because what you're saying looks very
much like my first approach from last year, the one that crashed and
burnt.

For full disclosure though, IIRC, the patch from that era also did try
to call Event Trigger User Code at dangerous places in the middle of the
commands implementation. That was removed before the end of the commit
fests, though, but we then had to say no for 9.2.

> BTW, how badly do we need the "before" case at all? If we were to
> restrict our ambitions to "after" triggers, it would perhaps be
> relatively simple for DDL execution to create data structures noting
> what it does as it goes along, and then use those to fire "after"
> triggers at the end (and provide desired info to them too). Still
> a lot of new code of course, but not very much risk involved.

I personally need the ddl_command_start case for implementing some
CREATE EXTENSION features in user code. And I would need to also have
the extension's name from the parsetree at that point. In the current
state of the patch, that will mean that I need a little new function
coded in C and loaded in the backend to be able to get at it.

> [ reads further... ] Robert seems to have pretty much that same idea
> down at the end:
>
>> One idea would be to save off all the names of the objects actually
>> dropped at the time the drops are done, and then pass it to the event
>> trigger afterward.

We can go even further in simplicity.

Document that you only get information about objects that exists at the
time the event trigger user function is run. Then you don't have any
specific information for a DROP command at ddl_command_end, and you
don't need any code to make it happen. You also don't have the
information at ddl_command_start for a CREATE operation.

That happens to be what my patch implements, too.

I think that the refactoring of the DDL commands in several explicit
steps of parsing, analysis then execution is the way forward, and I
didn't see any way to provide that in 1 cycle (that was my first target,
remember), and even if I had then known that it would take 2 cycles for
the limited edition of the feature, I don't think I would have called
that path realistic. As you said, this refactoring looks more like a 3
cycles effort in itself, let alone building anything on top of it.

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

On 2013-01-17 22:39:18 -0500, Robert Haas wrote:
> On Thu, Jan 17, 2013 at 8:33 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > I have no problem requiring C code to use the even data, be it via hooks
> > or via C functions called from event triggers. The problem I have with
> > putting in some hooks is that I doubt that you can find sensible spots
> > with enough information to actually recreate the DDL for a remote system
> > without doing most of the work for command triggers.
>
> It should be noted that the point of KaiGai's work over the last three
> years has been to solve exactly this problem. Well, KaiGai wants to
> check security rather than do replication, but he wants to be able
> grovel through the entire node tree and make security decisions based
> on stuff core PG doesn't care about, so in effect the requirements are
> identical. Calling the facility "event triggers" rather than "object
> access hooks" doesn't make the underlying problem any easier to solve.
> I actually believe that the object access hook stuff is getting
> pretty close to a usable solution if you don't mind coding in C, but
> I've had trouble convincing anyone else of that.
>
> I find it a shame that it hasn't been taken more seriously, because it
> really does solve the same problem. sepgsql, for example, has no
> trouble at all checking permissions for dropped objects. You can't
> call procedural code from the spot where we've got that hook, but you
> sure can call C code, with the usual contract that if it breaks you
> get to keep both pieces. The CREATE stuff works fine too. Support
> for ALTER is not all there yet, but that's because it's a hard
> problem.

I don't have a problem reusing the object access infrastructure at all. I just
don't think its providing even remotely enough. You have (co-)written that
stuff, so you probably know more than I do, but could you explain to me how it
could be reused to replicate a CREATE TABLE?

Problems I see:
- afaics for CREATE TABLE the only hook is in ATExecAddColumn
- no access to the CreateStm, making it impossible to decipher whether e.g. a
sequence was created as part of this or not
- No way to regenerate the table definition for execution on the remote system
without creating libpqdump.

Greetings,

Andres Freund

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

On Fri, Jan 18, 2013 at 9:07 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-01-17 22:39:18 -0500, Robert Haas wrote:
>> On Thu, Jan 17, 2013 at 8:33 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > I have no problem requiring C code to use the even data, be it via hooks
>> > or via C functions called from event triggers. The problem I have with
>> > putting in some hooks is that I doubt that you can find sensible spots
>> > with enough information to actually recreate the DDL for a remote system
>> > without doing most of the work for command triggers.
>>
>> It should be noted that the point of KaiGai's work over the last three
>> years has been to solve exactly this problem. Well, KaiGai wants to
>> check security rather than do replication, but he wants to be able
>> grovel through the entire node tree and make security decisions based
>> on stuff core PG doesn't care about, so in effect the requirements are
>> identical. Calling the facility "event triggers" rather than "object
>> access hooks" doesn't make the underlying problem any easier to solve.
>> I actually believe that the object access hook stuff is getting
>> pretty close to a usable solution if you don't mind coding in C, but
>> I've had trouble convincing anyone else of that.
>>
>> I find it a shame that it hasn't been taken more seriously, because it
>> really does solve the same problem. sepgsql, for example, has no
>> trouble at all checking permissions for dropped objects. You can't
>> call procedural code from the spot where we've got that hook, but you
>> sure can call C code, with the usual contract that if it breaks you
>> get to keep both pieces. The CREATE stuff works fine too. Support
>> for ALTER is not all there yet, but that's because it's a hard
>> problem.
>
> I don't have a problem reusing the object access infrastructure at all. I just
> don't think its providing even remotely enough. You have (co-)written that
> stuff, so you probably know more than I do, but could you explain to me how it
> could be reused to replicate a CREATE TABLE?
>
> Problems I see:
> - afaics for CREATE TABLE the only hook is in ATExecAddColumn

No, there's one also in heap_create_with_catalog. Took me a minute to
find it, as it does not use InvokeObjectAccessHook. The idea is that
OAT_POST_CREATE fires once per object creation, regardless of the
object type - table, column, whatever.

> - no access to the CreateStm, making it impossible to decipher whether e.g. a
> sequence was created as part of this or not

Yep, that's a problem. We could of course add additional hook sites
with relevant context information - that's what this infrastructure is
supposed to allow for.

> - No way to regenerate the table definition for execution on the remote system
> without creating libpqdump.

IMHO, that is one of the really ugly problems that we haven't come up
with a good solution for yet. If you want to replicate DDL, you have
basically three choices:

1. copy over the statement text that was used on the origin server and
hope none of the corner cases bite you
2. come up with some way of reconstituting a DDL statement based on
(a) the parse tree or (b) what the server actually decided to do
3. reconstitute the state of the object from the catalogs after the
command has run

(2a) differs from (2b) for things like CREATE INDEX, where the index
name might be left for the server to determine, but when replicating
you'd like to get the same name out. (3) is workable for CREATE but
not ALTER or DROP.

The basic problem here is that (1) and (3) are not very
reliable/complete and (2) is a lot of work and introduces a huge code
maintenance burden. But it's unfair to pin that on the object-access
hook mechanism - any reverse-parsing or catalog-deconstruction
solution for DDL is going to have that problem. The decision we have
to make as a community is whether we're prepared to support and
maintain that code for the indefinite future. Although I think it's
easy to say "yes, because DDL replication would be really cool" - and
I sure agree with that - I think it needs to be thought through a bit
more deeply than that.

I have been involved in PostgreSQL development for about 4.5 years
now. This is less time than many people here, but it's still long
enough to say a whole lot of people ask for some variant of this idea,
and yet I have yet to see anybody produce a complete, working version
of this functionality and maintain it outside of the PostgreSQL tree
for one release cycle (did I miss something?). If we pull that
functionality into core, that's just a way of taking work that
nobody's been willing to do and force the responsibility to be spread
across the whole development group. Now, sometimes it is OK to do
that, but sometimes it means that we're just bolting more things onto
the already-long list of reasons for which patches can be rejected.
Anybody who is now feeling uncomfortable at the prospect of not having
that facility in core ought to think about how they'll feel about,
say, one additional patch per year not getting committed in every
future release because of this requirement. Does that feel OK? What
if the number is two, or three? What's the tipping point where you'd
say the cost is too high? (Note: If anyone reading this is tempted to
answer that there is no such tipping point, then you have got a bad
case of patch myopia.)

There's a totally legitimate debate to be had here, but my feeling
about what you're calling libpqdump is:

- It's completely separate from event triggers.
- It's completely separate from object access hooks.
- The fact that noone to my knowledge has maintained such a thing
outside of core, and that it seems like a hard project with lots of
required maintenance, makes me very wary about pushing it into core.

[ BTW: Sorry our IM session got cut off. It started erroring out
every time I messaged you. But no problem, anyway. ]

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

On 18 January 2013 02:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> I have no problem requiring C code to use the even data, be it via hooks
>> or via C functions called from event triggers. The problem I have with
>> putting in some hooks is that I doubt that you can find sensible spots
>> with enough information to actually recreate the DDL for a remote system
>> without doing most of the work for command triggers.
>
> "most of the work"? No, I don't think so. Here's the thing that's
> bothering me about that: if the use-case that everybody is worried about
> is replication, then triggers of any sort are the Wrong Thing, IMO.
>
> The difference between a replication hook and a trigger is that a
> replication hook has no business changing any state of the local system,
> whereas a trigger *has to be expected to change the state of the local
> system* because if it has no side-effects you might as well not bother
> firing it. And the fear of having to cope with arbitrary side-effects
> occuring in the midst of DDL is about 80% of my angst with this whole
> concept.
>
> If we're only interested in replication, let's put in some hooks whose
> contract does not allow for side-effects on the local catalogs, and be
> done. Otherwise we'll be putting in man-years of unnecessary (or at
> least unnecessary for this use-case) work.

I would be happy to see something called "replication triggers" or
"replication hooks" committed.

The things I want to be able to do are:
* Replicate DDL for use on other systems, in multiple ways selected by
user, possibly >1 at same time
* Put in a block to prevent all DDL, for use during upgrades or just a
general lockdown, then remove it again when complete.
* Perform auditing of DDL statements (which requires more info than
simply the text submitted)

If there are questions about wider functionality, or risks with that,
then reducing the functionality is OK, for me. The last thing we need
is a security hole introduced by this, or weird behaviour from people
interfering with things. (If you want that, write an executor plugin
and cook your own spaghetti).

We can do this by declaring clearly that there are limits on what can
be achieved with event triggers, perhaps even testing to check bad
things haven't been allowed to occur. I don't see that as translating
into massive change in the patch, just focusing on the main cases,
documenting that and perhaps introducing some restrictions on wider
usage. We will likely slowly expand the functionality of event
triggers over time, so keeping things general still works as a long
range goal.

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

On 2013-01-18 09:58:53 -0500, Robert Haas wrote:
> On Fri, Jan 18, 2013 at 9:07 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > I don't have a problem reusing the object access infrastructure at all. I just
> > don't think its providing even remotely enough. You have (co-)written that
> > stuff, so you probably know more than I do, but could you explain to me how it
> > could be reused to replicate a CREATE TABLE?
> >
> > Problems I see:
> > - afaics for CREATE TABLE the only hook is in ATExecAddColumn
>
> No, there's one also in heap_create_with_catalog. Took me a minute to
> find it, as it does not use InvokeObjectAccessHook. The idea is that
> OAT_POST_CREATE fires once per object creation, regardless of the
> object type - table, column, whatever.

Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
comment explaining why we InvokeObjectAccessHook isn't used just for
enhanced grepability.

> > - No way to regenerate the table definition for execution on the remote system
> > without creating libpqdump.
>
> IMHO, that is one of the really ugly problems that we haven't come up
> with a good solution for yet. If you want to replicate DDL, you have
> basically three choices:
>
> 1. copy over the statement text that was used on the origin server and
> hope none of the corner cases bite you
> 2. come up with some way of reconstituting a DDL statement based on
> (a) the parse tree or (b) what the server actually decided to do
> 3. reconstitute the state of the object from the catalogs after the
> command has run
>
> (2a) differs from (2b) for things like CREATE INDEX, where the index
> name might be left for the server to determine, but when replicating
> you'd like to get the same name out. (3) is workable for CREATE but
> not ALTER or DROP.
>
> The basic problem here is that (1) and (3) are not very
> reliable/complete and (2) is a lot of work and introduces a huge code
> maintenance burden.

I agree with that analysis.

> But it's unfair to pin that on the object-access
> hook mechanism

I agree. I don't think anybody has pinned it on it though.

> - any reverse-parsing or catalog-deconstruction
> solution for DDL is going to have that problem. The decision we have
> to make as a community is whether we're prepared to support and
> maintain that code for the indefinite future. Although I think it's
> easy to say "yes, because DDL replication would be really cool" - and
> I sure agree with that - I think it needs to be thought through a bit
> more deeply than that.
>
> I have been involved in PostgreSQL development for about 4.5 years
> now. This is less time than many people here, but it's still long
> enough to say a whole lot of people ask for some variant of this idea,
> and yet I have yet to see anybody produce a complete, working version
> of this functionality and maintain it outside of the PostgreSQL tree
> for one release cycle (did I miss something?).

I don't really think thats a fair argument.

For one, I didn't really ask for a libpgdump - I said that I don't see
any way to generate re-executable SQL without it if we don't get the
parse-tree but only the oid of the created object. Not to speak of the
complexities of supporting ALTER that way (which is, as you note,
basically not the way to do this).

Also, even though I don't think its the right way *for this*, I think
pg_dump and pgadmin pretty much prove that it's possible? The latter
even is out-of-core and has existed for multiple years.

Its also not really fair to compare out-of-tree effort of maintaining
such a library to in-core support. pg_dump *needs* to be maintained, so
the additional maintenance overhead once the initial development is done
shouldn't really be higher than now. Lower if anything, due to getting
rid of a good bit of ugly code ;). There's no such effect out of core.

If youre also considering parsetree->SQL transformation under the
libpgdump umbrella its even less fair. The backend already has a *lot*
of infrastructure to regenerate sql from trees, even if its mostly
centered arround around DQL. A noticeable amount of that code is
unavailable to extensions (i.e. many are static funcs).
Imo its completely unreasonable to expose that code to the outside and
expect it to have a stable interface. We *will* need to rewrite parts of
that regularly.
For those reasons I think the only reasonable way to create textual DDL
is in the backend trying to allow outside code to do that will impose
far greater limitations.

Whats the way you suggest to go on about this?

> There's a totally legitimate debate to be had here, but my feeling
> about what you're calling libpqdump is:

> - It's completely separate from event triggers.
> - It's completely separate from object access hooks.

Well, it is one of the major use-cases (or even *the* major use case)
for event triggers that I heard of. So it seems a valid point in a
dicussion on how to design the whole mechanism, doesn't it?

Some version of the event trigger patch contained partial support for
regenerating the DDL so it seems like a justified point there. Your
complained that suggestions about reusing object access hooks were
ignored, so mentioning that they currently don't provide *enough* (they
*do* provide a part, but it doesn't seem to be the most critical one)
also seems justifyable.

> [ BTW: Sorry our IM session got cut off. It started erroring out
> every time I messaged you. But no problem, anyway. ]

NP, its good to keep the technical stuff here anyway... Stupid
technology. In which business are we in again?

Greetings,

Andres Freund

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

On Fri, Jan 18, 2013 at 10:47 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> No, there's one also in heap_create_with_catalog. Took me a minute to
>> find it, as it does not use InvokeObjectAccessHook. The idea is that
>> OAT_POST_CREATE fires once per object creation, regardless of the
>> object type - table, column, whatever.
>
> Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
> comment explaining why we InvokeObjectAccessHook isn't used just for
> enhanced grepability.

I cannot parse that sentence, but I agree that the ungreppability of
it is suboptimal. I resorted to git log -SInvokeObjectAccessHook -p
to find it.

>> I have been involved in PostgreSQL development for about 4.5 years
>> now. This is less time than many people here, but it's still long
>> enough to say a whole lot of people ask for some variant of this idea,
>> and yet I have yet to see anybody produce a complete, working version
>> of this functionality and maintain it outside of the PostgreSQL tree
>> for one release cycle (did I miss something?).
>
> I don't really think thats a fair argument.
>
> For one, I didn't really ask for a libpgdump - I said that I don't see
> any way to generate re-executable SQL without it if we don't get the
> parse-tree but only the oid of the created object. Not to speak of the
> complexities of supporting ALTER that way (which is, as you note,
> basically not the way to do this).

Oh, OK. I see. Well, I've got no problem making the parse tree
available via InvokeObjectAccessHook. Isn't that just a matter of
adding a new hook type and a new call site?

> Also, even though I don't think its the right way *for this*, I think
> pg_dump and pgadmin pretty much prove that it's possible? The latter
> even is out-of-core and has existed for multiple years.

Fair point.

> Its also not really fair to compare out-of-tree effort of maintaining
> such a library to in-core support. pg_dump *needs* to be maintained, so
> the additional maintenance overhead once the initial development is done
> shouldn't really be higher than now. Lower if anything, due to getting
> rid of a good bit of ugly code ;). There's no such effect out of core.

This I don't follow. Nothing we might add to core to reverse-parse
either the catalogs or the parse tree is going to make pg_dump go
away.

> If youre also considering parsetree->SQL transformation under the
> libpgdump umbrella its even less fair. The backend already has a *lot*
> of infrastructure to regenerate sql from trees, even if its mostly
> centered arround around DQL. A noticeable amount of that code is
> unavailable to extensions (i.e. many are static funcs).
> Imo its completely unreasonable to expose that code to the outside and
> expect it to have a stable interface. We *will* need to rewrite parts of
> that regularly.
> For those reasons I think the only reasonable way to create textual DDL
> is in the backend trying to allow outside code to do that will impose
> far greater limitations.

I'm having trouble following this. Can you restate? I wasn't sure
what you meant by libpqdump. I assumed you were speaking of a
parsetree->DDL or catalog->DDL facility.

> Well, it is one of the major use-cases (or even *the* major use case)
> for event triggers that I heard of. So it seems a valid point in a
> dicussion on how to design the whole mechanism, doesn't it?

Yes, but is a separate problem from how we give control to the event
trigger (or object access hook).

> Some version of the event trigger patch contained partial support for
> regenerating the DDL so it seems like a justified point there. Your
> complained that suggestions about reusing object access hooks were
> ignored, so mentioning that they currently don't provide *enough* (they
> *do* provide a part, but it doesn't seem to be the most critical one)
> also seems justifyable.

Sure, but we could if we wanted decide to commit the partial support
for regenerating the DDL and tell people to use it via object access
hooks. Of course, that would thin out even further the number of
people who would actually be using that code, which I fear will
already be too small to achieve bug-free operation in a reasonable
time. If we add some hooks now and someone maintains the DDL
reverse-parsing code as an out-of-core replication solution for a few
releases, and that doesn't turn out to be hideous, I'd be a lot more
sanguine about incorporating it at that point. I basically don't
think that there's any way we're going to commit something along those
lines now and not have it turn out to be a far bigger maintenance
headache than anyone wants. What that tends to end up meaning in
practice is that Tom gets suckered into fixing it because nobody else
can take the time, and that's neither fair nor good for the project.

> NP, its good to keep the technical stuff here anyway... Stupid
> technology. In which business are we in again?

I don't know, maybe we can figure it out based on the objects in our
immediate vicinity. I see twelve cans of paint, most of them opened,
and something that looks sort of like a badly-made shed. Does that
help at all? :-)

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

On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
> On Fri, Jan 18, 2013 at 10:47 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> No, there's one also in heap_create_with_catalog. Took me a minute to
> >> find it, as it does not use InvokeObjectAccessHook. The idea is that
> >> OAT_POST_CREATE fires once per object creation, regardless of the
> >> object type - table, column, whatever.
> >
> > Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
> > comment explaining why we InvokeObjectAccessHook isn't used just for
> > enhanced grepability.
>
> I cannot parse that sentence, but I agree that the ungreppability of
> it is suboptimal. I resorted to git log -SInvokeObjectAccessHook -p
> to find it.

I was thinking of adding a comment like
/* We don't use InvokeObjectAccessHook here because we need to ... */
not because the comment in itself is important but because it allows to
grep ;)

> >> I have been involved in PostgreSQL development for about 4.5 years
> >> now. This is less time than many people here, but it's still long
> >> enough to say a whole lot of people ask for some variant of this idea,
> >> and yet I have yet to see anybody produce a complete, working version
> >> of this functionality and maintain it outside of the PostgreSQL tree
> >> for one release cycle (did I miss something?).
> >
> > I don't really think thats a fair argument.
> >
> > For one, I didn't really ask for a libpgdump - I said that I don't see
> > any way to generate re-executable SQL without it if we don't get the
> > parse-tree but only the oid of the created object. Not to speak of the
> > complexities of supporting ALTER that way (which is, as you note,
> > basically not the way to do this).
>
> Oh, OK. I see. Well, I've got no problem making the parse tree
> available via InvokeObjectAccessHook. Isn't that just a matter of
> adding a new hook type and a new call site?

Sure. Its probably not easy to choose the correct callsites though, they
need to be somewhat different for create & alter in comparison to drop.

create & alter
* after the object is created/altered
drop:
* after the object is locked, but *before* its dropped

As far as I understood thats basically the behind the ddl_trace event
trigger mentioned earlier.

> > Also, even though I don't think its the right way *for this*, I think
> > pg_dump and pgadmin pretty much prove that it's possible? The latter
> > even is out-of-core and has existed for multiple years.
>
> Fair point.

Imo its already a good justification for a libpgdump, not that I am
volunteering, but ...

> > Its also not really fair to compare out-of-tree effort of maintaining
> > such a library to in-core support. pg_dump *needs* to be maintained, so
> > the additional maintenance overhead once the initial development is done
> > shouldn't really be higher than now. Lower if anything, due to getting
> > rid of a good bit of ugly code ;). There's no such effect out of core.
>
> This I don't follow. Nothing we might add to core to reverse-parse
> either the catalogs or the parse tree is going to make pg_dump go
> away.

I was still talking about the hypothetical libpgdump we don't really
need for this ;)

> > If youre also considering parsetree->SQL transformation under the
> > libpgdump umbrella its even less fair. The backend already has a *lot*
> > of infrastructure to regenerate sql from trees, even if its mostly
> > centered arround around DQL. A noticeable amount of that code is
> > unavailable to extensions (i.e. many are static funcs).
> > Imo its completely unreasonable to expose that code to the outside and
> > expect it to have a stable interface. We *will* need to rewrite parts of
> > that regularly.
> > For those reasons I think the only reasonable way to create textual DDL
> > is in the backend trying to allow outside code to do that will impose
> > far greater limitations.
>
> I'm having trouble following this. Can you restate? I wasn't sure
> what you meant by libpqdump. I assumed you were speaking of a
> parsetree->DDL or catalog->DDL facility.

Yea, that wasn't really clear, sorry for that.

What I was trying to say that I think doing parstree->text conversion
out of tree is noticeably more complex and essentially places a higher
maintenance burden on the project because
1) the core already has a lot of infrastructure for such conversions
2) any external project would either need to copy that infrastructure or
make a large number of functions externally visible
3) any refactoring in that area would now break external tools even if
it would be trivial to adjust potential in-core support for such a
conversion

> > Some version of the event trigger patch contained partial support for
> > regenerating the DDL so it seems like a justified point there. Your
> > complained that suggestions about reusing object access hooks were
> > ignored, so mentioning that they currently don't provide *enough* (they
> > *do* provide a part, but it doesn't seem to be the most critical one)
> > also seems justifyable.
>
> Sure, but we could if we wanted decide to commit the partial support
> for regenerating the DDL and tell people to use it via object access
> hooks.

Yes, thats a possibility.

> Of course, that would thin out even further the number of
> people who would actually be using that code, which I fear will
> already be too small to achieve bug-free operation in a reasonable
> time.

Well, replication solutions seem to be the main potential consumer of
such a capability and while the C level stuff wouldn't be used by many,
hopefully the stuff built on top will.

> If we add some hooks now and someone maintains the DDL
> reverse-parsing code as an out-of-core replication solution for a few
> releases, and that doesn't turn out to be hideous, I'd be a lot more
> sanguine about incorporating it at that point.

As I said above, I think due to the available infrastructure doing this
out of tree is *far* harder and I fear that a good part of that code
would turn out not to be applicable for inclusion into core.

> I basically don't
> think that there's any way we're going to commit something along those
> lines now and not have it turn out to be a far bigger maintenance
> headache than anyone wants. What that tends to end up meaning in
> practice is that Tom gets suckered into fixing it because nobody else
> can take the time, and that's neither fair nor good for the project.

Now as is in "at this point of the cycle" or now as in "without a
several year old out-of-core project"?

> > NP, its good to keep the technical stuff here anyway... Stupid
> > technology. In which business are we in again?
>
> I don't know, maybe we can figure it out based on the objects in our
> immediate vicinity. I see twelve cans of paint, most of them opened,
> and something that looks sort of like a badly-made shed. Does that
> help at all? :-)

By the smell and looks from where I am sitting I seem to be a barrista
making rather deliciously smelling coffee and my boss seems to suck at
decorating ;). Oh, and I seem to have a terrible taste in music.

Regards,

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
>> I'm having trouble following this. Can you restate? I wasn't sure
>> what you meant by libpqdump. I assumed you were speaking of a
>> parsetree->DDL or catalog->DDL facility.

> Yea, that wasn't really clear, sorry for that.

> What I was trying to say that I think doing parstree->text conversion
> out of tree is noticeably more complex and essentially places a higher
> maintenance burden on the project because

Ah. That's not pg_dump's job though, so you're choosing a bad name for
it. What I think you are really talking about is extending the
deparsing logic in ruleutils.c to cover utility statements. Which is
not a bad plan in principle; we've just never needed it before. It
would be quite a lot of new code though.

An issue that would have to be thought about is that there are assorted
scenarios where we generate "parse trees" that contain options that
cannot be generated from SQL, so there's no way to reverse-list them
into working SQL. But often those hidden options are essential to know
what the statement will really do, so I'm not sure ignoring them will be
a workable answer for replication purposes.

regards, tom lane

On 2013-01-18 12:44:13 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
> >> I'm having trouble following this. Can you restate? I wasn't sure
> >> what you meant by libpqdump. I assumed you were speaking of a
> >> parsetree->DDL or catalog->DDL facility.
>
> > Yea, that wasn't really clear, sorry for that.
>
> > What I was trying to say that I think doing parstree->text conversion
> > out of tree is noticeably more complex and essentially places a higher
> > maintenance burden on the project because
>
> Ah. That's not pg_dump's job though, so you're choosing a bad name for
> it.

I really only mentioned libpgdump because the object access hooks at the
moment only get passed the object oid and because Robert doubted the
need for deparsing the parsetrees in the past. Without the parsetree you
obviousl cannot deparse the parsetree ;)

I do *not* think that using something that reassembles the statement
solely based on the catalog context is a good idea.

> What I think you are really talking about is extending the
> deparsing logic in ruleutils.c to cover utility statements. Which is
> not a bad plan in principle; we've just never needed it before. It
> would be quite a lot of new code though.

Yes, I think that is the only realistic approach at providing somewhat
unambigious SQL to replicate DDL.

> An issue that would have to be thought about is that there are assorted
> scenarios where we generate "parse trees" that contain options that
> cannot be generated from SQL, so there's no way to reverse-list them
> into working SQL. But often those hidden options are essential to know
> what the statement will really do, so I'm not sure ignoring them will be
> a workable answer for replication purposes.

Dimitri's earlier patches had support for quite some commands via
deparsing and while its a noticeable amount of code it seemed to work
ok.
The last revision I could dig out is
https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c

I think some things in there would have to change (e.g. I doubt that its
good that EventTriggerData is involved at that level) but I think it
shows very roughly how it could look.

Greetings,

Andres

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

Andres Freund escribió:
> On 2013-01-18 12:44:13 -0500, Tom Lane wrote:

> > An issue that would have to be thought about is that there are assorted
> > scenarios where we generate "parse trees" that contain options that
> > cannot be generated from SQL, so there's no way to reverse-list them
> > into working SQL. But often those hidden options are essential to know
> > what the statement will really do, so I'm not sure ignoring them will be
> > a workable answer for replication purposes.
>
> Dimitri's earlier patches had support for quite some commands via
> deparsing and while its a noticeable amount of code it seemed to work
> ok.
> The last revision I could dig out is
> https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c
>
> I think some things in there would have to change (e.g. I doubt that its
> good that EventTriggerData is involved at that level) but I think it
> shows very roughly how it could look.

I looked at that code back then and didn't like it very much. The
problem I see (as Robert does, I think) is that it raises the burden
when you change the grammar -- you now need to edit not only gram.y, but
the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.
One idea I had was to add annotations on gram.y so that an external
(Perl?) program could generate the ddl_rewrite.c equivalent, without
forcing the developer to do it by hand.

Another problem is what Tom mentions: there are internal options that
cannot readily be turned back into SQL. We would have to expose these
at the SQL level somehow; but to me that looks painful because we would
be offering users the option to break their systems by calling commands
that do things that should only be done internally.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-01-18 15:22:55 -0300, Alvaro Herrera wrote:
> Andres Freund escribió:
> > On 2013-01-18 12:44:13 -0500, Tom Lane wrote:
>
> > > An issue that would have to be thought about is that there are assorted
> > > scenarios where we generate "parse trees" that contain options that
> > > cannot be generated from SQL, so there's no way to reverse-list them
> > > into working SQL. But often those hidden options are essential to know
> > > what the statement will really do, so I'm not sure ignoring them will be
> > > a workable answer for replication purposes.
> >
> > Dimitri's earlier patches had support for quite some commands via
> > deparsing and while its a noticeable amount of code it seemed to work
> > ok.
> > The last revision I could dig out is
> > https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c
> >
> > I think some things in there would have to change (e.g. I doubt that its
> > good that EventTriggerData is involved at that level) but I think it
> > shows very roughly how it could look.
>
> I looked at that code back then and didn't like it very much. The
> problem I see (as Robert does, I think) is that it raises the burden
> when you change the grammar -- you now need to edit not only gram.y, but
> the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.

Yea, but thats nothing new, you already need to do all that for normal
SQL. Changes that to the grammar that don't involve modifying ruleutils.c et
al. are mostly trivial enough that this doesn't really place that much of a
burden.

And I yet to hear any other proposal of how to do this?

> Another problem is what Tom mentions: there are internal options that
> cannot readily be turned back into SQL. We would have to expose these
> at the SQL level somehow; but to me that looks painful because we would
> be offering users the option to break their systems by calling commands
> that do things that should only be done internally.

Hm. Which options are you two thinking of right now?

Greetings,

Andres Freund

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Andres Freund escribi:
>> Dimitri's earlier patches had support for quite some commands via
>> deparsing and while its a noticeable amount of code it seemed to work
>> ok.
>> The last revision I could dig out is
>> https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c

> I looked at that code back then and didn't like it very much. The
> problem I see (as Robert does, I think) is that it raises the burden
> when you change the grammar -- you now need to edit not only gram.y, but
> the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.

Well, that burden already exists for non-utility statements --- why
should utility statements get a pass? Other than that we tend to invent
new utility syntax freely, which might be a good thing to discourage
anyhow.

regards, tom lane

On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> Andres Freund escribió:
>>> Dimitri's earlier patches had support for quite some commands via
>>> deparsing and while its a noticeable amount of code it seemed to work
>>> ok.
>>> The last revision I could dig out is
>>> https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c
>
>> I looked at that code back then and didn't like it very much. The
>> problem I see (as Robert does, I think) is that it raises the burden
>> when you change the grammar -- you now need to edit not only gram.y, but
>> the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.
>
> Well, that burden already exists for non-utility statements --- why
> should utility statements get a pass? Other than that we tend to invent
> new utility syntax freely, which might be a good thing to discourage
> anyhow.

My concerns are that (1) it will slow down the addition of new
features to PostgreSQL by adding yet another barrier to commit and (2)
it won't be get enough use or regression test coverage to be, or
remain, bug-free.

There may be other concerns with respect to the patch-as-proposed, but
those are my high-level concerns.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Well, that burden already exists for non-utility statements --- why
>> should utility statements get a pass? Other than that we tend to invent
>> new utility syntax freely, which might be a good thing to discourage
>> anyhow.

> My concerns are that (1) it will slow down the addition of new
> features to PostgreSQL by adding yet another barrier to commit and (2)
> it won't be get enough use or regression test coverage to be, or
> remain, bug-free.

Meh. The barriers to inventing new statements are already mighty tall.
As for (2), I agree there's risk of bugs, but what alternative have you
got that is likely to be less bug-prone? At least a reverse-list
capability could be tested standalone without having to set up a logical
replication configuration.

This should not be interpreted as saying I'm gung-ho to do this, mind
you. I'm just saying that if our intention is to support logical
replication of all DDL operations, none of the alternatives look cheap.

regards, tom lane

On Fri, Jan 18, 2013 at 5:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Well, that burden already exists for non-utility statements --- why
>>> should utility statements get a pass? Other than that we tend to invent
>>> new utility syntax freely, which might be a good thing to discourage
>>> anyhow.
>
>> My concerns are that (1) it will slow down the addition of new
>> features to PostgreSQL by adding yet another barrier to commit and (2)
>> it won't be get enough use or regression test coverage to be, or
>> remain, bug-free.
>
> Meh. The barriers to inventing new statements are already mighty tall.
> As for (2), I agree there's risk of bugs, but what alternative have you
> got that is likely to be less bug-prone? At least a reverse-list
> capability could be tested standalone without having to set up a logical
> replication configuration.
>
> This should not be interpreted as saying I'm gung-ho to do this, mind
> you. I'm just saying that if our intention is to support logical
> replication of all DDL operations, none of the alternatives look cheap.

Well, we agree on that, anyway. :-)

I honestly don't know what to do about this. I think you, Alvaro, and
I all have roughly the same opinion of this, which is that it doesn't
sound fun, but there's probably nothing better. So, what do we do
when a really cool potential feature (logical replication of DDL)
butts up against an expensive future maintenance requirement? I'm not
even sure what the criteria should be for making a decision on whether
it's "worth it".

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Ok. Will prepare a non controversial patch for ddl_command_end.
>
> Thanks. I will make a forceful effort to review that in a timely
> fashion when it's posted.

Please find it attached to this email.

COLUMNS=72 git diff --stat
doc/src/sgml/event-trigger.sgml | 93 ++++++-
src/backend/commands/event_trigger.c | 122 +++++++--
src/backend/tcop/utility.c | 273 ++++++++++---------
src/backend/utils/cache/evtcache.c | 2 +
src/include/commands/event_trigger.h | 1 +
src/include/utils/evtcache.h | 3 +-
src/test/regress/expected/event_trigger.out | 6 +-
src/test/regress/sql/event_trigger.sql | 4 +
8 files changed, 359 insertions(+), 145 deletions(-)

What is the next part you want to see separated away?

We have a choice of:

- Context exposing and filtering

My preference for that point is to include it and limit it to event
triggers written in C, leaving the door open to add new events in
the future, and allowing existing consumers of that framework see
through implementation details should they need it.

It's better than just a ProcessUtility_hook because you can see it
in \dy, decide to disable it with ALTER EVENT TRIGGER (e.g. in a
transcation to avoid recursive calling into itself), and it won't
run in single user mode even when in shared_preload_libraries.

Another idea is to protect it with a GUC.

Yet another idea is to add a new property in the CREATE EVENT
TRIGGER command so that user can request explicitely to have that
and know the feature will break at next release and all the ones
after that:

CREATE EVENT TRIGGER foo ON ddl_command_start
WHEN show_internals in ('Yes I know what I''m doing',
'Thanks')
EXECUTE PROCEDURE …();

That particular phrasing and option choice might be revisited before
commit, though, I'm not wedded to it.

- Additional Information to PL/pgSQL

We're talking about :

- operation (CREATE, ALTER, DROP)
- object type (TABLE, FUNCTION, TEXT SEARCH DICTIONARY, …)
- object OID (when there's a single object target)
- object shema name (when there's a single object target)
- object name (when there's a single object target)

- Additional Information to PL/*

It's separated away because you did opt-out from reviewing that
part, so I guess we will continue to make that a separate patch, and
it will still need to happen. I've been said that I shouldn't be on
the hook to provide for that, and I wonder, but anyway I did already
implement it in a previous version so I can do it again as soon as
we know what we expose.

- Command String Normalisation

I removed it already from the main patch because we needed to talk
about it more, and it's kind of a big part in itself. Meanwhile it
seems to me that an agreement has been reached and that I will be
able to follow the consensus and resume working on that part.

- ddl_event_trace

Now is the time to kill it (for 9.3) or allow it in.

It means another cache lookup at DDL Event time (so 2 cache lookups
per DDL in 9.3); and allow users who just want to have the
information not to have to care about when it is available.

- Non DDL related Events

Extra bonus items, a table_rewrite event. Unlikely for 9.3 at best,
unless someone else wants to have at it, maybe?

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

On Mon, Jan 21, 2013 at 12:27 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Please find it attached to this email.

Nice clean patch, thanks!

Committed, after tinkering with the CommandCounterIncrement() stuff a bit.

I will respond to the rest of your email later. Reading through this
patch left me with a slight concern regarding both ddl_command_start
and ddl_command_end: what happens if there's more than one event
trigger scheduled to fire, and one of them does something like drop
(with cascade) the function that a later one uses? Admittedly, that
seems like an unlikely case, but we probably want to check that
nothing too awful happens (e.g. crashing the server) and maybe add a
regression test to cover this scenario.

Another thing is that we might want to document that if a command
errors out, ddl_command_end will never be reached; and perhaps also
that if ddl_command_start errors out, the command itself will never be
reached. Perhaps this is so obvious as to not bear mentioning, I
don't know, but the thought crossed my mind that someone might fail to
realize it.

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

2013/1/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
> Another thing is that we might want to document that if a command
> errors out, ddl_command_end will never be reached; and perhaps also
> that if ddl_command_start errors out, the command itself will never be
> reached. Perhaps this is so obvious as to not bear mentioning, I
> don't know, but the thought crossed my mind that someone might fail to
> realize it.

I think that should be a mention about that in docs, someone could
expect that ddl_command_end be reached even when
ddl_command_start erros out, and try to use it in some way.

Regards,
--
Dickson S. Guedes
mail/xmpp: guedes(at)guedesoft(dot)net - skype: guediz
http://github.com/guedes - http://guedesoft.net
http://www.postgresql.org.br

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Nice clean patch, thanks!
>
> Committed, after tinkering with the CommandCounterIncrement() stuff a bit.

Cool. thanks!

> I will respond to the rest of your email later. Reading through this
> patch left me with a slight concern regarding both ddl_command_start
> and ddl_command_end: what happens if there's more than one event
> trigger scheduled to fire, and one of them does something like drop
> (with cascade) the function that a later one uses? Admittedly, that
> seems like an unlikely case, but we probably want to check that
> nothing too awful happens (e.g. crashing the server) and maybe add a
> regression test to cover this scenario.

I added some in the attached patch.

doc/src/sgml/event-trigger.sgml | 10 ++
src/backend/commands/event_trigger.c | 6 +-
src/test/regress/expected/event_trigger.out | 106 +++++++++++++++++++
src/test/regress/sql/event_trigger.sql | 47 ++++++++
4 files changed, 167 insertions(+), 2 deletions(-)

I had to make quiet a part of the regression tests because of the ERROR
message containing an OID, where I think it's the right error message.

I don't think we want to add complexity to the code to be able to report
the procedure name that we depend on in the event cache and that the
user just deleted while in the middle of actually running the event
trigger list we already got from the cache, right?

"Dickson S. Guedes" <listas(at)guedesoft(dot)net> writes:
> 2013/1/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> Another thing is that we might want to document that if a command
>> errors out, ddl_command_end will never be reached; and perhaps also
>> that if ddl_command_start errors out, the command itself will never be
>> reached. Perhaps this is so obvious as to not bear mentioning, I
>> don't know, but the thought crossed my mind that someone might fail to
>> realize it.
>
> I think that should be a mention about that in docs, someone could
> expect that ddl_command_end be reached even when
> ddl_command_start erros out, and try to use it in some way.

Yeah, I share both your views. I've been playing with event triggers for
too long to think about such documentation it seems, thanks for
reminding me about it.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> I added some in the attached patch.
>
> doc/src/sgml/event-trigger.sgml | 10 ++
> src/backend/commands/event_trigger.c | 6 +-
> src/test/regress/expected/event_trigger.out | 106 +++++++++++++++++++
> src/test/regress/sql/event_trigger.sql | 47 ++++++++
> 4 files changed, 167 insertions(+), 2 deletions(-)

And I did drop a comment line I didn't mean to when trying things out,
so here's the update copy. There's a bug fix in there too, in both the
versions of the patch, that the new regression tests exercise.

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

On 22 January 2013 12:02, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>
> "Dickson S. Guedes" <listas(at)guedesoft(dot)net> writes:
> > 2013/1/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
> >> Another thing is that we might want to document that if a command
> >> errors out, ddl_command_end will never be reached; and perhaps also
> >> that if ddl_command_start errors out, the command itself will never be
> >> reached. Perhaps this is so obvious as to not bear mentioning, I
> >> don't know, but the thought crossed my mind that someone might fail to
> >> realize it.
> >
> > I think that should be a mention about that in docs, someone could
> > expect that ddl_command_end be reached even when
> > ddl_command_start erros out, and try to use it in some way.
>
> Yeah, I share both your views. I've been playing with event triggers for
> too long to think about such documentation it seems, thanks for
> reminding me about it.

I haven't followed the long discussions this patch has undergone, so
forgive me if what I'm about to ask has already been discussed and
discarded.

Would it be desirable to have ddl_command_success and
ddl_command_failed events. These would effectively be subsets to
ddl_command_end. Or should this be something handled by the event
trigger function? This would mean that you could have a command end
trigger that wouldn't necessarily fire, depending on the outcome of
the command.

Alternatively, and more elegantly, there could be a filter_variable of
(...thinks...) ERROR (?) which can have a value of true or false.
(previous paragraph seems like an awful idea now)

CREATE EVENT TRIGGER log_bad_event ON ddl_command_end WHEN ERROR IN (true)
EXECUTE PROCEDURE abort_any_command();

This, unfortunately, introducing awkwardness with the WHEN clause
restriction which doesn't accommodate simple equality. And looking at
the IN part of the syntax, it looks awful: WHEN TAG IN ('DROP
SEQUENCE' AND 'CREATE TABLE').

This looks like a confusing hybrid of a standard WHERE clause and an
IN list, but is neither. You'd expect an IN to evaluate a list of
expressions, but instead it's saying what TAGs are allowed. And this
is where it starts to read like a WHERE clause, except such logic in a
WHERE clause wouldn't make any sense as AND might suggest it must
equal both. Please, please let it not stay like this. :)

So note that I'm coming into this syntax kinda fresh, so just giving
first impressions of the current implementation.

--
Thom

Thom Brown <thom(at)linux(dot)com> writes:
> Would it be desirable to have ddl_command_success and
> ddl_command_failed events. These would effectively be subsets to

No, because you can't run any SQL in a failed transaction.

> This, unfortunately, introducing awkwardness with the WHEN clause
> restriction which doesn't accommodate simple equality. And looking at
> the IN part of the syntax, it looks awful: WHEN TAG IN ('DROP
> SEQUENCE' AND 'CREATE TABLE').

The syntax is using a comma, not an "AND", as seen in the tests:

create event trigger regress_event_trigger2 on ddl_command_start
when tag in ('create table', 'CREATE FUNCTION')
execute procedure test_event_trigger();

> So note that I'm coming into this syntax kinda fresh, so just giving
> first impressions of the current implementation.

Thanks for that, I'm all for getting something better at the end!

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

On 22 January 2013 13:28, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
>> Would it be desirable to have ddl_command_success and
>> ddl_command_failed events. These would effectively be subsets to
>
> No, because you can't run any SQL in a failed transaction.

Okay, I had misunderstood something someone wrote previously, and that
makes sense now.

>> This, unfortunately, introducing awkwardness with the WHEN clause
>> restriction which doesn't accommodate simple equality. And looking at
>> the IN part of the syntax, it looks awful: WHEN TAG IN ('DROP
>> SEQUENCE' AND 'CREATE TABLE').
>
> The syntax is using a comma, not an "AND", as seen in the tests:
>
> create event trigger regress_event_trigger2 on ddl_command_start
> when tag in ('create table', 'CREATE FUNCTION')
> execute procedure test_event_trigger();

Ah, in that case, the docs are wrong:
http://www.postgresql.org/docs/devel/static/sql-createeventtrigger.html

--
Thom

Thom Brown <thom(at)linux(dot)com> writes:
> Ah, in that case, the docs are wrong:
> http://www.postgresql.org/docs/devel/static/sql-createeventtrigger.html

Oh. It's missing the comma and applying the AND at the wrong level,
here's a fix:

diff --git a/doc/src/sgml/ref/create_event_trigger.sgml b/doc/src/sgml/ref/create_event_trigger.sgml
index 040df11..3088ffa 100644
--- a/doc/src/sgml/ref/create_event_trigger.sgml
+++ b/doc/src/sgml/ref/create_event_trigger.sgml
@@ -23,7 +23,7 @@ PostgreSQL documentation
<synopsis>
CREATE EVENT TRIGGER <replaceable class="PARAMETER">name</replaceable>
ON <replaceable class="PARAMETER">event</replaceable>
- [ WHEN <replaceable class="PARAMETER">filter_variable</replaceable> IN (filter_value [ AND ... ] ) ]
+ [ WHEN <replaceable class="PARAMETER">filter_variable</replaceable> IN (filter_value [, ... ]) [ AND ... ] ]
EXECUTE PROCEDURE <replaceable class="PARAMETER">function_name</replaceable>()
</synopsis>
</refsynopsisdiv>

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

On 22 January 2013 14:45, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
>> Ah, in that case, the docs are wrong:
>> http://www.postgresql.org/docs/devel/static/sql-createeventtrigger.html
>
> Oh. It's missing the comma and applying the AND at the wrong level,
> here's a fix:
>
> diff --git a/doc/src/sgml/ref/create_event_trigger.sgml b/doc/src/sgml/ref/create_event_trigger.sgml
> index 040df11..3088ffa 100644
> --- a/doc/src/sgml/ref/create_event_trigger.sgml
> +++ b/doc/src/sgml/ref/create_event_trigger.sgml
> @@ -23,7 +23,7 @@ PostgreSQL documentation
> <synopsis>
> CREATE EVENT TRIGGER <replaceable class="PARAMETER">name</replaceable>
> ON <replaceable class="PARAMETER">event</replaceable>
> - [ WHEN <replaceable class="PARAMETER">filter_variable</replaceable> IN (filter_value [ AND ... ] ) ]
> + [ WHEN <replaceable class="PARAMETER">filter_variable</replaceable> IN (filter_value [, ... ]) [ AND ... ] ]
> EXECUTE PROCEDURE <replaceable class="PARAMETER">function_name</replaceable>()
> </synopsis>
> </refsynopsisdiv>

Okay, that makes sense now. :)

--
Thom

On 22 January 2013 14:47, Thom Brown <thom(at)linux(dot)com> wrote:
> On 22 January 2013 14:45, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Thom Brown <thom(at)linux(dot)com> writes:
>>> Ah, in that case, the docs are wrong:
>>> http://www.postgresql.org/docs/devel/static/sql-createeventtrigger.html
>>
>> Oh. It's missing the comma and applying the AND at the wrong level,
>> here's a fix:
>>
>> diff --git a/doc/src/sgml/ref/create_event_trigger.sgml b/doc/src/sgml/ref/create_event_trigger.sgml
>> index 040df11..3088ffa 100644
>> --- a/doc/src/sgml/ref/create_event_trigger.sgml
>> +++ b/doc/src/sgml/ref/create_event_trigger.sgml
>> @@ -23,7 +23,7 @@ PostgreSQL documentation
>> <synopsis>
>> CREATE EVENT TRIGGER <replaceable class="PARAMETER">name</replaceable>
>> ON <replaceable class="PARAMETER">event</replaceable>
>> - [ WHEN <replaceable class="PARAMETER">filter_variable</replaceable> IN (filter_value [ AND ... ] ) ]
>> + [ WHEN <replaceable class="PARAMETER">filter_variable</replaceable> IN (filter_value [, ... ]) [ AND ... ] ]
>> EXECUTE PROCEDURE <replaceable class="PARAMETER">function_name</replaceable>()
>> </synopsis>
>> </refsynopsisdiv>
>
> Okay, that makes sense now. :)

ALTER INDEX is missing from the event trigger matrix.

When renaming a column on a foreign table, tg_tag reports 'ALTER
TABLE' instead of 'ALTER FOREIGN TABLE'. It doesn't do this for any
other ALTER FOREIGN TABLE operation, including altering, adding or
dropping a column.

DROP OWNED BY doesn't fire any type of event trigger. Is this right?

And also I can't get ddl_command_end to do anything:

test=# CREATE OR REPLACE FUNCTION public.cmd_trg_info()
test-# RETURNS event_trigger
test-# LANGUAGE plpgsql
test-# AS $function$
test$# BEGIN
test$# RAISE NOTICE E'Command trigger: tg_event=\'%\' tg_tag=\'%\'',
tg_event, tg_tag;
test$# END; $function$;
CREATE FUNCTION
test=# CREATE EVENT TRIGGER cmd_trg_before_create_table ON
ddl_command_end WHEN TAG IN ('CREATE TABLE') EXECUTE PROCEDURE
cmd_trg_info();
CREATE EVENT TRIGGER
test=# CREATE TABLE test4 AS SELECT 1::int id, ''::text test;
SELECT 1
test=#

Tried this with every possible command and the corresponding trigger
never fires. Was this actually tested or am I missing something? :S

Also, I'm assuming the 'ANY COMMAND' special tag is supposed to be removed?

postgres=# CREATE EVENT TRIGGER cmd_trg_before_any_command ON
ddl_command_start WHEN TAG IN ('ANY COMMAND') EXECUTE PROCEDURE
cmd_trg_info_any();
ERROR: filter value "ANY COMMAND" not recognized for filter variable "tag"

... but it lets me use it for ddl_command_end triggers:

postgres=# CREATE EVENT TRIGGER cmd_trg_after_any_command ON
ddl_command_end WHEN TAG IN ('ANY COMMAND') EXECUTE PROCEDURE
cmd_trg_info();
CREATE EVENT TRIGGER

This doesn't actually fire anyway due to the issue I mentioned above
where ddl_command_end doesn't appear to be functional.

However, I *can* get ddl_command_end to run on any command by omitting
the WHEN clause, but this is the only scenario where I can get a
trigger firing when using ddl_command_end:

postgres=# CREATE EVENT TRIGGER cmd_trg_after_any_command_all ON
ddl_command_end EXECUTE PROCEDURE cmd_trg_info();
CREATE EVENT TRIGGER
postgres=# CREATE TABLE moooo2(id serial);
NOTICE: Command trigger: tg_event='ddl_command_end' tg_tag='CREATE TABLE'
CREATE TABLE

--
Thom

On 22 January 2013 16:28, Thom Brown <thom(at)linux(dot)com> wrote:
> On 22 January 2013 14:47, Thom Brown <thom(at)linux(dot)com> wrote:
>
> postgres=# CREATE EVENT TRIGGER cmd_trg_after_any_command ON
> ddl_command_end WHEN TAG IN ('ANY COMMAND') EXECUTE PROCEDURE
> cmd_trg_info();
> CREATE EVENT TRIGGER

I can see why this works now. The ddl_command_end option stops the
TAG being evaluated:

test2=# CREATE EVENT TRIGGER cmd_trg_before_any_command ON
ddl_command_end WHEN TAG IN ('MEOW LIKE A CAT') EXECUTE PROCEDURE
cmd_trg_info_any();
CREATE EVENT TRIGGER

...unless I've coincidentally stumbled upon the new MEOW LIKE A CAT
command coming in 9.3. ;)

--
Thom

Thom Brown <thom(at)linux(dot)com> writes:
> ALTER INDEX is missing from the event trigger matrix.
>
> When renaming a column on a foreign table, tg_tag reports 'ALTER
> TABLE' instead of 'ALTER FOREIGN TABLE'. It doesn't do this for any
> other ALTER FOREIGN TABLE operation, including altering, adding or
> dropping a column.

Will see about that later, thanks for reporting.

> DROP OWNED BY doesn't fire any type of event trigger. Is this right?

It's currently being discussed.

> And also I can't get ddl_command_end to do anything:

Due to a bug in the form of a stray & in the patch that got commited,
and that I fixed this morning:

http://www.postgresql.org/message-id/m2vcapl7tl.fsf@2ndQuadrant.fr

> Also, I'm assuming the 'ANY COMMAND' special tag is supposed to be
> removed?

Yes, just omit the WHEN clause for the TAG.

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

Thom Brown <thom(at)linux(dot)com> writes:
> test2=# CREATE EVENT TRIGGER cmd_trg_before_any_command ON
> ddl_command_end WHEN TAG IN ('MEOW LIKE A CAT') EXECUTE PROCEDURE
> cmd_trg_info_any();
> CREATE EVENT TRIGGER
>
> ...unless I've coincidentally stumbled upon the new MEOW LIKE A CAT
> command coming in 9.3. ;)

Oe Noe, my carefully planted Easter Egg!

Seriously it's covered in the regression tests and shouldn't work, I'm
surprised here. Will see about that tomorrow with the other findings of
yours, thanks again.

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

On 2013-01-18 17:20:51 -0500, Robert Haas wrote:
> On Fri, Jan 18, 2013 at 5:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> Well, that burden already exists for non-utility statements --- why
> >>> should utility statements get a pass? Other than that we tend to invent
> >>> new utility syntax freely, which might be a good thing to discourage
> >>> anyhow.
> >
> >> My concerns are that (1) it will slow down the addition of new
> >> features to PostgreSQL by adding yet another barrier to commit and (2)
> >> it won't be get enough use or regression test coverage to be, or
> >> remain, bug-free.
> >
> > Meh. The barriers to inventing new statements are already mighty tall.
> > As for (2), I agree there's risk of bugs, but what alternative have you
> > got that is likely to be less bug-prone? At least a reverse-list
> > capability could be tested standalone without having to set up a logical
> > replication configuration.
> >
> > This should not be interpreted as saying I'm gung-ho to do this, mind
> > you. I'm just saying that if our intention is to support logical
> > replication of all DDL operations, none of the alternatives look cheap.
>
> Well, we agree on that, anyway. :-)
>
> I honestly don't know what to do about this. I think you, Alvaro, and
> I all have roughly the same opinion of this, which is that it doesn't
> sound fun, but there's probably nothing better.

Count me in.

> So, what do we do
> when a really cool potential feature (logical replication of DDL)
> butts up against an expensive future maintenance requirement?

I just tried to estimate the effort of that by looking at:
git log REL9_1_0..REL9_2_0 src/backend/parser/gram.y src/include/nodes/
which seems like it would catch most such patches.

First thing I have to say is there are more patches potentially
affecting such a potential feature than I previously had thought.
On the other hand, just about all of those patches are so big that the
amount of changes to the ruleutils.c equivalent for DDL look like a
minor pain in comparison to the rest.

The patches that seemed relevant during 9.1=>9.2 seem to be to be the
following when counting ones that directly would add new deparsing code:
* NOT VALID
* SECURITY LABEL
* per column fdw options
* range datatypes
* security barrier
* LEAKPROOF
* privileges on types
* ALTER FDW
* rename domain constraints

Then there were a bunch of patches adding new features that required
restructuring of existing code that would have affected possible
deparsing code:
* plancache restructuring
* parametrized indexscans
* DROP ... consolidation

In all of those but "ALTER FDW" and "rename domain constraints" possible
deparsing code looks like it would be an absolutely minor part of the
code. And in those its only a relevant part because they are really
small.

There were some bugfixes that would have required touching it as well,
but those seemed to be cases that are basically s/a/b/.

Not sure what the consequence of that is, but I wanted to share it
anyway after spending the time ;).

> I'm not
> even sure what the criteria should be for making a decision on whether
> it's "worth it".

In my experience it is the weakest point in the non-WAL based postgres
replication tools. So I personally think it is worth some overhead when
adding new features.

Greetings,

Andres Freund

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

On Tue, Jan 22, 2013 at 7:29 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
>> I added some in the attached patch.
>>
>> doc/src/sgml/event-trigger.sgml | 10 ++
>> src/backend/commands/event_trigger.c | 6 +-
>> src/test/regress/expected/event_trigger.out | 106 +++++++++++++++++++
>> src/test/regress/sql/event_trigger.sql | 47 ++++++++
>> 4 files changed, 167 insertions(+), 2 deletions(-)
>
> And I did drop a comment line I didn't mean to when trying things out,
> so here's the update copy. There's a bug fix in there too, in both the
> versions of the patch, that the new regression tests exercise.

I think these new regression tests are no good, because I doubt that
the number of recursive calls that can fit into any given amount of
stack space is guaranteed to be the same on all platforms. I have
committed the bug fixes themselves, however.

I wasn't entirely happy with your proposed documentation so I'm
attaching a counter-proposal. My specific complaints are (1) telling
people that event triggers are run in a savepoint seems a little too
abstract; I have attempted to make the consequences more concrete; (2)
RAISE EXCEPTION is PL/pgsql specific and not the only possible reason
for an error; I have attempted to be more generic; and (3) in the
process of fiddling with this, I noticed that the ddl_command_end
documentation can, I believe, be made both shorter and more clear by
turning it into a rider on the previous paragraph.

Comments?

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

On Tue, Jan 22, 2013 at 7:02 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> I had to make quiet a part of the regression tests because of the ERROR
> message containing an OID, where I think it's the right error message.

I have a feeling that's the sort of error message that we don't want
people to get. Can we arrange for something cleaner? Not sure
exactly what. Maybe something like this would do it:

ERROR: queued event trigger function not found

Or maybe we should just silently ignore failures to look up the event
trigger. That might be better, because the DBA could always do:

DROP FUNCTION myeventtrgfn() CASCADE;

...and it would be undesirable for other sessions to error out in that
case due to SnapshotNow effects.

> I don't think we want to add complexity to the code to be able to report
> the procedure name that we depend on in the event cache and that the
> user just deleted while in the middle of actually running the event
> trigger list we already got from the cache, right?

Yeah ... that seems like more code than the situation justifies.

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

On Tue, Jan 22, 2013 at 11:38 AM, Thom Brown <thom(at)linux(dot)com> wrote:
> ...unless I've coincidentally stumbled upon the new MEOW LIKE A CAT
> command coming in 9.3. ;)

+1 for adding that. I've been wanting it for years.

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