Re: [HACKERS] Command Triggers, patch v11
Tom Lane writes: > I've applied the CTAS patch after rather heavy editorialization. Don't > know what consequences that will have for Dimitri's patch. It allows my patch to add support for CREATE TABLE AS and SELECT INTO, I've been doing that and am on my way to sending a v18 now. The way you worked out the command tag is exactly what I needed, so thanks a lot for your work comitting this and paying attention :) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Tuesday, March 20, 2012 02:39:56 AM Tom Lane wrote: > I've applied the CTAS patch after rather heavy editorialization. Don't > know what consequences that will have for Dimitri's patch. Thanks for all the work you put into this! Looks cleaner now... Thanks, Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
I've applied the CTAS patch after rather heavy editorialization. Don't know what consequences that will have for Dimitri's patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
I wrote: > One thing I soon found is that it lacks support for EXPLAIN SELECT INTO. While I'm not particularly excited about fixing PREPARE ... SELECT INTO or CREATE RULE ... SELECT INTO, I've come to the conclusion that the EXPLAIN case is a must-fix. Because not only is EXPLAIN SELECT INTO broken, but so is EXPLAIN CREATE TABLE AS, and the latter functionality is actually documented. So presumably somebody went out of their way to make this work, at some point. Since I've got the table-creation code moved into intorel_startup, this doesn't look to be that painful, but it will require an API change for ExplainOnePlan, which is slightly annoying because that's probably in use by third-party plugins. We could either break them obviously (by adding an explicit parameter) or break them subtly (by adding an ExplainState field they might forget to initialize). The former seems preferable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Robert Haas writes: > On Mon, Mar 19, 2012 at 12:53 PM, Peter Eisentraut wrote: >> Doesn't seem worth it to me. At least, "SELECT " makes some sense: >> rows were selected. "CREATE TABLE " means what? tables >> were created? >> >> What might make sense is to delegate this additional information to >> separate fields in a future protocol revision. > I think that we would not have bothered to add the row count to the > command tag output for SELECT unless it were useful. It seems to be > *more* useful for CTAS than for SELECT; after all, SELECT also returns > the actual rows. I think we're all in agreement that we need to keep the rowcount functionality. What seems to me to be in some doubt is whether to continue to present the tag "SELECT " or to change it to something like "CREATE TABLE ". For the moment I've got the patch doing the former. It would not be terribly hard to change it, but I'm not going to break backward compatibility unless there's a pretty clear consensus to do so. BTW, I just came across another marginal-loss-of-functionality issue: in previous versions you could PREPARE a SELECT INTO, but now you get regression=# prepare foo as select * into bar from int8_tbl; ERROR: utility statements cannot be prepared Is anybody excited about that? If it is something we have to keep, it seems like pretty nearly a deal-breaker for this patch, because storing a CreateTableAsStmt containing an already-prepared plan would complicate matters unreasonably. You can still get approximately the same result with prepare foo as select * from int8_tbl; create table bar as execute foo; which if anything is more useful since you didn't nail down the target table name in the PREPARE, but nonetheless it's different. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Mon, Mar 19, 2012 at 12:53 PM, Peter Eisentraut wrote: > On sön, 2012-03-18 at 21:16 -0400, Tom Lane wrote: >> If we were going to change the output at all, I would vote for "CREATE >> TABLE " so as to preserve the rowcount functionality. Keep in >> mind though that this would force client-side changes, for instance in >> libpq's PQcmdTuples(). Fixing that one routine isn't so painful, but >> what of other client-side libraries, not to mention applications? > > Doesn't seem worth it to me. At least, "SELECT " makes some sense: > rows were selected. "CREATE TABLE " means what? tables > were created? > > What might make sense is to delegate this additional information to > separate fields in a future protocol revision. I think that we would not have bothered to add the row count to the command tag output for SELECT unless it were useful. It seems to be *more* useful for CTAS than for SELECT; after all, SELECT also returns the actual rows. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On sön, 2012-03-18 at 21:16 -0400, Tom Lane wrote: > If we were going to change the output at all, I would vote for "CREATE > TABLE " so as to preserve the rowcount functionality. Keep in > mind though that this would force client-side changes, for instance in > libpq's PQcmdTuples(). Fixing that one routine isn't so painful, but > what of other client-side libraries, not to mention applications? Doesn't seem worth it to me. At least, "SELECT " makes some sense: rows were selected. "CREATE TABLE " means what? tables were created? What might make sense is to delegate this additional information to separate fields in a future protocol revision. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Mon, Mar 19, 2012 at 12:45 PM, Tom Lane wrote: > Robert Haas writes: >> On Sun, Mar 18, 2012 at 2:29 PM, Tom Lane wrote: >>> 1. ExecuteQuery still has to know that's a CREATE TABLE AS operation so >>> that it can enforce that the prepared query is a SELECT. (BTW, maybe >>> this should be weakened to "something that returns tuples", in view of >>> RETURNING?) > >> +1 for "something that returns with tuples". CREATE TABLE ... AS >> DELETE FROM ... WHERE ... RETURNING ... seems like a cool thing to >> support. > > For the moment I've backed off that idea. The main definitional > question we'd have to resolve is whether we want to allow WITH NO DATA, > and if so what does that mean (should the DELETE execute, or not?). > I am also not certain that the RETURNING code paths would cope with > a WITH OIDS specification, and there are some other things that would > need fixed. It might be cool to do it sometime, but it's not going to > happen in this patch. Fair enough. It would be nice to have, but it definitely does not seem worth spending a lot of time on right now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Robert Haas writes: > On Sun, Mar 18, 2012 at 2:29 PM, Tom Lane wrote: >> 1. ExecuteQuery still has to know that's a CREATE TABLE AS operation so >> that it can enforce that the prepared query is a SELECT. (BTW, maybe >> this should be weakened to "something that returns tuples", in view of >> RETURNING?) > +1 for "something that returns with tuples". CREATE TABLE ... AS > DELETE FROM ... WHERE ... RETURNING ... seems like a cool thing to > support. For the moment I've backed off that idea. The main definitional question we'd have to resolve is whether we want to allow WITH NO DATA, and if so what does that mean (should the DELETE execute, or not?). I am also not certain that the RETURNING code paths would cope with a WITH OIDS specification, and there are some other things that would need fixed. It might be cool to do it sometime, but it's not going to happen in this patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Sun, Mar 18, 2012 at 2:29 PM, Tom Lane wrote: > 1. ExecuteQuery still has to know that's a CREATE TABLE AS operation so > that it can enforce that the prepared query is a SELECT. (BTW, maybe > this should be weakened to "something that returns tuples", in view of > RETURNING?) +1 for "something that returns with tuples". CREATE TABLE ... AS DELETE FROM ... WHERE ... RETURNING ... seems like a cool thing to support. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Sunday, March 18, 2012 07:29:30 PM Tom Lane wrote: > BTW, I've been looking through how to do what I suggested earlier to get > rid of the coziness and code duplication between CreateTableAs and the > prepare.c code; namely, let CreateTableAs create a DestReceiver and then > call ExecuteQuery with that receiver. It appears that we still need at > least two bits of added complexity with that approach: > > 1. ExecuteQuery still has to know that's a CREATE TABLE AS operation so > that it can enforce that the prepared query is a SELECT. (BTW, maybe > this should be weakened to "something that returns tuples", in view of > RETURNING?) I don't really see the use case but given the amount of work it probably takes it seems reasonable to allow that. > 2. Since ExecuteQuery goes through the Portal machinery, there's no way > for it to pass in eflags to control the table OIDs setup. This is > easily solved by adding an eflags parameter to PortalStart, which > doesn't seem too awful to me, since the typical caller would just pass > zero. (ExecuteQuery would also have to know about testing > interpretOidsOption to compute the eflags to use, unless we add an > eflags parameter to it, which might be the cleaner option.) If we go down this route I think adding an eflag is the better choice. Thinking of it - my patch already added that ;) > In short I'm thinking: add an eflags parameter to PortalStart, and add > both an eflags parameter and a "bool mustReturnTuples" parameter to > ExecuteQuery. This definitely seems cleaner than the current > duplication of code. Hm. I am not *that* convinced anymore. It wasn't that much duplication in the end... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown writes: > Will you also be committing the trigger function variable changes > shortly? I don't wish to test anything prior to this as that will > involve a complete re-test from my side anyway. It's on its way, I had to spend time elsewhere, sorry about that. With some luck I can post a intermediate patch later this evening limited to PLpgSQL support for your testing. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Peter Eisentraut writes: > On lör, 2012-03-17 at 18:04 -0400, Tom Lane wrote: >> I'm not sure what we should do instead. We have gotten push-back before >> anytime we changed the command tag for an existing command (and in fact >> it seems that we intentionally added the rowcount display in 9.0, which >> means there are people out there who care about that functionality). >> On the other hand, the traditional output for CREATE TABLE AS doesn't >> seem to satisfy the principle of least astonishment. A third >> consideration is that if we are pushing CREATE TABLE AS as the preferred >> spelling, people will probably complain if it omits functionality that >> SELECT INTO provides; so I'm not sure that "SELECT n" in one case and >> "CREATE TABLE AS" in the other would be a good idea either. Any >> opinions what to do here? > Another consideration is that the SQL command tags are defined by the > SQL standard. So if we were to change it, then it should be "CREATE > TABLE". I'm not convinced that it should be changed, though. (I recall > cross-checking our list against the SQL standard in the past, so there > might have been discussion on this already.) If we were going to change the output at all, I would vote for "CREATE TABLE " so as to preserve the rowcount functionality. Keep in mind though that this would force client-side changes, for instance in libpq's PQcmdTuples(). Fixing that one routine isn't so painful, but what of other client-side libraries, not to mention applications? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Dimitri Fontaine writes: > That lights a bulb: what about rewriting CREATE TABLE AS as two > commands, internally: Given the compatibility constraints on issues like what command tag to return, I think that would probably make our jobs harder not easier. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On lör, 2012-03-17 at 18:04 -0400, Tom Lane wrote: > I'm not sure what we should do instead. We have gotten push-back before > anytime we changed the command tag for an existing command (and in fact > it seems that we intentionally added the rowcount display in 9.0, which > means there are people out there who care about that functionality). > On the other hand, the traditional output for CREATE TABLE AS doesn't > seem to satisfy the principle of least astonishment. A third > consideration is that if we are pushing CREATE TABLE AS as the preferred > spelling, people will probably complain if it omits functionality that > SELECT INTO provides; so I'm not sure that "SELECT n" in one case and > "CREATE TABLE AS" in the other would be a good idea either. Any > opinions what to do here? Another consideration is that the SQL command tags are defined by the SQL standard. So if we were to change it, then it should be "CREATE TABLE". I'm not convinced that it should be changed, though. (I recall cross-checking our list against the SQL standard in the past, so there might have been discussion on this already.) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Tom Lane writes: > 1. ExecuteQuery still has to know that's a CREATE TABLE AS operation so > that it can enforce that the prepared query is a SELECT. (BTW, maybe > this should be weakened to "something that returns tuples", in view of > RETURNING?) That lights a bulb: what about rewriting CREATE TABLE AS as two commands, internally: 1. CREATE TABLE … 2. WITH x ( ) INSERT INTO … SELECT * FROM x; It seems like not solving much from a practical point of view though as you need to be able to parse the output of the subquery before you can do the first step, but on the other hand it might be that you already have the pieces to just do that. Well, I had to share that though, somehow. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
BTW, I've been looking through how to do what I suggested earlier to get rid of the coziness and code duplication between CreateTableAs and the prepare.c code; namely, let CreateTableAs create a DestReceiver and then call ExecuteQuery with that receiver. It appears that we still need at least two bits of added complexity with that approach: 1. ExecuteQuery still has to know that's a CREATE TABLE AS operation so that it can enforce that the prepared query is a SELECT. (BTW, maybe this should be weakened to "something that returns tuples", in view of RETURNING?) 2. Since ExecuteQuery goes through the Portal machinery, there's no way for it to pass in eflags to control the table OIDs setup. This is easily solved by adding an eflags parameter to PortalStart, which doesn't seem too awful to me, since the typical caller would just pass zero. (ExecuteQuery would also have to know about testing interpretOidsOption to compute the eflags to use, unless we add an eflags parameter to it, which might be the cleaner option.) In short I'm thinking: add an eflags parameter to PortalStart, and add both an eflags parameter and a "bool mustReturnTuples" parameter to ExecuteQuery. This definitely seems cleaner than the current duplication of code. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Saturday, March 17, 2012 11:04:30 PM Tom Lane wrote: > I've found a couple more issues in the CTAS patch: > > 1. Previous versions delivered a "SELECT n" command tag for either > spelling of the command: > > regression=# select * into t1 from int8_tbl; > SELECT 6 > regression=# create table t2 as select * from int8_tbl; > SELECT 6 > > With the patch I get > > regression=# select * into t1 from int8_tbl; > SELECT 0 0 hm. Stupid me. > regression=# create table t2 as select * from int8_tbl; > CREATE TABLE AS > > The first of these is particularly unfortunate since it's outright lying > as to the number of rows processed. > I'm not sure what we should do instead. We have gotten push-back before > anytime we changed the command tag for an existing command (and in fact > it seems that we intentionally added the rowcount display in 9.0, which > means there are people out there who care about that functionality). > On the other hand, the traditional output for CREATE TABLE AS doesn't > seem to satisfy the principle of least astonishment. A third > consideration is that if we are pushing CREATE TABLE AS as the preferred > spelling, people will probably complain if it omits functionality that > SELECT INTO provides; so I'm not sure that "SELECT n" in one case and > "CREATE TABLE AS" in the other would be a good idea either. Any > opinions what to do here? I would prefer both returning CREATE TABLE AS since thats what actually happens. We already document that SELECT INTO is kinda deprecated: "The PostgreSQL usage of SELECT INTO to represent table creation is historical. It is best to use CREATE TABLE AS for this purpose in new code." I have seen code that uses the command code for selecting the, app level, logging. Its kinda hard to do that if a CREATE TABLE AS/SELECT INTO returns SELECT. Does CTAS ommit any functionality currently? I don't see any reason not to support stuff there thats supported in SELECT INTO. > 2. Historically, CREATE RULE has allowed a rule action to be SELECT INTO > (though not CREATE TABLE AS). Currently the patch is throwing an error > for that. This seems like something that might not be worth fixing, > though. It's fairly hard to conceive of a use-case for such a rule, > since it would work only once before starting to throw "table already > exists" errors. How much do we care about preserving backward > compatibility here? I vote for not supporting that anymore. That being possible looks more like an implementation detail to me. Thanks for looking at this! Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
I've found a couple more issues in the CTAS patch: 1. Previous versions delivered a "SELECT n" command tag for either spelling of the command: regression=# select * into t1 from int8_tbl; SELECT 6 regression=# create table t2 as select * from int8_tbl; SELECT 6 With the patch I get regression=# select * into t1 from int8_tbl; SELECT 0 0 regression=# create table t2 as select * from int8_tbl; CREATE TABLE AS The first of these is particularly unfortunate since it's outright lying as to the number of rows processed. I'm not sure what we should do instead. We have gotten push-back before anytime we changed the command tag for an existing command (and in fact it seems that we intentionally added the rowcount display in 9.0, which means there are people out there who care about that functionality). On the other hand, the traditional output for CREATE TABLE AS doesn't seem to satisfy the principle of least astonishment. A third consideration is that if we are pushing CREATE TABLE AS as the preferred spelling, people will probably complain if it omits functionality that SELECT INTO provides; so I'm not sure that "SELECT n" in one case and "CREATE TABLE AS" in the other would be a good idea either. Any opinions what to do here? 2. Historically, CREATE RULE has allowed a rule action to be SELECT INTO (though not CREATE TABLE AS). Currently the patch is throwing an error for that. This seems like something that might not be worth fixing, though. It's fairly hard to conceive of a use-case for such a rule, since it would work only once before starting to throw "table already exists" errors. How much do we care about preserving backward compatibility here? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Saturday, March 17, 2012 06:45:27 PM Tom Lane wrote: > I'm not sure that anybody cares about being able to fire command > triggers on DECLARE CURSOR I actually think it would make sense to explicitly not fire command triggers there given that DECLARE CURSOR actually potentially is somewhat performance relevant. > , but just from a consistency standpoint it > would make sense for this to work more like EXPLAIN and CREATE TABLE AS. > So that convinces me that UtilityContainsQuery() would be a good thing, > and I'll add that to the patch. (Actually changing DECLARE CURSOR seems > like a separate patch, though.) Aggreed on that. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
While looking at this I also noticed that DECLARE CURSOR uses a structure that's randomly different in yet a third way: we start with a utility statement containing a query, and then flip that upside down so that the SELECT Query contains a utility statement! I have a vague feeling that I'm the one who's guilty of that hack, too. I'm not sure that anybody cares about being able to fire command triggers on DECLARE CURSOR, but just from a consistency standpoint it would make sense for this to work more like EXPLAIN and CREATE TABLE AS. So that convinces me that UtilityContainsQuery() would be a good thing, and I'll add that to the patch. (Actually changing DECLARE CURSOR seems like a separate patch, though.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Andres Freund writes: > On Friday, March 16, 2012 10:52:55 PM Tom Lane wrote: >> Something else I just came across is that there are assorted places that >> are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c, >> and those have got to treat CreateTableAsStmt similarly. > Hm. Is that so? As implemented in my version the planner just sees a plain > statement instead of a utility statement. Am I missing something? Well, the work flow for EXPLAIN is: parse analysis: recursively do parse analysis on contained query plan: do nothing execution: call planner on contained query, then optionally run it and the reason for doing it that way is explained by transformExplainStmt: * EXPLAIN is like other utility statements in that we emit it as a * CMD_UTILITY Query node; however, we must first transform the contained * query. We used to postpone that until execution, but it's really necessary * to do it during the normal parse analysis phase to ensure that side effects * of parser hooks happen at the expected time. ISTM that argument applies just as much to CREATE TABLE AS, especially in view of the fact that we're restructuring the SELECT INTO case, in which parse analysis of the SELECT certainly does happen early. It's also not clear to me why it wouldn't apply to COPY (SELECT ...). I'm not going to touch the COPY (SELECT ...) issue right now, but somebody ought to go back and check up on the exact user-visible bugs that motivated moving EXPLAIN's parse analysis processing. (I suspect it had to do with plpgsql variable processing, but too lazy to go look right now.) If there's a plausible use case where similar bugs could be exhibited in COPY, we're going to have to restructure that too. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Friday, March 16, 2012 10:52:55 PM Tom Lane wrote: > Andres Freund writes: > > On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote: > >> I'm thinking that if the table creation > >> were to be moved into the tuple receiver's startup routine, we could > >> avoid needing to get control back between ExecutorStartup and > >> ExecutorRun, and then all that would be required would be to call > >> ExecuteQuery with a different DestReceiver. > > > > Hm. I seriously dislike doing that in the receiver. I can't really point > > out why though. Unsurprisingly I like getting the control back to > > CreateTableAs... > > I don't see the argument. Receiver startup functions are intended to do > exactly this type of thing. I'd be okay with leaving it in > CreateTableAs if it didn't contort the code to do so, but what we have > here is precisely that we've got to contort the interface with prepare.c > to do it that way. Hm. > (It also occurs to me that moving this work into the DestReceiver might > make things self-contained enough that we could continue to support > EXPLAIN SELECT INTO with not an undue amount of pain.) > Something else I just came across is that there are assorted places that > are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c, > and those have got to treat CreateTableAsStmt similarly. Hm. Is that so? As implemented in my version the planner just sees a plain statement instead of a utility statement. Am I missing something? I am not even sure why the planner ever needs to see ExplainStmts? Protocol level prepares? Shouldn't those top level nodes be simply removed once? > We could just > add more code in each of those places. I'm wondering though if it would > be a good idea to invent an abstraction layer, to wit a utility.c > function along the lines of "Query *UtilityContainsQuery(Node > *parsetree)", which would centralize the knowledge of exactly which > utility statements are like this and how to dig the Query out of them. > It's only marginally attractive unless there's a foreseeable reason > why we'd someday have more than two of them; but on the other hand, > just formalizing the concept that some utility statements are like > this might be a good thing. If its really necessary to do that I think that would be a good idea. Alone the increased greppablility/readablility seems to be worth it. > (Actually, as I type this I wonder whether > COPY (SELECT ...) isn't a member of this class too, and whether we don't > have bugs from the fact that it's not being treated like EXPLAIN.) > Thoughts? Hm. On a first glance the planner also never sees the content of a CopyStmt... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Andres Freund writes: > On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote: >> I'm thinking that if the table creation >> were to be moved into the tuple receiver's startup routine, we could >> avoid needing to get control back between ExecutorStartup and >> ExecutorRun, and then all that would be required would be to call >> ExecuteQuery with a different DestReceiver. > Hm. I seriously dislike doing that in the receiver. I can't really point out > why though. Unsurprisingly I like getting the control back to CreateTableAs... I don't see the argument. Receiver startup functions are intended to do exactly this type of thing. I'd be okay with leaving it in CreateTableAs if it didn't contort the code to do so, but what we have here is precisely that we've got to contort the interface with prepare.c to do it that way. (It also occurs to me that moving this work into the DestReceiver might make things self-contained enough that we could continue to support EXPLAIN SELECT INTO with not an undue amount of pain.) Something else I just came across is that there are assorted places that are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c, and those have got to treat CreateTableAsStmt similarly. We could just add more code in each of those places. I'm wondering though if it would be a good idea to invent an abstraction layer, to wit a utility.c function along the lines of "Query *UtilityContainsQuery(Node *parsetree)", which would centralize the knowledge of exactly which utility statements are like this and how to dig the Query out of them. It's only marginally attractive unless there's a foreseeable reason why we'd someday have more than two of them; but on the other hand, just formalizing the concept that some utility statements are like this might be a good thing. (Actually, as I type this I wonder whether COPY (SELECT ...) isn't a member of this class too, and whether we don't have bugs from the fact that it's not being treated like EXPLAIN.) Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote: > Andres Freund writes: > > One more thing I disliked quite a bit was the duplication of the EXECUTE > > handling. Do you see a way to deduplicate that? > Yeah, that's what's bugging me, too. I think a chunk of the problem is > that you're insisting on having control come back to CreateTableAs() > to perform the table creation. I'm thinking that if the table creation > were to be moved into the tuple receiver's startup routine, we could > avoid needing to get control back between ExecutorStartup and > ExecutorRun, and then all that would be required would be to call > ExecuteQuery with a different DestReceiver. Hm. I seriously dislike doing that in the receiver. I can't really point out why though. Unsurprisingly I like getting the control back to CreateTableAs... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Andres Freund writes: > One more thing I disliked quite a bit was the duplication of the EXECUTE > handling. Do you see a way to deduplicate that? Yeah, that's what's bugging me, too. I think a chunk of the problem is that you're insisting on having control come back to CreateTableAs() to perform the table creation. I'm thinking that if the table creation were to be moved into the tuple receiver's startup routine, we could avoid needing to get control back between ExecutorStartup and ExecutorRun, and then all that would be required would be to call ExecuteQuery with a different DestReceiver. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Friday, March 16, 2012 09:54:47 PM Tom Lane wrote: > Andres Freund writes: > > [ ctas-01.patch ] > > I'm starting to look at this now. Great! > For a patch that's supposed to de-complicate things, it seems pretty messy :-( Yea. It started out simple but never stopped getting more complicated. Getting rid of the duplication still seems to make sense to me though. > One thing I soon found is that it lacks support for EXPLAIN SELECT INTO. > That used to work, but now you get > > regression=# explain select * into foo from tenk1; > ERROR: INTO is only allowed on first SELECT of UNION/INTERSECT/EXCEPT > LINE 1: explain select * into foo from tenk1; > ^ > > and while fixing the parse analysis for that is probably not too hard, > it would still fail to work nicely, since explain.c lacks support for > CreateTableAsStmt utility statements.I think we'd have to invent > something parallel to ExplainExecuteQuery to support this, and I really > doubt that it's worth the trouble. Does anyone have a problem with > desupporting the case? I am all for removing it. > Also, it seems to me that the patch is spending way too much effort on > trying to exactly duplicate the old error messages for various flavors > of "INTO not allowed here", and still not succeeding terribly well. > I'm inclined to just have a one-size-fits-all message in > transformSelectStmt, which will fire if intoClause hasn't been cleared > before we get there; any objections? I was/am decidedly unhappy about the whole error message stuff. Thats what turned the patch from removing lines to making it bigger than before. I tried to be compatible to make sure I understood what was happening. I am fine with making it one simple error message. > A couple of other cosmetic thoughts: I'm tempted to split the execution > support out into a new file, rather than bloat tablecmds.c any further; > and I'm wondering if the interface to EXECUTE INTO can't be simplified. > (It may have been a mistake to remove the IntoClause in ExecuteStmt.) Hm. I vote against keeping the IntoClause stuff in ExecuteStmt. Splitting stuff from tablecmd.c seems sensible though. One more thing I disliked quite a bit was the duplication of the EXECUTE handling. Do you see a way to deduplicate that? If you give me some hints about what to change I am happy to revise the patch myself should you prefer that. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Andres Freund writes: > [ ctas-01.patch ] I'm starting to look at this now. For a patch that's supposed to de-complicate things, it seems pretty messy :-( One thing I soon found is that it lacks support for EXPLAIN SELECT INTO. That used to work, but now you get regression=# explain select * into foo from tenk1; ERROR: INTO is only allowed on first SELECT of UNION/INTERSECT/EXCEPT LINE 1: explain select * into foo from tenk1; ^ and while fixing the parse analysis for that is probably not too hard, it would still fail to work nicely, since explain.c lacks support for CreateTableAsStmt utility statements. I think we'd have to invent something parallel to ExplainExecuteQuery to support this, and I really doubt that it's worth the trouble. Does anyone have a problem with desupporting the case? Also, it seems to me that the patch is spending way too much effort on trying to exactly duplicate the old error messages for various flavors of "INTO not allowed here", and still not succeeding terribly well. I'm inclined to just have a one-size-fits-all message in transformSelectStmt, which will fire if intoClause hasn't been cleared before we get there; any objections? A couple of other cosmetic thoughts: I'm tempted to split the execution support out into a new file, rather than bloat tablecmds.c any further; and I'm wondering if the interface to EXECUTE INTO can't be simplified. (It may have been a mistake to remove the IntoClause in ExecuteStmt.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 14 March 2012 21:33, Dimitri Fontaine wrote: > Ok, I've implemented that. No patch attached because I need to merge > with master again and I'm out to sleep now, it sometimes ring when being > on-call… > > Curious people might have a look at my github repository where the > command_triggers branch is updated: Will you also be committing the trigger function variable changes shortly? I don't wish to test anything prior to this as that will involve a complete re-test from my side anyway. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Robert Haas writes: >> Also, when calling the user's procedure from the same place in case of an >> ANY command trigger or a specific one it's then possible to just hand >> them over the exact same set of info (object id, name, schema name). > > Yes, I think that's an essential property of the system, here. Ok, I've implemented that. No patch attached because I need to merge with master again and I'm out to sleep now, it sometimes ring when being on-call… Curious people might have a look at my github repository where the command_triggers branch is updated: https://github.com/dimitri/postgres/compare/daf69e1e...e3714cb9e6 Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Wed, Mar 14, 2012 at 4:27 AM, Dimitri Fontaine wrote: > Also, when calling the user's procedure from the same place in case of an > ANY command trigger or a specific one it's then possible to just hand > them over the exact same set of info (object id, name, schema name). Yes, I think that's an essential property of the system, here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Robert Haas writes: > On Tue, Mar 13, 2012 at 5:06 PM, Andres Freund wrote: >> Generally, uppon rereading, I have to say that I am not very happy with the >> decision that ANY triggers are fired from other places than the specific >> triggers. That seams to be a rather dangerous/confusing route to me. > > I agree. I think that's a complete non-starter. Ok, well, let me react in 2 ways here: A. it's very easy to change and will simplify the code B. it's been done this way for good reasons (at the time) Specifically, I've been asked to implement the feature of blocking all and any DDL activity on a machine in a single command, and we don't have support for triggers on all commands (remember shared objects). Now, as I've completed support for all interesting commands the discrepancy between what's supported in the ANY case and in the specific command case has reduced. If you're saying to nothing, that's good news. Also, when calling the user's procedure from the same place in case of an ANY command trigger or a specific one it's then possible to just hand them over the exact same set of info (object id, name, schema name). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Tue, Mar 13, 2012 at 5:06 PM, Andres Freund wrote: > Generally, uppon rereading, I have to say that I am not very happy with the > decision that ANY triggers are fired from other places than the specific > triggers. That seams to be a rather dangerous/confusing route to me. I agree. I think that's a complete non-starter. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Tuesday, March 13, 2012 09:07:32 PM Dimitri Fontaine wrote:
> Hi,
>
> Andres Freund writes:
> > I did a short review of what I found after merging master
>
> Thanks!
>
> > - I still find it strange not to fire on cascading actions
>
> We don't build statement for cascading so we don't fire command
> triggers. The user view is that there was no drop command on the sub
> objects, only on the main one.
>
> I know it's not ideal, but that's a limit we have to bite for 9.2
> unfortunately.
Hm. Especially in partially replicated scenarios I think that will bite. But
then: There will be a 9.3 at some point ;)
> > - I dislike the missing locking leading to strange errors uppon
> > concurrent changes. But then thats just about all the rest of commands/*
> > is handling it...
> >
> > - I think list_command_triggers should do a
> > heap_lock_tuple(LockTupleShared)
> >
> > on the command trigger tuple. But then again just about nothing else
> > does :(
>
> As you say, about nothing else does. I think that's a work for another
> patch.
Not sure, that way the required work is getting bigger and bigger. But I can
live with that... I think the command trigger work will make better
concurrency safeness of DDL necessary.
> > 2. the switch to cmd->oldmctx made me very wary at first because I wasn't
> > sure its guaranteed to be non NULL
> >
> > - why is there a special CommandTriggerContext if its not reset
> > separately? Should it be reset? I have to say that I dislike the api
> > around this.
>
> Some call sites need to be able to call those functions a dynamic number
> of times. I could add a reset boolean parameter that would mostly be
> true in all call site and false in two of them (RemoveObjects,
> RemoveRelations), and add a new function to just reset the memory
> context then.
> Or maybe you have a better idea about the ideal API here?
I wonder if the answer is making the API more symmetric. Seems more future-
proof in combination to being cleaner.
//create a new memory context
InitCommandContext(cmd);
if(CommandFiresTriggers(cmd)){
//still in current memory context, after all not much memory should be
allocated here
cmd.foo = bar;
//switches memory context during function execution, resets it afterwards
ExecBeforeCommandTriggers(cmd);
}
if(CommandFiresTriggers(cmd)){
cmd.zap = blub;
ExecAfterCommandTriggers(cmd);
}
//drop the memory context
CleanupCommandContext(cmd);
I find the changing of memory context in CommandFires[After]Trigger + switch
back in Exec*CommandTrigger rather bad style and I don't really see the point
anyway.
> > - an AFTER .. ALTER AGGREATE ... SET SCHEMA has the wrong schema.
> > Probably the same problem exists elsewhere. Or is that as-designed?
> > Would be inconsistent with the way object names are handled.
>
> I'm surprised, here's an excerpt from the added regression tests:
>
> alter function notfun(int) set schema cmd;
> NOTICE: snitch: BEFORE any ALTER FUNCTION
> NOTICE: snitch: BEFORE ALTER FUNCTION public notfun
> NOTICE: snitch: AFTER ALTER FUNCTION cmd notfun
> NOTICE: snitch: AFTER any ALTER FUNCTION
I was not looking at ALTER FUNCTION but ALTER AGGREGATE. And I looked wrongly.
Sorry for that.
Generally, uppon rereading, I have to say that I am not very happy with the
decision that ANY triggers are fired from other places than the specific
triggers. That seams to be a rather dangerous/confusing route to me.
Especially because sometimes errors (permissions, duplicated names, etc) are
raised differently in ANY than in specific triggers now:
postgres=# ALTER AGGREGATE bar.array_agg_union3(anyarray) SET SCHEMA public;
NOTICE: when BEFORE, tag ALTER AGGREGATE, objectid , schemaname ,
objectname
ERROR: aggregate bar.array_agg_union3(anyarray) does not exist
postgres=# ALTER AGGREGATE public.array_agg_union3(anyarray) SET SCHEMA
public;
NOTICE: when BEFORE, tag ALTER AGGREGATE, objectid , schemaname ,
objectname
ERROR: function array_agg_union3(anyarray) is already in schema "public"
postgres=# ALTER AGGREGATE public.array_agg_union3(anyarray) SET SCHEMA bar;
NOTICE: when BEFORE, tag ALTER AGGREGATE, objectid , schemaname ,
objectname
NOTICE: when BEFORE, tag ALTER AGGREGATE, objectid 16415, schemaname public,
objectname array_agg_union3
NOTICE: when AFTER, tag ALTER AGGREGATE, objectid 16415, schemaname bar,
objectname array_agg_union3
NOTICE: when AFTER, tag ALTER AGGREGATE, objectid , schemaname ,
objectname
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Hi,
Andres Freund writes:
> I did a short review of what I found after merging master
Thanks!
> - I still find it strange not to fire on cascading actions
We don't build statement for cascading so we don't fire command
triggers. The user view is that there was no drop command on the sub
objects, only on the main one.
I know it's not ideal, but that's a limit we have to bite for 9.2
unfortunately.
> - I dislike the missing locking leading to strange errors uppon concurrent
> changes. But then thats just about all the rest of commands/* is handling
> it...
>
> - I think list_command_triggers should do a heap_lock_tuple(LockTupleShared)
> on the command trigger tuple. But then again just about nothing else does :(
As you say, about nothing else does. I think that's a work for another
patch.
> - ExecBeforeOrInsteadOfCommandTriggers is referenced in
> exec_command_triggers_internal comments
> - InitCommandContext comments are outdated
> - generally comments look a bit outdated
Fixed.
> - shouldn't the command trigger stuff for ALTER TABLE be done in inside
> AlterTable instead of utility.c?
Right, done.
> - you have repetitions of the following pattern:
> void
> ExecBeforeCommandTriggers(CommandContext cmd)
> {
> /* that will execute under command trigger memory context */
> if (cmd != NULL && cmd->before != NIL)
> exec_command_triggers_internal(cmd, cmd->before, "BEFORE");
>
> /* switch back to the command Memory Context now */
> MemoryContextSwitchTo(cmd->oldmctx);
> }
>
> 1. Either cmd != NULL does not need to be checked or you need to check it
> before the MemoryContextSwitchTo
I've fixed that code.
> 2. the switch to cmd->oldmctx made me very wary at first because I wasn't sure
> its guaranteed to be non NULL
>
> - why is there a special CommandTriggerContext if its not reset separately?
> Should it be reset? I have to say that I dislike the api around this.
Some call sites need to be able to call those functions a dynamic number
of times. I could add a reset boolean parameter that would mostly be
true in all call site and false in two of them (RemoveObjects,
RemoveRelations), and add a new function to just reset the memory
context then.
Or maybe you have a better idea about the ideal API here?
> - an AFTER .. ALTER AGGREATE ... SET SCHEMA has the wrong schema. Probably the
> same problem exists elsewhere. Or is that as-designed? Would be inconsistent
> with the way object names are handled.
I'm surprised, here's an excerpt from the added regression tests:
alter function notfun(int) set schema cmd;
NOTICE: snitch: BEFORE any ALTER FUNCTION
NOTICE: snitch: BEFORE ALTER FUNCTION public notfun
NOTICE: snitch: AFTER ALTER FUNCTION cmd notfun
NOTICE: snitch: AFTER any ALTER FUNCTION
> - what does that mean?
> + cmd.objectname = NULL; /* composite object name */
User mapping and casts object names are composite, and I don't know how
to represent that in a single text structure.
> - DropPropertyStmt seems to be an unused leftover?
Seems so, cleaned out.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Excerpts from Andres Freund's message of mar mar 13 08:22:26 -0300 2012: > - I think list_command_triggers should do a heap_lock_tuple(LockTupleShared) > on the command trigger tuple. But then again just about nothing else does :( If you want to do something like that, I think it's probably more appropriate to use LockDatabaseObject than heap_lock_tuple. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Hi,
I did a short review of what I found after merging master
(b4af1c25bbc636379efc5d2ffb9d420765705b8a) to what I currently fetched from
your repo (d63df64580114de4d83cfe8eb45eb630724b8b6f).
- I still find it strange not to fire on cascading actions
- I dislike the missing locking leading to strange errors uppon concurrent
changes. But then thats just about all the rest of commands/* is handling
it...
T1:
BEGIN;
ALTER COMMAND TRIGGER cmd_before SET DISABLE;
T2:
BEGIN;
ALTER COMMAND TRIGGER cmd_before SET DISABLE;
T1:
COMMIT;
T2:
ERROR: tuple concurrently updated
- I think list_command_triggers should do a heap_lock_tuple(LockTupleShared)
on the command trigger tuple. But then again just about nothing else does :(
- ExecBeforeOrInsteadOfCommandTriggers is referenced in
exec_command_triggers_internal comments
- InitCommandContext comments are outdated
- generally comments look a bit outdated
- shouldn't the command trigger stuff for ALTER TABLE be done in inside
AlterTable instead of utility.c?
- you have repetitions of the following pattern:
void
ExecBeforeCommandTriggers(CommandContext cmd)
{
/* that will execute under command trigger memory context */
if (cmd != NULL && cmd->before != NIL)
exec_command_triggers_internal(cmd, cmd->before, "BEFORE");
/* switch back to the command Memory Context now */
MemoryContextSwitchTo(cmd->oldmctx);
}
1. Either cmd != NULL does not need to be checked or you need to check it
before the MemoryContextSwitchTo
2. the switch to cmd->oldmctx made me very wary at first because I wasn't sure
its guaranteed to be non NULL
- why is there a special CommandTriggerContext if its not reset separately?
Should it be reset? I have to say that I dislike the api around this.
- an AFTER .. ALTER AGGREATE ... SET SCHEMA has the wrong schema. Probably the
same problem exists elsewhere. Or is that as-designed? Would be inconsistent
with the way object names are handled.
- what does that mean?
+ cmd.objectname = NULL; /* composite object name */
- DropPropertyStmt seems to be an unused leftover?
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 9 March 2012 21:38, Dimitri Fontaine wrote: > Hi, > > Please find attached v15 of the patch, addressing all known issues apart > from the trigger function argument passing style. Expect a new patch > with that taken care of early next week. > > (The github branch too, should you be using that) > > Thom Brown writes: >> CREATE OPERATOR CLASS now shows objectname as 'hash' instead of its >> name where it's declared as "USING hash". This isn't a problem with >> ALTER/DROP OPERATOR CLASS. Everything else above works as expected >> now. > > Ah yes that needed a special case, it's properly handled now, and > tested. Confirmed. When dropping domains, the name of the domain includes the schema name: > > Fixed. Confirmed. >> Could we change this to "REINDEX DATABASE triggers are not supported"? >> This way it would be consistent with the "AFTER CREATE INDEX >> CONCURRENTLY" warning. > > Sure, done. Confirmed, thanks. REINDEX on a table seems to show no schema name but an object name for specific triggers: > > Was a typo, fixed. Confirmed When REINDEXing an index rather than a table, the table's details are shown in the trigger. Is this expected?: > > Fixed. Confirmed. >> ALTER CAST is still listed and needs removing, not just from the >> documentation but every place it's used your code too. I can >> currently create a trigger for it, but it's impossible for it to fire >> since there's no such command. > > Removed. Confirmed that it's removed from the code. Just needs removing from the docs too now. (doc/src/sgml/ref/create_command_trigger.sgml) >> All these corrections I mentioned previously still needs to be made: > > That's about the docs, I edited them accordingly to your comments. Confirmed. >>> Thom Brown writes: >> All other command triggers don't fire on read-only standbys, and the >> inconsistency doesn't seem right. On the one hand all >> CREATE/DROP/ALTER triggers aren't fired because of the "cannot execute >> in a read-only transaction" error message, but triggers do >> occur before utility commands, which would otherwise display the same >> message, and might not display it at all if the trigger causes an >> error in its function call. So it seems like they should either all >> fire, or none of them should. What are you thoughts? > > The others trigger don't fire because an ERROR case is detected before > they have a chance to run, much like on a primary in some ERROR cases. Yes, I've since discovered that I shouldn't have been treating them equally due to the different type of error those sets of commands generate. That's fine then. > Thom Brown writes: >> It was late last night and I forgot to get around to testing pg_dump, >> which isn't working correctly: > > Fixed. Confirmed. >> Also I notice that CREATE/ALTER/DROP COMMAND TRIGGER appears just >> before CREATE/ALTER/DROP TRIGGER in the documentation. This breaks >> the alphabetical order and I wasn't expecting to find it there when >> scanning down the page. Could we move them into an alphabetic >> position? > > I don't see that problem in the source files, could you be more specific? I can't see the problem in the source either, but when viewing postgresql/doc/src/sgml/html/reference.html in my browser, CREATE COMMAND TRIGGER appears between CREATE TRIGGER and CREATE TYPE. Not sure why though. Unless you're cleverly distracted me away from some issue that's yet to be resolved, that appears to be nearly all my concerns addressed. All that appears to remain is the trigger-variable stuff which you said shall arrive early next week, and also the that odd doc issue I mentioned in the paragraph above (although that could just be something weird happening when I build it). Sounds like I have the weekend off. :) Thanks Dimitri. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Fri, Mar 9, 2012 at 12:51 PM, Dimitri Fontaine wrote: > Robert Haas writes: >> I think we had better look seriously at postponing this patch to 9.3. > > I understand why you're drawing that conclusion, but I don't think > that's the best we can do here, by a long shot. Well, if you get to the point where you're done churning the code in the next week or so, I'm willing to do one or two more rounds of serious review, but if that doesn't get us there then I think we need to give up. The energy you've put into this is commendable, but we're about to start the third month of this CommitFest, and until we get this release at least to beta or so, we can't start any new CommitFests or branch the tree. That basically means that nothing else of mine is going to get committed until the current crop of patches are dealt with - or for a good while after, for that matter, but getting the current crop of patches dealt with is the first step. Of course, I also want to have a good release and I understand the necessity of spending time on other people's patches as well as my own, as I believe I've demonstrated, but I don't want to stay in that mode indefinitely, which I think is an understandable position. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Robert Haas writes: > I think we had better look seriously at postponing this patch to 9.3. I understand why you're drawing that conclusion, but I don't think that's the best we can do here, by a long shot. > Your reviewing is obviously moving things forward rapidly, but I think > it's unrealistic to think this is going to be in a committable state > any time in the next week or two. What's happening is that I've been abusing Thom's availability, leaving him with the testing and fixing oddities along the way. Those came mainly from an attempt at being as automatic as possible when writing commands support. I'm now about done reviewing each and every call site and having them covered in the tests. What remains to be done now is how to pass down arguments to the triggers (switching from function arguments to trigger style magic variables, per Tom request), and review REINDEX and CREATE OPERATOR CLASS support. That's about it. The API and the call sites location have been stable for a long time now, and are following your previous round of review. The catalog storage and commands grammar are ok too, we've been hashing them out. We've been very careful about not introducing code path hazards, the only novelty being a new place to ERROR out (no fancy silent utility command execution control). Really, I would think we're about there now. I would be rather surprised not to be able to finish that patch by the end of next week, and will arrange myself to be able to devote more time on it each day if that's what needed. Remember that we intend to build an extension providing a C-coded function doing the heavy lifting of back parsing the command string from the Node parse tree, with the goal of having Slony, Londiste and Bucardo able to use that and implement support for DLLs. I really want that to happen in 2012. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Robert Haas writes: > I'm not convinced. Right now, it's fairly useless - all the triggers > could possibly do is throw an error, and an error is going to get > thrown anyway, so it's only a question of which error message the user > will see. But we discussed before the idea of adding a capability for > BEFORE triggers to request that the actual execution of the command > get skipped, and then it's possible to imagine this being useful. > Someone could even use a command trigger that detects which machine > it's running on, and if it's the standby, uses dblink to execute the > command on the master, or something crazy like that. Command triggers > could also be useful for logging all attempts to execute a particular > command, which is probably still appropriate on the standby. There are some other use cases, like using plsh to go apt-get install an extension's package when you see the master just created it, so that your read only queries on the hot standby have a chance of loading the code you need. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Fri, Mar 9, 2012 at 10:05 AM, Tom Lane wrote: > Robert Haas writes: >> On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown wrote: >>> Sorry, I meant any command trigger. It's because none of the commands >>> can be run on a standby, so the triggers don't seem appropriate. > >> I'm not convinced. Right now, it's fairly useless - all the triggers >> could possibly do is throw an error, and an error is going to get >> thrown anyway, so it's only a question of which error message the user >> will see. But we discussed before the idea of adding a capability for >> BEFORE triggers to request that the actual execution of the command >> get skipped, and then it's possible to imagine this being useful. > > Um, surely the "you can't do that in a read-only session" error is going > to get thrown long before the command trigger could be called? Hmmm yeah. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 9 March 2012 15:05, Tom Lane wrote: > Robert Haas writes: >> On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown wrote: >>> Sorry, I meant any command trigger. It's because none of the commands >>> can be run on a standby, so the triggers don't seem appropriate. > >> I'm not convinced. Right now, it's fairly useless - all the triggers >> could possibly do is throw an error, and an error is going to get >> thrown anyway, so it's only a question of which error message the user >> will see. But we discussed before the idea of adding a capability for >> BEFORE triggers to request that the actual execution of the command >> get skipped, and then it's possible to imagine this being useful. > > Um, surely the "you can't do that in a read-only session" error is going > to get thrown long before the command trigger could be called? Yes, at the moment that's the case. I said that this wasn't the case for utility commands but I've noticed the message is different for those: ERROR: cannot execute VACUUM during recovery vs ERROR: cannot execute CREATE TABLE in a read-only transaction So my complaint around that was misleading and wrong. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Robert Haas writes: > On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown wrote: >> Sorry, I meant any command trigger. It's because none of the commands >> can be run on a standby, so the triggers don't seem appropriate. > I'm not convinced. Right now, it's fairly useless - all the triggers > could possibly do is throw an error, and an error is going to get > thrown anyway, so it's only a question of which error message the user > will see. But we discussed before the idea of adding a capability for > BEFORE triggers to request that the actual execution of the command > get skipped, and then it's possible to imagine this being useful. Um, surely the "you can't do that in a read-only session" error is going to get thrown long before the command trigger could be called? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 9 March 2012 14:47, Robert Haas wrote: > On Fri, Mar 9, 2012 at 9:35 AM, Thom Brown wrote: >> I see your point. My suggestion to Dimitri in another email was >> either enable triggers for all commands or none. At the moment it's >> only available on utility commands. > > Yeah, that's clearly not the best of all possible worlds. :-) > > I think we had better look seriously at postponing this patch to 9.3. > Your reviewing is obviously moving things forward rapidly, but I think > it's unrealistic to think this is going to be in a committable state > any time in the next week or two. That's unfortunate if that's the case. I'll dedicate any bandwidth necessary for additional testing as I would really like to see this get in, but if it transpires there's more outstanding work and polishing needed than time Dimitri personally has available, then I guess it'll have to be a 9.3 feature. :'( -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Fri, Mar 9, 2012 at 9:35 AM, Thom Brown wrote: > I see your point. My suggestion to Dimitri in another email was > either enable triggers for all commands or none. At the moment it's > only available on utility commands. Yeah, that's clearly not the best of all possible worlds. :-) I think we had better look seriously at postponing this patch to 9.3. Your reviewing is obviously moving things forward rapidly, but I think it's unrealistic to think this is going to be in a committable state any time in the next week or two. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 9 March 2012 14:30, Robert Haas wrote: > On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown wrote: >> On 9 March 2012 14:09, Robert Haas wrote: >>> On Wed, Mar 7, 2012 at 4:53 PM, Thom Brown wrote: I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on a read-only standby, the BEFORE ANY COMMAND trigger fires. I don't think any trigger should fire on a read-only standby. >>> >>> Why ever not? >> >> Sorry, I meant any command trigger. It's because none of the commands >> can be run on a standby, so the triggers don't seem appropriate. > > I'm not convinced. Right now, it's fairly useless - all the triggers > could possibly do is throw an error, and an error is going to get > thrown anyway, so it's only a question of which error message the user > will see. But we discussed before the idea of adding a capability for > BEFORE triggers to request that the actual execution of the command > get skipped, and then it's possible to imagine this being useful. > Someone could even use a command trigger that detects which machine > it's running on, and if it's the standby, uses dblink to execute the > command on the master, or something crazy like that. Command triggers > could also be useful for logging all attempts to execute a particular > command, which is probably still appropriate on the standby. > > I think that it will be a good thing to try to treat Hot Standby mode > as much like regular operation as is reasonably possible, across the > board. I see your point. My suggestion to Dimitri in another email was either enable triggers for all commands or none. At the moment it's only available on utility commands. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown wrote: > On 9 March 2012 14:09, Robert Haas wrote: >> On Wed, Mar 7, 2012 at 4:53 PM, Thom Brown wrote: >>> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on >>> a read-only standby, the BEFORE ANY COMMAND trigger fires. I don't >>> think any trigger should fire on a read-only standby. >> >> Why ever not? > > Sorry, I meant any command trigger. It's because none of the commands > can be run on a standby, so the triggers don't seem appropriate. I'm not convinced. Right now, it's fairly useless - all the triggers could possibly do is throw an error, and an error is going to get thrown anyway, so it's only a question of which error message the user will see. But we discussed before the idea of adding a capability for BEFORE triggers to request that the actual execution of the command get skipped, and then it's possible to imagine this being useful. Someone could even use a command trigger that detects which machine it's running on, and if it's the standby, uses dblink to execute the command on the master, or something crazy like that. Command triggers could also be useful for logging all attempts to execute a particular command, which is probably still appropriate on the standby. I think that it will be a good thing to try to treat Hot Standby mode as much like regular operation as is reasonably possible, across the board. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 9 March 2012 14:09, Robert Haas wrote: > On Wed, Mar 7, 2012 at 4:53 PM, Thom Brown wrote: >> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on >> a read-only standby, the BEFORE ANY COMMAND trigger fires. I don't >> think any trigger should fire on a read-only standby. > > Why ever not? Sorry, I meant any command trigger. It's because none of the commands can be run on a standby, so the triggers don't seem appropriate. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Wed, Mar 7, 2012 at 4:53 PM, Thom Brown wrote: > I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on > a read-only standby, the BEFORE ANY COMMAND trigger fires. I don't > think any trigger should fire on a read-only standby. Why ever not? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 9 March 2012 00:28, Thom Brown wrote: > On 8 March 2012 22:24, Dimitri Fontaine wrote: > > We're getting there. :) It was late last night and I forgot to get around to testing pg_dump, which isn't working correctly: -- -- Name: cmd_trg_after_alter_aggregate; Type: COMMAND TRIGGER; Schema: -; Owner: -- CREATE COMMAND TRIGGER cmd_trg_after_alter_aggregate AFTER"ALTER AGGREGATE" EXECUTE PROCEDURE cmd_trg_info (); There shouldn't be quotes around the command, and when removing them, ensure there's a space before the command. All variations of the ALTER statements in the dump are fine, so are CREATE statements for ANY COMMAND command triggers. Also I notice that CREATE/ALTER/DROP COMMAND TRIGGER appears just before CREATE/ALTER/DROP TRIGGER in the documentation. This breaks the alphabetical order and I wasn't expecting to find it there when scanning down the page. Could we move them into an alphabetic position? -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 8 March 2012 22:24, Dimitri Fontaine wrote: We're getting there. :) > Hi, > > Thom Brown writes: >> The message returned by creating a command trigger after create index >> is still problematic: > > Fixed. I'm attaching an incremental patch here, the github branch is > updated too. Confirmed. >> CREATE VIEW doesn't return schema: > > Fixed, and as an added bonus I fixed the CREATE SEQUENCE oddity about > that too. Yes, working now. >> No specific triggers fire when altering a conversion: > > Couldn't reproduce, works here, added tests. My apologies. It seems I neglected to set up a specific trigger for it. It does indeed work. >> No specific triggers fire when altering the properties of a function: > > Fixed. Yes, tried every property available and working in every case now. >> No specific triggers fire when altering a sequence: > > Couldn't reproduce, added tests. Again, I wrongly assumed I had set up a command trigger for this. I think it's because I based my specific triggers on what was listed on the CREATE COMMAND TRIGGER documentation page, and those were only recently added. This is working fine. >> No specific triggers when altering a view: > > Same again. Working correctly. (see above) >> The object name shown in specific triggers when dropping aggregates >> shows the schema name: >> Same for collations: >> Dropping functions shows the object name as the schema name: >> Same with dropping operators: >> ...and operator family: >> … and the same for dropping text search >> configuration/dictionary/parser/template. > > Fixed in the attached (all of those where located at exactly the same > place, by the way, that's one fix). CREATE OPERATOR CLASS now shows objectname as 'hash' instead of its name where it's declared as "USING hash". This isn't a problem with ALTER/DROP OPERATOR CLASS. Everything else above works as expected now. >> When dropping domains, the name of the domain includes the schema name: > > I'm using format_type_be(objectId) so that int4 is integer and int8 > bigint etc, but that's adding the schemaname. I didn't have time to look > for another API that wouldn't add the schemaname nor to add one myself, > will do that soon. Okay, skipping test. >> When creating a trigger on REINDEX, I get the following message: > > Fixed. Could we change this to "REINDEX DATABASE triggers are not supported"? This way it would be consistent with the "AFTER CREATE INDEX CONCURRENTLY" warning. >> VACUUM doesn't fire a specific command trigger: > > I though it was better this way, I changed my mind and completed the code. Yes, working now. >> REINDEX on a table seems to show no schema name but an object name for >> specific triggers: > > Still on the TODO. Skipped. >> When REINDEXing an index rather than a table, the table's details are >> shown in the trigger. Is this expected?: > > Yeah well. Will see about how much damage needs to be done in the > current APIs, running out of steam for tonight's batch. Skipped. >> Documentation: > > Fixed. ALTER CAST is still listed and needs removing, not just from the documentation but every place it's used your code too. I can currently create a trigger for it, but it's impossible for it to fire since there's no such command. All these corrections I mentioned previously still needs to be made: “A command trigger's function must return void, the only it can aborts the execution of the command is by raising an exception.” should be: “A command trigger's function must return void. It can then only abort the execution of the command by raising an exception.” Remove: “For a constraint trigger, this is also the name to use when modifying the trigger's behavior using SET CONSTRAINTS.” “that's the case for VACUUM, CLUSTER CREATE INDEX CONCURRENTLY, and REINDEX DATABASE.” should be: “that's the case for VACUUM, CLUSTER, CREATE INDEX CONCURRENTLY, and REINDEX DATABASE.” > Thom Brown writes: >> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on >> a read-only standby, the BEFORE ANY COMMAND trigger fires. I don't >> think any trigger should fire on a read-only standby. > > Well I'm not sold on that myself (think pl/untrusted that would reach > out to the OS and do whatever is needed there). You can even set the > session_replication_role GUC to replica and only have the replica > command triggers fired. All other command triggers don't fire on read-only standbys, and the inconsistency doesn't seem right. On the one hand all CREATE/DROP/ALTER triggers aren't fired because of the "cannot execute in a read-only transaction" error message, but triggers do occur before utility commands, which would otherwise display the same message, and might not display it at all if the trigger causes an error in its function call. So it seems like they should either all fire, or none of them should. What are you thoughts? -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to
Re: [HACKERS] Command Triggers, patch v11
Hi,
Thom Brown writes:
> The message returned by creating a command trigger after create index
> is still problematic:
Fixed. I'm attaching an incremental patch here, the github branch is
updated too.
> CREATE VIEW doesn't return schema:
Fixed, and as an added bonus I fixed the CREATE SEQUENCE oddity about
that too.
> No specific triggers fire when altering a conversion:
Couldn't reproduce, works here, added tests.
> No specific triggers fire when altering the properties of a function:
Fixed.
> No specific triggers fire when altering a sequence:
Couldn't reproduce, added tests.
> No specific triggers when altering a view:
Same again.
> The object name shown in specific triggers when dropping aggregates
> shows the schema name:
> Same for collations:
> Dropping functions shows the object name as the schema name:
> Same with dropping operators:
> ...and operator family:
> … and the same for dropping text search
> configuration/dictionary/parser/template.
Fixed in the attached (all of those where located at exactly the same
place, by the way, that's one fix).
> When dropping domains, the name of the domain includes the schema name:
I'm using format_type_be(objectId) so that int4 is integer and int8
bigint etc, but that's adding the schemaname. I didn't have time to look
for another API that wouldn't add the schemaname nor to add one myself,
will do that soon.
> I hadn't previously tested triggers against CLUSTER, REINDEX, VACUUM
> and LOAD, but have tested them now.
Cool :)
> When creating a trigger on REINDEX, I get the following message:
Fixed.
> Creating AFTER CLUSTER command triggers produce an error (as expected
> since it's not supported), but AFTER REINDEX only produces a warning.
> These should be the same, probably both an error.
Fixed.
> VACUUM doesn't fire a specific command trigger:
I though it was better this way, I changed my mind and completed the code.
> REINDEX on a table seems to show no schema name but an object name for
> specific triggers:
Still on the TODO.
> When REINDEXing an index rather than a table, the table's details are
> shown in the trigger. Is this expected?:
Yeah well. Will see about how much damage needs to be done in the
current APIs, running out of steam for tonight's batch.
> REINDEXing the whole database doesn't fire specific command triggers:
We don't want to because REINDEX DATABASE is managing transactions on
its own, same limitation as with AFTER VACUUM and all. Will have a look
at what it takes to document that properly.
> Documentation:
Fixed.
Thom Brown writes:
> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
> a read-only standby, the BEFORE ANY COMMAND trigger fires. I don't
> think any trigger should fire on a read-only standby.
Well I'm not sold on that myself (think pl/untrusted that would reach
out to the OS and do whatever is needed there). You can even set the
session_replication_role GUC to replica and only have the replica
command triggers fired.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
*** a/doc/src/sgml/ref/create_command_trigger.sgml
--- b/doc/src/sgml/ref/create_command_trigger.sgml
***
*** 41,48 CREATE COMMAND TRIGGER name { BEFOR
ALTER LANGUAGE
ALTER OPERATOR
ALTER OPERATOR CLASS
- ALTER OPERATOR CLASS
- ALTER OPERATOR FAMILY
ALTER OPERATOR FAMILY
ALTER SCHEMA
ALTER SEQUENCE
--- 41,46
***
*** 137,146 CREATE COMMAND TRIGGER name { BEFOR
!Note that objects dropped by effect of DROP CASCADE
!will not provoque firing a command trigger, only the top-level command
!for the main object will fire a command trigger. That also applis to
!other dependencies following, as in DROP OWNED BY.
--- 135,145
!Note that objects dropped by the effect of DROP
!CASCADE will not result in a command trigger firing, only the
!top-level command for the main object will fire a command trigger. That
!also applies to other dependencies following, as in DROP OWNED
!BY.
***
*** 192,198 CREATE COMMAND TRIGGER name { BEFOR
you are connected to.
! Commands that exercize their own transaction control are only
supported in BEFORE command triggers, that's the
case for VACUUM, CLUSTER
CREATE INDEX CONCURRENTLY, and REINDEX
--- 191,197
you are connected to.
! Commands that exercise their own transaction control are only
supported in BEFORE command triggers, that's the
case for VACUUM, CLUSTER
CREATE INDEX CONCURRENTLY, and REINDEX
***
*** 215,220 CREATE COMMAND TRIGGER name { BEFOR
--- 214,224
not to be able to take over control from a superuser.
+ Triggers on ANY command support more commands than just this list
Re: [HACKERS] Command Triggers, patch v11
On 6 March 2012 23:25, Thom Brown wrote: > On 6 March 2012 21:18, Thom Brown wrote: >> On 6 March 2012 21:04, Dimitri Fontaine wrote: > [CASCADE will not run the command triggers for cascaded objects] If these are all expected, does it in any way compromise the effectiveness of DDL triggers in major use-cases? >>> >>> I don't think so. When replicating the replica will certainly drop the >>> same set of dependent objects, for example. Auditing is another story. >>> Do we want to try having cascaded object support in drop commands? >> >> I wasn't sure if auditing was one of the rationale behind the feature >> or not. If it is, it presents a major problem. How does the replica >> know that the objects were dropped? >> >> Thanks for the updated patch and the quick turnaround time. I'll give >> it another review. > > Okay, here's the update: > > The message returned by creating a command trigger after create index > is still problematic: > > thom@test=# CREATE COMMAND TRIGGER cmd_trg_after_create_index AFTER > CREATE INDEX EXECUTE PROCEDURE cmd_trg_info(); > WARNING: AFTER CREATE INDEX CONCURRENTLY triggers are not supported > DETAIL: The command trigger will not get fired. > CREATE COMMAND TRIGGER > > The detail suggests that even though the command trigger has been > requested, it won't be fired. Might I suggest: > > WARNING: AFTER CREATE INDEX CONCURRENTLY triggers are not supported > DETAIL: The command trigger will not fire on concurrently-created indexes. > > > > CREATE VIEW doesn't return schema: > > thom@test=# CREATE VIEW view_test AS SELECT id, stuff FROM public.test; > NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE > VIEW' objectid= schemaname='' objectname='' > NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='CREATE VIEW' > objectid= schemaname='' objectname='view_test' > NOTICE: Command trigger: tg_when='AFTER' cmd_tag='CREATE VIEW' > objectid=16606 schemaname='' objectname='view_test' > NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='CREATE VIEW' > objectid= schemaname='' objectname='' > CREATE VIEW > > No specific triggers fire when altering a conversion: > > thom@test=# ALTER CONVERSION test9 RENAME TO test8; > NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER > CONVERSION' objectid= schemaname='' objectname='' > NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER > CONVERSION' objectid= schemaname='' objectname='' > ALTER CONVERSION > > Note: I hadn’t previously tested this, but I don’t think it was listed > as supported until now. > > > No specific triggers fire when altering the properties of a function: > > thom@test=# ALTER FUNCTION test.testfunc2() COST 77; > NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER > FUNCTION' objectid= schemaname='' objectname='' > NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER > FUNCTION' objectid= schemaname='' objectname='' > ALTER FUNCTION > > > No specific triggers fire when altering a sequence: > > thom@test=# ALTER SEQUENCE test_seq2 OWNER TO test; > NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER > SEQUENCE' objectid= schemaname='' objectname='' > NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER > SEQUENCE' objectid= schemaname='' objectname='' > ALTER SEQUENCE > > thom@test=# ALTER SEQUENCE test_seq2 RESTART WITH 4; > NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER > SEQUENCE' objectid= schemaname='' objectname='' > NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER > SEQUENCE' objectid= schemaname='' objectname='' > ALTER SEQUENCE > > > No specific triggers when altering a view: > > thom@test=# ALTER VIEW view_test2 SET SCHEMA testnew; > NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW' > objectid= schemaname='' objectname='' > NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW' > objectid= schemaname='' objectname='' > ALTER VIEW > > thom@test=# ALTER VIEW testnew.view_test2 ALTER COLUMN id SET DEFAULT 9; > NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW' > objectid= schemaname='' objectname='' > NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW' > objectid= schemaname='' objectname='' > ALTER VIEW > > > The object name shown in specific triggers when dropping aggregates > shows the schema name: > > thom@test=# DROP AGGREGATE testnew.avgtest2(bigint); > NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP > AGGREGATE' objectid= schemaname='' objectname='' > NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP AGGREGATE' > objectid=16539 schemaname='testnew' objectname='testnew' > NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP AGGREGATE' > objectid= schemaname='testnew' objectname='testnew' > NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP > AGGREGATE' objectid= schemaname='' objectname='' > DROP AGGREGATE > > Same for collations: > > thom@test=# DROP COLLA
Re: [HACKERS] Command Triggers, patch v11
On 6 March 2012 21:18, Thom Brown wrote: > On 6 March 2012 21:04, Dimitri Fontaine wrote: [CASCADE will not run the command triggers for cascaded objects] >>> If these are all expected, does it in any way compromise the >>> effectiveness of DDL triggers in major use-cases? >> >> I don't think so. When replicating the replica will certainly drop the >> same set of dependent objects, for example. Auditing is another story. >> Do we want to try having cascaded object support in drop commands? > > I wasn't sure if auditing was one of the rationale behind the feature > or not. If it is, it presents a major problem. How does the replica > know that the objects were dropped? > > Thanks for the updated patch and the quick turnaround time. I'll give > it another review. Okay, here's the update: The message returned by creating a command trigger after create index is still problematic: thom@test=# CREATE COMMAND TRIGGER cmd_trg_after_create_index AFTER CREATE INDEX EXECUTE PROCEDURE cmd_trg_info(); WARNING: AFTER CREATE INDEX CONCURRENTLY triggers are not supported DETAIL: The command trigger will not get fired. CREATE COMMAND TRIGGER The detail suggests that even though the command trigger has been requested, it won't be fired. Might I suggest: WARNING: AFTER CREATE INDEX CONCURRENTLY triggers are not supported DETAIL: The command trigger will not fire on concurrently-created indexes. CREATE VIEW doesn't return schema: thom@test=# CREATE VIEW view_test AS SELECT id, stuff FROM public.test; NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE VIEW' objectid= schemaname='' objectname='' NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='CREATE VIEW' objectid= schemaname='' objectname='view_test' NOTICE: Command trigger: tg_when='AFTER' cmd_tag='CREATE VIEW' objectid=16606 schemaname='' objectname='view_test' NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='CREATE VIEW' objectid= schemaname='' objectname='' CREATE VIEW No specific triggers fire when altering a conversion: thom@test=# ALTER CONVERSION test9 RENAME TO test8; NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER CONVERSION' objectid= schemaname='' objectname='' NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER CONVERSION' objectid= schemaname='' objectname='' ALTER CONVERSION Note: I hadn’t previously tested this, but I don’t think it was listed as supported until now. No specific triggers fire when altering the properties of a function: thom@test=# ALTER FUNCTION test.testfunc2() COST 77; NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER FUNCTION' objectid= schemaname='' objectname='' NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER FUNCTION' objectid= schemaname='' objectname='' ALTER FUNCTION No specific triggers fire when altering a sequence: thom@test=# ALTER SEQUENCE test_seq2 OWNER TO test; NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER SEQUENCE' objectid= schemaname='' objectname='' NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER SEQUENCE' objectid= schemaname='' objectname='' ALTER SEQUENCE thom@test=# ALTER SEQUENCE test_seq2 RESTART WITH 4; NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER SEQUENCE' objectid= schemaname='' objectname='' NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER SEQUENCE' objectid= schemaname='' objectname='' ALTER SEQUENCE No specific triggers when altering a view: thom@test=# ALTER VIEW view_test2 SET SCHEMA testnew; NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW' objectid= schemaname='' objectname='' NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW' objectid= schemaname='' objectname='' ALTER VIEW thom@test=# ALTER VIEW testnew.view_test2 ALTER COLUMN id SET DEFAULT 9; NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW' objectid= schemaname='' objectname='' NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW' objectid= schemaname='' objectname='' ALTER VIEW The object name shown in specific triggers when dropping aggregates shows the schema name: thom@test=# DROP AGGREGATE testnew.avgtest2(bigint); NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP AGGREGATE' objectid= schemaname='' objectname='' NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP AGGREGATE' objectid=16539 schemaname='testnew' objectname='testnew' NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP AGGREGATE' objectid= schemaname='testnew' objectname='testnew' NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP AGGREGATE' objectid= schemaname='' objectname='' DROP AGGREGATE Same for collations: thom@test=# DROP COLLATION testnew.en_gb_test; NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP COLLATION' objectid= schemaname='' objectname='' NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP COLLATION' objectid=16542 schemaname='testnew' obj
Re: [HACKERS] Command Triggers, patch v11
On 6 March 2012 21:04, Dimitri Fontaine wrote: >>> [CASCADE will not run the command triggers for cascaded objects] >> If these are all expected, does it in any way compromise the >> effectiveness of DDL triggers in major use-cases? > > I don't think so. When replicating the replica will certainly drop the > same set of dependent objects, for example. Auditing is another story. > Do we want to try having cascaded object support in drop commands? I wasn't sure if auditing was one of the rationale behind the feature or not. If it is, it presents a major problem. How does the replica know that the objects were dropped? Thanks for the updated patch and the quick turnaround time. I'll give it another review. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 5 March 2012 20:42, Dimitri Fontaine wrote: > Hi, > > Thanks for the extensive testing. I'm adding your tests to the > regression suite, and keep wondering if you saw that lots of them were > already covered? Did you try make installcheck? Yes, but I felt it better that I come up with my own separate tests. > Thom Brown writes: >> Creating a command trigger using ANY COMMAND results in oid, >> schemaname, objectname (function parameters 4 & 5) not being set for >> either BEFORE or AFTER. > > Yes, that's documented. It could be better documented though, it seems. Is there a reason why we can't provide the OID for ANY COMMAND... if it's available? I'm guessing it would end up involving having to special case for every command. :/ >> Command triggers for creating sequences don’t show the schema: > > Documented already, it's uneasy to get at it in the code and I figured I > might as well drop the ball on that in the current patch's form. Fair enough. >> Command triggers for BEFORE CREATE TYPE (exluding ANY COMMAND) don’t >> fire if the type isn’t created due to an error: > > Per design, although we might want to talk about it. I made it so that > specific command triggers are only fired after errors checks have been > made. > > That's not the case with ANY command triggers so that you can actually > block DDLs on your instances, as has been asked on list. I don't have any strong feelings about it, so I'll bear it in mind for future tests. >> The ANY COMMAND trigger fires on creating roles, but there’s no >> corresponding allowance to create the trigger explicitly for creating >> roles. > > Roles are global objects, you don't want the behavior of role commands > to depend on which database you happen to have been logged in when > issuing the command. That would call for removing the ANY command > support for them too, but I can't seem to decide about that. > > Any input? If that's your reasoning, then it would make sense to remove ANY command support for it too. >> There doesn’t appear to be command trigger support for ALTER LARGE OBJECT. > > Do we want to have some? Those are in between data and command. *shrug* But ANY COMMAND triggers fire for it. So I'd say either remove support for that, or add a specific trigger. >> Specific command triggers on DROP AGGREGATE don’t fire in the IF >> EXISTS scenario if the target object doesn’t exist: > > So, do we want to run the command triggers here? Is the IF EXISTS check > to be considered like the other error conditions? Maybe. If that's expected behaviour, I'll start expecting it then. >> When adding objects to an extension, then dropping the extension with >> a cascade, the objects are dropped with it, but triggers aren’t fired >> to the removal of those dependant objects: > > Yes, that's expected and needs documenting. > >> Using DROP OWNED BY allows objects to be dropped without their >> respective specific triggers firing. > > Expected too. > >> Using DROP SCHEMA … CASACDE also allows objects to be dropped without >> their respective specific triggers firing: > > Again, same expectation here. If these are all expected, does it in any way compromise the effectiveness of DDL triggers in major use-cases? > I'm not sending a revised patch, please use the github branch if you > want to do some more tests already, or ask me for either a new patch > version or a patch-on-patch, as you see fit. Hmm... how does that work with regards to the commitfest process? But I'll re-test when you let me know when you've committed your remaining fixes to Github. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Sat, Mar 3, 2012 at 2:25 PM, Dimitri Fontaine wrote: > "Kevin Grittner" writes: >> Right. What I thought I was agreeing with was the notion that you >> should need to specify more than the trigger name to drop the >> trigger. Rather like how you can create a trigger AFTER INSERT OR >> UPDATE OR DELETE, but you don't need to specify all those events to >> drop the trigger -- just the name will do. > > The parallel between INSERT/UPDATE/DELETE and the trigger's command is > not working well enough, because in the data trigger case we're managing > a single catalog entry with a single command, and in the command trigger > case, in my model at least, we would be managing several catalog entries > per command. > > To take an example: > > CREATE COMMAND TRIGGER foo AFTER create table, create view; > DROP COMMAND TRIGGER foo; > > The first command would create two catalog entries, and the second one > would delete the same two entries. It used to work this way in the > patch, then when merging with the new remove object infrastructure I > lost that ability. From the beginning Robert has been saying he didn't > want that behavior, and Tom is now saying the same, IIUC. > > So we're back to one command, one catalog entry. I hadn't made the connection here until you read this, but I agree there's a problem there. One command, one catalog entry is, I think, pretty important. So that means that if want to support a trigger on CREATE TABLE OR CREATE VIEW OR DROP EXTENSION, then the command names (or integers that serve as proxies for them) need to go into an array somewhere, and we had to look for arrays that contain the command we're looking for, rather than just the command name. That might seem prohibitively slow, but I bet if you put a proper cache in place it isn't, because pg_cmdtrigger should be pretty small and not updated very often. You can probably afford to seq-scan it and rebuild your entire cache across all command types every time it changes in any way. But just supporting one command type per trigger seems fine for a first version, too. There's nothing to prevent us from adding that later. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Monday, March 05, 2012 09:42:00 PM Dimitri Fontaine wrote: > > Still no command triggers firing for CREATE TABLE AS: > Yes, Andres made CTAS a utility command, he didn't add the code that > make them fire command triggers. I would expect his patch to get in > first, so I don't expect him to be adding that support, I think I will > have to add it when rebasing once his patch has landed. That was my assumption as well. Any opinions about adding the patch to the commitfest other than from dim? I feel a bit bad adding it to the in-progress one even if its belongs there because is a part of the command trigger stuff... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Hi, Thanks for the extensive testing. I'm adding your tests to the regression suite, and keep wondering if you saw that lots of them were already covered? Did you try make installcheck? Thom Brown writes: > Creating a command trigger using ANY COMMAND results in oid, > schemaname, objectname (function parameters 4 & 5) not being set for > either BEFORE or AFTER. Yes, that's documented. It could be better documented though, it seems. > There is no support for ALTER CONVERSION. It was missing in the grammar and docs only, added. > WARNING: CREATE INDEX CONCURRENTLY is not supported > DETAIL: The command trigger will not get fired. > > This should probably say that it’s not supported on AFTER command > triggers yet rather than the general DDL itself. Edited. > Command triggers for AFTER creating rules don’t return OIDs. Fixed. > Command triggers for creating sequences don’t show the schema: Documented already, it's uneasy to get at it in the code and I figured I might as well drop the ball on that in the current patch's form. > Command triggers for AFTER creating extensions with IF NOT EXISTS > don’t fire, but do in the ANY COMMAND instance: Fixed. > Command triggers on CREATE TEXT SEARCH DICTIONARY show the name as garbage: Fixed, test case added. > Command triggers for BEFORE CREATE TYPE (exluding ANY COMMAND) don’t > fire if the type isn’t created due to an error: Per design, although we might want to talk about it. I made it so that specific command triggers are only fired after errors checks have been made. That's not the case with ANY command triggers so that you can actually block DDLs on your instances, as has been asked on list. > The ANY COMMAND trigger fires on creating roles, but there’s no > corresponding allowance to create the trigger explicitly for creating > roles. Roles are global objects, you don't want the behavior of role commands to depend on which database you happen to have been logged in when issuing the command. That would call for removing the ANY command support for them too, but I can't seem to decide about that. Any input? > Command triggers for AFTER CREATE VIEW don’t show the schema: Couldn't reproduce, added test cases. > Command triggers for BEFORE and AFTER ALTER DOMAIN show a garbage name > and no schema when dropping a constraint: Fixed, added test cases. > Continuing with this same trigger, we do get a schema but a garbage > name for OWNER TO: Fixed, added test cases. > When an ALTER EXTENSION fails to upgrade, the AFTER ANY COMMAND > trigger fires, but not command triggers specifically for ALTER > EXTENSION: > > Same on ALTER EXTENSION, when failing to add a member, the BEFORE ANY > COMMAND trigger fires, but not the one specifically for ALTER > EXTENSION: Again, per design. Let's talk about it, it will probably need at least documentation. > Specific command triggers against ALTER FOREIGN TABLE (i.e. not ANY > COMMAND) for BEFORE and AFTER aren’t working when renaming columns: > > Specific command triggers agains ALTER FUNCTION (i.e. not ANY COMMAND) > don’t fire for any changes except renaming, changing owner or changing > schema. Everything else fails to trigger (cost, rows, setting > configuration parameters, setting strict, security invoker etc.).: I kept some TODO items as I feared I would get bored tomorrow otherwise… > There doesn’t appear to be command trigger support for ALTER LARGE OBJECT. Do we want to have some? Those are in between data and command. > Specific command triggers on ALTER SEQUENCE don’t fire: > Specific command triggers on ALTER TABLE don’t fire for renaming columns: > Also renaming attributes doesn’t fire specific triggers: > Specific command triggers on ALTER VIEW don’t fire for any type of change: Kept on the TODO. > Command triggers on ALTER TYPE when changing owner produce a garbage name: Fixed along with the DOMAIN test case (same code). > Specific command triggers on DROP AGGREGATE don’t fire in the IF > EXISTS scenario if the target object doesn’t exist: So, do we want to run the command triggers here? Is the IF EXISTS check to be considered like the other error conditions? > When adding objects to an extension, then dropping the extension with > a cascade, the objects are dropped with it, but triggers aren’t fired > to the removal of those dependant objects: Yes, that's expected and needs documenting. > Using DROP OWNED BY allows objects to be dropped without their > respective specific triggers firing. Expected too. > Using DROP SCHEMA … CASACDE also allows objects to be dropped without > their respective specific triggers firing: Again, same expectation here. > Command triggers on all DROP commands for TEXT SEARCH > CONFIGURATION/DICTIONARY/PARSER/TEMPLATE show the schema name as the > relation name: Now that's strange and will keep me awake longer tomorrow. > Still no command triggers firing for CREATE TABLE AS: Yes, Andres made CTAS a utility command, he didn't add the co
Re: [HACKERS] Command Triggers, patch v11
Tom Lane writes: > FWIW, I agree with Thom on this. If we do it as you suggest, I > confidently predict that it will be less than a year before we seriously > regret it. Given all the discussion around this, it's borderline insane > to believe that the set of parameters to be passed to command triggers > is nailed down and won't need to change in the future. > > As for special coding of support, it hardly seems onerous when every > language that has triggers at all has got some provision for the > existing trigger parameters. A bit of copying and pasting should get > the job done. So, for that to happen I need to add a new macro and use it in most call sites of CALLED_AS_TRIGGER(fcinfo). That includes the setup switch in src/pl/plpgsql/src/pl_comp.c doCompile() and plpgsql_call_handler() for starters. Let's call the macro CALLED_AS_COMMAND_TRIGGER(fcinfo), and I would extend CommandContextData to be a Node of type CommandTriggerData, that needs to be added to the NodeTag enum as T_CommandTriggerData. With that in place I can stuff the data into the function's call context (via InitFunctionCallInfoData) then edit the call handlers of included procedure languages until they are able to init their language variables with the info. Then, do we want the command trigger functions to return type trigger or another specific type? I guess we want to forbid registering any function as a command trigger? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 3 March 2012 19:25, Dimitri Fontaine wrote: > "Kevin Grittner" writes: >> Right. What I thought I was agreeing with was the notion that you >> should need to specify more than the trigger name to drop the >> trigger. Rather like how you can create a trigger AFTER INSERT OR >> UPDATE OR DELETE, but you don't need to specify all those events to >> drop the trigger -- just the name will do. > > The parallel between INSERT/UPDATE/DELETE and the trigger's command is > not working well enough, because in the data trigger case we're managing > a single catalog entry with a single command, and in the command trigger > case, in my model at least, we would be managing several catalog entries > per command. > > To take an example: > > CREATE COMMAND TRIGGER foo AFTER create table, create view; > DROP COMMAND TRIGGER foo; > > The first command would create two catalog entries, and the second one > would delete the same two entries. It used to work this way in the > patch, then when merging with the new remove object infrastructure I > lost that ability. From the beginning Robert has been saying he didn't > want that behavior, and Tom is now saying the same, IIUC. > > So we're back to one command, one catalog entry. That sucks. I'm surprised there's no provision for overriding it on a command-by-command basis. I would suggest an array instead, but that sounds costly from a look-up perspective. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
"Kevin Grittner" writes: > Right. What I thought I was agreeing with was the notion that you > should need to specify more than the trigger name to drop the > trigger. Rather like how you can create a trigger AFTER INSERT OR > UPDATE OR DELETE, but you don't need to specify all those events to > drop the trigger -- just the name will do. The parallel between INSERT/UPDATE/DELETE and the trigger's command is not working well enough, because in the data trigger case we're managing a single catalog entry with a single command, and in the command trigger case, in my model at least, we would be managing several catalog entries per command. To take an example: CREATE COMMAND TRIGGER foo AFTER create table, create view; DROP COMMAND TRIGGER foo; The first command would create two catalog entries, and the second one would delete the same two entries. It used to work this way in the patch, then when merging with the new remove object infrastructure I lost that ability. From the beginning Robert has been saying he didn't want that behavior, and Tom is now saying the same, IIUC. So we're back to one command, one catalog entry. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown wrote: > Don't you mean "shouldn't need to specify more than the trigger > name"? You are right, that's what I meant to say. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 3 March 2012 16:12, Kevin Grittner wrote:
>> Thom Brown wrote:
>> Dimitri Fontaine wrote:
>>> Thom Brown writes:
problem. It was the DROP COMMAND TRIGGER statement that garnered
comment, as it makes more sense to drop the entire trigger than
individual commands for that trigger.
>>>
>>> What you're saying here is that a single command could have more
>>> than one command attached to it, and what I understand Tom, Robert
>>> and Kevin are saying is that any given command trigger should only
>>> be attached to a single command.
>>
>> I hadn't interpreted it that way
>
> Nor had I.
>
>> I'm still of the opinion that a single command trigger should be
> able to attach to multiple commands, just not
>> amendable once the trigger has been created.
>
> That was my understanding of what we were discussing, too.
>
>> So my vote was for:
>>
>> CREATE COMMAND TRIGGER name { BEFORE | AFTER } command [, ... ]
>> EXECUTE PROCEDURE function_name ()
>>
>> ALTER COMMAND TRIGGER name SET enabled
>>
>> DROP COMMAND TRIGGER [ IF EXISTS ] name [ CASCADE | RESTRICT ]
>
> Same here.
>
>> My argument for this is that you may wish to implement the same
>> trigger for tables, views and foreign tables rather than create 3
>> separate triggers.
>
> Right. What I thought I was agreeing with was the notion that you
> should need to specify more than the trigger name to drop the
> trigger. Rather like how you can create a trigger AFTER INSERT OR
> UPDATE OR DELETE, but you don't need to specify all those events to
> drop the trigger -- just the name will do.
Don't you mean "shouldn't need to specify more than the trigger name"?
--
Thom
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
> Thom Brown wrote:
> Dimitri Fontaine wrote:
>> Thom Brown writes:
>>> problem. It was the DROP COMMAND TRIGGER statement that garnered
>>> comment, as it makes more sense to drop the entire trigger than
>>> individual commands for that trigger.
>>
>> What you're saying here is that a single command could have more
>> than one command attached to it, and what I understand Tom, Robert
>> and Kevin are saying is that any given command trigger should only
>> be attached to a single command.
>
> I hadn't interpreted it that way
Nor had I.
> I'm still of the opinion that a single command trigger should be
able to attach to multiple commands, just not
> amendable once the trigger has been created.
That was my understanding of what we were discussing, too.
> So my vote was for:
>
> CREATE COMMAND TRIGGER name { BEFORE | AFTER } command [, ... ]
> EXECUTE PROCEDURE function_name ()
>
> ALTER COMMAND TRIGGER name SET enabled
>
> DROP COMMAND TRIGGER [ IF EXISTS ] name [ CASCADE | RESTRICT ]
Same here.
> My argument for this is that you may wish to implement the same
> trigger for tables, views and foreign tables rather than create 3
> separate triggers.
Right. What I thought I was agreeing with was the notion that you
should need to specify more than the trigger name to drop the
trigger. Rather like how you can create a trigger AFTER INSERT OR
UPDATE OR DELETE, but you don't need to specify all those events to
drop the trigger -- just the name will do.
-Kevin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 3 March 2012 14:34, Dimitri Fontaine wrote:
> Thom Brown writes:
>> problem. It was the DROP COMMAND TRIGGER statement that garnered
>> comment, as it makes more sense to drop the entire trigger than
>> individual commands for that trigger.
>
> What you're saying here is that a single command could have more than
> one command attached to it, and what I understand Tom, Robert and Kevin
> are saying is that any given command trigger should only be attached to
> a single command.
I hadn't interpreted it that way, but then that could just be my
misinterpretation. I'm still of the opinion that a single command
trigger should be able to attach to multiple commands, just not
amendable once the trigger has been created. If that's not how others
saw it, I'm outnumbered.
So my vote was for:
CREATE COMMAND TRIGGER name { BEFORE | AFTER } command [, ... ]
EXECUTE PROCEDURE function_name ()
ALTER COMMAND TRIGGER name SET enabled
DROP COMMAND TRIGGER [ IF EXISTS ] name [ CASCADE | RESTRICT ]
The only difference shown above compared to the current implementation
in your patch is [, ... ] after command in the CREATE COMMAND TRIGGER
statement.
My argument for this is that you may wish to implement the same
trigger for tables, views and foreign tables rather than create 3
separate triggers.
> If we wanted to be more consistent we would need to have a way to attach
> the same trigger to both BEFORE and AFTER the command, as of now you
> need two triggers here.
I'm not sure I understand. Attaching a trigger to both BEFORE and
AFTER isn't possible for regular triggers so I don't see why that
would introduce consistency. Could you explain?
--
Thom
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 3 March 2012 14:26, Dimitri Fontaine wrote: > Thom Brown writes: >> And having tried building it, it appears to fail. > > Sorry about that, my compiler here was happy building the source (and I > had been doing make clean install along the way) and make installcheck > passed, here. > > Now fixed on my github's branch, including docs. > > I'll send an updated patch revision later, hopefully including pg_dump > support fixtures (well, adaptation to the new way of doing things IIUC) > and maybe with some trigger arguments rework done. > > I understand that you're not blocked until that new version is out, > right? You could use either the incremental patch attached for > convenience. Thanks for the extra patch. Do you see any functional changes between now and your next patch? If so, it doesn't make sense for me to test anything yet. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown writes: > problem. It was the DROP COMMAND TRIGGER statement that garnered > comment, as it makes more sense to drop the entire trigger than > individual commands for that trigger. What you're saying here is that a single command could have more than one command attached to it, and what I understand Tom, Robert and Kevin are saying is that any given command trigger should only be attached to a single command. If we wanted to be more consistent we would need to have a way to attach the same trigger to both BEFORE and AFTER the command, as of now you need two triggers here. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown writes:
> And having tried building it, it appears to fail.
Sorry about that, my compiler here was happy building the source (and I
had been doing make clean install along the way) and make installcheck
passed, here.
Now fixed on my github's branch, including docs.
I'll send an updated patch revision later, hopefully including pg_dump
support fixtures (well, adaptation to the new way of doing things IIUC)
and maybe with some trigger arguments rework done.
I understand that you're not blocked until that new version is out,
right? You could use either the incremental patch attached for
convenience.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
commit 301d8e58b6bfe89f35d82ad7a5216891f56c5d48
Author: Dimitri Fontaine
Date: Sat Mar 3 15:18:59 2012 +0100
Fix RenameCmdTrigger(), per review.
diff --git a/doc/src/sgml/ref/alter_command_trigger.sgml b/doc/src/sgml/ref/alter_command_trigger.sgml
index dd903e7..48b536c 100644
--- a/doc/src/sgml/ref/alter_command_trigger.sgml
+++ b/doc/src/sgml/ref/alter_command_trigger.sgml
@@ -22,6 +22,7 @@ PostgreSQL documentation
ALTER COMMAND TRIGGER name SET enabled
+ALTER COMMAND TRIGGER name RENAME TO newname
where enabled can be one of:
@@ -62,10 +63,10 @@ ALTER COMMAND TRIGGER name SET
-command
+newname
- The command tag on which this trigger acts.
+ The new name of the command trigger.
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 4d07642..b447306 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -66,7 +66,7 @@ ExecRenameStmt(RenameStmt *stmt)
break;
case OBJECT_CMDTRIGGER:
- RenameCmdTrigger(stmt->object, stmt->subname, stmt->newname);
+ RenameCmdTrigger(stmt->object, stmt->newname);
break;
case OBJECT_DATABASE:
diff --git a/src/backend/commands/cmdtrigger.c b/src/backend/commands/cmdtrigger.c
index e8d294d..ef9794f 100644
--- a/src/backend/commands/cmdtrigger.c
+++ b/src/backend/commands/cmdtrigger.c
@@ -294,13 +294,17 @@ AlterCmdTrigger(AlterCmdTrigStmt *stmt)
* Rename command trigger
*/
void
-RenameCmdTrigger(const char *trigname, const char *newname)
+RenameCmdTrigger(List *name, const char *newname)
{
SysScanDesc tgscan;
ScanKeyData skey[1];
HeapTuple tup;
Relation rel;
Form_pg_cmdtrigger cmdForm;
+ char *trigname;
+
+ Assert(list_length(name) == 1);
+ trigname = strVal((Value *)linitial(name));
CheckCmdTriggerPrivileges();
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index defcdd1..6a20a6c 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3463,7 +3463,6 @@ _copyCreateCmdTrigStmt(const CreateCmdTrigStmt *from)
{
CreateCmdTrigStmt *newnode = makeNode(CreateCmdTrigStmt);
- COPY_NODE_FIELD(command);
COPY_STRING_FIELD(trigname);
COPY_SCALAR_FIELD(timing);
COPY_NODE_FIELD(funcname);
@@ -3476,7 +3475,6 @@ _copyAlterCmdTrigStmt(const AlterCmdTrigStmt *from)
{
AlterCmdTrigStmt *newnode = makeNode(AlterCmdTrigStmt);
- COPY_STRING_FIELD(command);
COPY_STRING_FIELD(trigname);
COPY_STRING_FIELD(tgenabled);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 1ad31f0..137075a 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1780,7 +1780,6 @@ _equalCreateTrigStmt(const CreateTrigStmt *a, const CreateTrigStmt *b)
static bool
_equalCreateCmdTrigStmt(const CreateCmdTrigStmt *a, const CreateCmdTrigStmt *b)
{
- COMPARE_NODE_FIELD(command);
COMPARE_STRING_FIELD(trigname);
COMPARE_SCALAR_FIELD(timing);
COMPARE_NODE_FIELD(funcname);
@@ -1791,7 +1790,6 @@ _equalCreateCmdTrigStmt(const CreateCmdTrigStmt *a, const CreateCmdTrigStmt *b)
static bool
_equalAlterCmdTrigStmt(const AlterCmdTrigStmt *a, const AlterCmdTrigStmt *b)
{
- COMPARE_STRING_FIELD(command);
COMPARE_STRING_FIELD(trigname);
COMPARE_STRING_FIELD(tgenabled);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e5a0d34..e9872d3 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -6931,13 +6931,12 @@ RenameStmt: ALTER AGGREGATE func_name aggr_args RENAME TO name
n->missing_ok = false;
$$ = (Node *)n;
}
- | ALTER TRIGGER name ON COMMAND trigger_command RENAME TO name
+ | ALTER COMMAND TRIGGER name RENAME TO name
{
RenameStmt *n = makeNode(RenameStmt);
n->renameType = OBJECT_CMDTRIGGER;
- n->object = list_make1(makeString($6));
- n->subname = $3;
- n->newname = $9;
+ n->object = list_make1(makeString($4));
+ n->newname = $7;
$$ = (Node *)n;
}
| ALTER ROLE RoleId RENAME TO RoleId
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 3c75cf3..69d6479 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -168,7 +168,6 @@ call_before_cmdtriggers(Node *parsetree, Co
Re: [HACKERS] Command Triggers, patch v11
On 3 March 2012 13:45, Dimitri Fontaine wrote: > Robert Haas writes: CREATE COMMAND TRIGGER name ... properties ...; DROP COMMAND TRIGGER name; full stop. If you want to run the same trigger function on some more commands, add another trigger name. >>> >>> +1 >> >> +1. I suggested the same thing a while back. > > Yeah, I know, I just wanted to hear from more people before ditching out > a part of the work I did, and Thom was balancing in the opposite > direction. I was? I agreed with Tom's comment, but I did query your interpretation of it with regards to the CREATE COMMAND TRIGGER statement. It seems you removed the ability to create a command trigger against multiple commands, but I don't think that was the problem. It was the DROP COMMAND TRIGGER statement that garnered comment, as it makes more sense to drop the entire trigger than individual commands for that trigger. Initially I had proposed a way to drop all commands on a trigger at once as an additional option, but just dropping it completely or not at all is preferable. But if there is agreement to have multiple commands on a command trigger, I'm wondering whether we should have an OR separator rather than a comma? The reason is that regular triggers define a list of statements this way. Personally I prefer the comma syntax, but my concern (not a strong concern) is for lack of consistency. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Robert Haas writes: >>> CREATE COMMAND TRIGGER name ... properties ...; >>> DROP COMMAND TRIGGER name; >>> >>> full stop. If you want to run the same trigger function on some >>> more commands, add another trigger name. >> >> +1 > > +1. I suggested the same thing a while back. Yeah, I know, I just wanted to hear from more people before ditching out a part of the work I did, and Thom was balancing in the opposite direction. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 3 March 2012 00:08, Thom Brown wrote: > On 2 March 2012 23:33, Thom Brown wrote: >> On 2 March 2012 22:32, Dimitri Fontaine wrote: > test=# CREATE TABLE badname (id int, a int, b text); > ERROR: invalid relation name: badname > test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; > SELECT 1 > > This doesn't even get picked up by ANY COMMAND. >>> >>> I think Andres should add an entry for his patch on the commitfest. Is >>> it ok? >> >> I'll try Andres' patch this weekend while I test yours. > > Actually no matter which patch I apply first, they cause the other to > fail to apply. And having tried building it, it appears to fail. gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o alter.o alter.c -MMD -MP -MF .deps/alter.Po alter.c: In function ‘ExecRenameStmt’: alter.c:69:4: warning: passing argument 1 of ‘RenameCmdTrigger’ from incompatible pointer type [enabled by default] ../../../src/include/commands/cmdtrigger.h:45:13: note: expected ‘const char *’ but argument is of type ‘struct List *’ alter.c:69:4: error: too many arguments to function ‘RenameCmdTrigger’ ../../../src/include/commands/cmdtrigger.h:45:13: note: declared here You've changed the signature of RenameCmdTrigger but still pass in the old arguments in alter.c. If I fix this locally, I then get: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o copyfuncs.o copyfuncs.c -MMD -MP -MF .deps/copyfuncs.Po copyfuncs.c: In function ‘_copyAlterCmdTrigStmt’: copyfuncs.c:3479:2: error: ‘AlterCmdTrigStmt’ has no member named ‘command’ copyfuncs.c:3479:2: error: ‘AlterCmdTrigStmt’ has no member named ‘command’ copyfuncs.c:3479:2: error: ‘AlterCmdTrigStmt’ has no member named ‘command’ There's an erroneous "COPY_STRING_FIELD(command)" in there, as well as in equalfuncs.c. After removing both those instances it builds. Are you sure you don't have any uncommitted changes? -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
"anara...@anarazel.de" schrieb: > > >Thom Brown schrieb: > >>On 2 March 2012 23:33, Thom Brown wrote: >>> On 2 March 2012 22:32, Dimitri Fontaine >>wrote: >> test=# CREATE TABLE badname (id int, a int, b text); >> ERROR: invalid relation name: badname >> test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, >>''::text b; >> SELECT 1 >> >> This doesn't even get picked up by ANY COMMAND. I think Andres should add an entry for his patch on the commitfest. >> Is it ok? >>> >>> I'll try Andres' patch this weekend while I test yours. >> >>Actually no matter which patch I apply first, they cause the other to >>fail to apply. >I will try to fix that on the plain tomorrow (to NYC) but I am not yet >sure when/where I will have internet access again. One more try: To the list. Andres Please excuse the brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Tue, Feb 28, 2012 at 10:09 AM, Kevin Grittner wrote: > Tom Lane wrote: >> This seems over-complicated. Triggers on tables do not have >> alterable properties, why should command triggers? I vote for >> >> CREATE COMMAND TRIGGER name ... properties ...; >> >> DROP COMMAND TRIGGER name; >> >> full stop. If you want to run the same trigger function on some >> more commands, add another trigger name. > > +1 +1. I suggested the same thing a while back. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 2 March 2012 23:33, Thom Brown wrote: > On 2 March 2012 22:32, Dimitri Fontaine wrote: test=# CREATE TABLE badname (id int, a int, b text); ERROR: invalid relation name: badname test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; SELECT 1 This doesn't even get picked up by ANY COMMAND. >> >> I think Andres should add an entry for his patch on the commitfest. Is >> it ok? > > I'll try Andres' patch this weekend while I test yours. Actually no matter which patch I apply first, they cause the other to fail to apply. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 2 March 2012 22:32, Dimitri Fontaine wrote: > Hi, > > Please find attached v13 of the command trigger patch, fixing most of > known items and rebased against master. Two important items remain to be > done, but I figured I should keep you posted in the meantime. Thanks Dimitri. I'll give it a spin this weekend. > Tom Lane writes: >> This seems over-complicated. Triggers on tables do not have alterable >> properties, why should command triggers? I vote for >> >> CREATE COMMAND TRIGGER name ... properties ...; >> >> DROP COMMAND TRIGGER name; >> >> full stop. If you want to run the same trigger function on some more >> commands, add another trigger name. > > I reworked ALTER COMMAND TRIGGER and DROP COMMAND TRIGGER in the > attached, and the catalog too so that the command trigger's name is now > unique there. I added separate index on the command name. I see you now only allow a single command to be attached to a trigger at creation time. I was actually fine with how it worked before, just not with the DROP COMMAND. So, CREATE COMMAND TRIGGER could add a trigger against several commands, but DROP COMMAND TRIGGER would drop the whole lot as a single trigger rather than having the ability to drop separate commands. This is how regular triggers work, in that you can attach to several types of SQL statement, but you have to drop the trigger as a whole rather than dropping individual statements. Tom, could you clarify your suggestion for this? By "properties", do you mean possibly several commands? > Thom Brown writes: >> Just so it's easy to scan. If someone is looking for CREATE CAST, >> they'd kind of expect it near the drop of the CREATE list, but it's >> actually toward the bottom. It just looks random at the moment. > > I did M-x sort-lines in the documentation. Still have to add entries > for the new catalog though. > >>> test=# CREATE TABLE badname (id int, a int, b text); >>> ERROR: invalid relation name: badname >>> test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; >>> SELECT 1 >>> >>> This doesn't even get picked up by ANY COMMAND. > > I think Andres should add an entry for his patch on the commitfest. Is > it ok? I'll try Andres' patch this weekend while I test yours. > Tom Lane writes: >> FWIW, I agree with Thom on this. If we do it as you suggest, I >> confidently predict that it will be less than a year before we seriously >> regret it. Given all the discussion around this, it's borderline insane >> to believe that the set of parameters to be passed to command triggers >> is nailed down and won't need to change in the future. > > That too will need to wait for the next revision, it's just about > finding enough cycles, which is definitely happening very soon. Could you give your thoughts on the design? -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 28 February 2012 15:03, Tom Lane wrote: > Thom Brown writes: >> Well the problem is that you can add commands to a trigger en masse, >> but you can only remove them one at a time. Couldn't we at least >> allow the removal of multiple commands at the same time? The docs you >> wrote suggest you can do this, but you can't. > > This seems over-complicated. Triggers on tables do not have alterable > properties, why should command triggers? I vote for > > CREATE COMMAND TRIGGER name ... properties ...; > > DROP COMMAND TRIGGER name; > > full stop. If you want to run the same trigger function on some more > commands, add another trigger name. This would make more sense, particularly since dropping a command trigger, as it stands, doesn't necessarily drop the trigger. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Tom Lane wrote: > This seems over-complicated. Triggers on tables do not have > alterable properties, why should command triggers? I vote for > > CREATE COMMAND TRIGGER name ... properties ...; > > DROP COMMAND TRIGGER name; > > full stop. If you want to run the same trigger function on some > more commands, add another trigger name. +1 -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown writes: > Well the problem is that you can add commands to a trigger en masse, > but you can only remove them one at a time. Couldn't we at least > allow the removal of multiple commands at the same time? The docs you > wrote suggest you can do this, but you can't. This seems over-complicated. Triggers on tables do not have alterable properties, why should command triggers? I vote for CREATE COMMAND TRIGGER name ... properties ...; DROP COMMAND TRIGGER name; full stop. If you want to run the same trigger function on some more commands, add another trigger name. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 28 February 2012 11:43, Thom Brown wrote: > On 27 February 2012 19:37, Dimitri Fontaine wrote: >> Thom Brown writes: >>> CREATE COMMAND TRIGGER test_cmd_trg >>> BEFORE CREATE SCHEMA, >>> CREATE OPERATOR, >>> CREATE COLLATION, >>> CREATE CAST >>> EXECUTE PROCEDURE my_func(); >>> >>> I couldn't drop it completely unless I specified all of those commands. >>> Why? >> >> Because I couldn't find a nice enough way to implement that given the >> shared infrastructure Robert and Kaigai did put into place to handle >> dropping of objects. It could be that I didn't look hard enough, I'll >> be happy to get back that feature. > > Well the problem is that you can add commands to a trigger en masse, > but you can only remove them one at a time. Couldn't we at least > allow the removal of multiple commands at the same time? The docs you > wrote suggest you can do this, but you can't. Also note that as of commit 66f0cf7da8eeaeca4b9894bfafd61789b514af4a (Remove useless const qualifier) the patch no longer applies due to changes to src/backend/commands/typecmds.c. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 27 February 2012 19:37, Dimitri Fontaine wrote: > Thom Brown writes: >> CREATE COMMAND TRIGGER test_cmd_trg >> BEFORE CREATE SCHEMA, >> CREATE OPERATOR, >> CREATE COLLATION, >> CREATE CAST >> EXECUTE PROCEDURE my_func(); >> >> I couldn't drop it completely unless I specified all of those commands. Why? > > Because I couldn't find a nice enough way to implement that given the > shared infrastructure Robert and Kaigai did put into place to handle > dropping of objects. It could be that I didn't look hard enough, I'll > be happy to get back that feature. Well the problem is that you can add commands to a trigger en masse, but you can only remove them one at a time. Couldn't we at least allow the removal of multiple commands at the same time? The docs you wrote suggest you can do this, but you can't. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Tom Lane schrieb: >Andres Freund writes: >> I refreshed the patch so it works again on current HEAD. Basically >some >> trivial fixes and dfd26f9c5f371437f243249025863ea9911aacaa. The >latter doesn't >> seem necessary to me after the changes, so I simply ditched it. Am I >missing >> something? > >No, that was only needed because execMain.c was overriding somebody >else's tuple receiver. If you're putting the right receiver into the >QueryDesc to start with, it shouldn't be necessary. > >I'm confused by this though: > >> This basically includes a revert of >d8fb1f9adbddd1eef123d66a89a9fc0ecd96f60b > >I don't find that commit ID anywhere. That should have been the aforementioned commit. Must have screwed up the copy&paste buffer. Sorry for that. Andres Please excuse the brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Andres Freund writes: > I refreshed the patch so it works again on current HEAD. Basically some > trivial fixes and dfd26f9c5f371437f243249025863ea9911aacaa. The latter > doesn't > seem necessary to me after the changes, so I simply ditched it. Am I missing > something? No, that was only needed because execMain.c was overriding somebody else's tuple receiver. If you're putting the right receiver into the QueryDesc to start with, it shouldn't be necessary. I'm confused by this though: > This basically includes a revert of d8fb1f9adbddd1eef123d66a89a9fc0ecd96f60b I don't find that commit ID anywhere. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Tom Lane writes: > FWIW, I agree with Thom on this. If we do it as you suggest, I > confidently predict that it will be less than a year before we seriously > regret it. Given all the discussion around this, it's borderline insane > to believe that the set of parameters to be passed to command triggers > is nailed down and won't need to change in the future. I agree with the analysis… > As for special coding of support, it hardly seems onerous when every > language that has triggers at all has got some provision for the > existing trigger parameters. A bit of copying and pasting should get > the job done. But had been (too easily) convinced not to take that route. You changed my mind already, I'll see about changing the code too tomorrow (a cold is having me out of steam for tonight). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Dimitri Fontaine writes: > Thom Brown writes: >> I've got a question regarding the function signatures required for >> command triggers, and apologies if it's already been discussed to >> death (I didn't see all the original conversations around this). >> These differ from regular trigger functions which don't require any >> arguments, and instead use special variables. Why aren't we doing the >> same for command triggers? So instead of having the parameters > Basically so that we don't have to special code support for each and > every language out there. FWIW, I agree with Thom on this. If we do it as you suggest, I confidently predict that it will be less than a year before we seriously regret it. Given all the discussion around this, it's borderline insane to believe that the set of parameters to be passed to command triggers is nailed down and won't need to change in the future. As for special coding of support, it hardly seems onerous when every language that has triggers at all has got some provision for the existing trigger parameters. A bit of copying and pasting should get the job done. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On Monday, February 27, 2012 08:30:31 PM Thom Brown wrote: > On 27 February 2012 19:19, Dimitri Fontaine wrote: > > Thom Brown writes: > >> test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; > >> SELECT 1 > >> > >> This doesn't even get picked up by ANY COMMAND. > > > > You won't believe it: CTAS is not implemented as a DDL. Andres did > > some work about that and sent a patch that received positive reviews by > > both Tom and Robert, once that's in I can easily add support for the > > command. I actually don't think anybody actually reviewed the patch so far. Tom and I discussed the implementation strategy beforehand a bit though. > > Thanks Andres :) Youre welcome. Thanks for your awesome work that actually made it necessary ;) > I don't see it anywhere in the commitfest. Has it been properly submitted? I actually always viewed it as a part of the Dim's patch which is why I didn't submit it as a separate patch. Maybe that was a mistake... http://archives.postgresql.org/message- id/201112112346.07611.and...@anarazel.de contains the latest revision. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown writes: > CREATE COMMAND TRIGGER test_cmd_trg > BEFORE CREATE SCHEMA, > CREATE OPERATOR, > CREATE COLLATION, > CREATE CAST > EXECUTE PROCEDURE my_func(); > > I couldn't drop it completely unless I specified all of those commands. Why? Because I couldn't find a nice enough way to implement that given the shared infrastructure Robert and Kaigai did put into place to handle dropping of objects. It could be that I didn't look hard enough, I'll be happy to get back that feature. > Incidentally, I've noticed the DROP COMMAND TRIGGER has a mistake in the > syntax. Thanks, fix will be in the next version. > The documentation also needs to cover the pg_cmdtrigger catalogue as > it's not mentioned anywhere. That too, working on it now, adding forgotten forms you reported and more, adding regression tests, fixing weird cases, getting there :) > I'm enjoying playing with this feature though btw. :) Thanks :) -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown writes: > I've got a question regarding the function signatures required for > command triggers, and apologies if it's already been discussed to > death (I didn't see all the original conversations around this). > These differ from regular trigger functions which don't require any > arguments, and instead use special variables. Why aren't we doing the > same for command triggers? So instead of having the parameters Basically so that we don't have to special code support for each and every language out there. > Disadvantages are that there's more maintenance overhead for > supporting multiple languages using special variables. Lots of, so I've been told, enough of it for not taking this choice seriously. I'll admit I didn't personally looked at what it would entail implementation wise. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 27 February 2012 19:19, Dimitri Fontaine wrote: > Thom Brown writes: >> test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; >> SELECT 1 >> >> This doesn't even get picked up by ANY COMMAND. > > You won't believe it: CTAS is not implemented as a DDL. Andres did > some work about that and sent a patch that received positive reviews by > both Tom and Robert, once that's in I can easily add support for the > command. > > Thanks Andres :) I don't see it anywhere in the commitfest. Has it been properly submitted? -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown writes: > SELECT * INTO badname FROM goodname; Again, see Andres' patch about that. -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thom Brown writes: > test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; > SELECT 1 > > This doesn't even get picked up by ANY COMMAND. You won't believe it: CTAS is not implemented as a DDL. Andres did some work about that and sent a patch that received positive reviews by both Tom and Robert, once that's in I can easily add support for the command. Thanks Andres :) -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 26 February 2012 19:49, Thom Brown wrote: > On 26 February 2012 14:12, Dimitri Fontaine wrote: >> Thanks for your further testing! >> >> Thom Brown writes: >>> Further testing reveals a problem with FTS configurations when using >>> the example function provided in the docs: >> >> Could you send me your tests so that I add them to the proper regression >> test? I've been lazy on one or two object types and obviously that's >> where I have to check some more. > > Which tests? The FTS Config test was what I posted before. I haven't > gone to any great effort to set up tests for each command. I've just > been making them up as I go along. > >>> What is DROP ASSERTION? It's showing as a valid command for a command >>> trigger, but it's not documented. >> >> It's a Not Implemented Feature for which we have the grammar support to >> be able to fill a standard compliant checkbox, or something like that. >> It could be better for me to remove explicit support for it in the >> command triggers patch? > > Well considering there are commands that exist which we don't allow > triggers on, it seems weird to support triggers on commands which > aren't implemented. DROP ASSERTION doesn't appear anywhere else in > the documentation, so I can't think of how supporting a trigger for it > could be useful. > >>> I've noticed that ALTER name OWNER TO role doesn't result in >>> any trigger being fired except for tables. >>> >>> ALTER OPERATOR FAMILY RENAME TO ... doesn't fire command triggers. >>> >>> ALTER OPERATOR CLASS with RENAME TO or OWNER TO doesn't fire command >>> triggers, but with SET SCHEMA it does. >> >> It seems I've forgotten to add some support here, that happens in >> alter.c and is easy enough to check and complete, thanks for the >> testing. > > So would the fix cover many cases at once? > >>> I'll hold off on testing any further until a new patch is available. >> >> That should happen soon. Ah, the joys of coding while kids are at home >> thanks to school holidays. I can't count how many times I've been killed >> by a captain and married to a princess while writing that patch, sorry >> about those hiccups here. > > Being killed by a captain does make things more difficult, yes. I've got a question regarding the function signatures required for command triggers, and apologies if it's already been discussed to death (I didn't see all the original conversations around this). These differ from regular trigger functions which don't require any arguments, and instead use special variables. Why aren't we doing the same for command triggers? So instead of having the parameters tg_when, cmd_tag, objectid, schemaname and objectname, using pl/pgsql as an example, we'd have the variables TG_WHEN (already exists), TG_OP (already exists and equivalent to cmd_tag), TG_RELID (already exists, although maybe not directly equivalent), TG_REL_SCHEMA (doesn't exist but would replace schemaname) and TG_RELNAME (this is actually deprecated but could be re-used for this purpose). Advantages of implementing it like this is that there's consistency in the trigger system, it's easier as no function parameters required, and any future options you may wish to add won't break functions from previous versions, meaning more room for adding stuff later on. Disadvantages are that there's more maintenance overhead for supporting multiple languages using special variables. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 26 February 2012 14:12, Dimitri Fontaine wrote: > Thanks for your further testing! > > Thom Brown writes: >> Further testing reveals a problem with FTS configurations when using >> the example function provided in the docs: > > Could you send me your tests so that I add them to the proper regression > test? I've been lazy on one or two object types and obviously that's > where I have to check some more. Which tests? The FTS Config test was what I posted before. I haven't gone to any great effort to set up tests for each command. I've just been making them up as I go along. >> What is DROP ASSERTION? It's showing as a valid command for a command >> trigger, but it's not documented. > > It's a Not Implemented Feature for which we have the grammar support to > be able to fill a standard compliant checkbox, or something like that. > It could be better for me to remove explicit support for it in the > command triggers patch? Well considering there are commands that exist which we don't allow triggers on, it seems weird to support triggers on commands which aren't implemented. DROP ASSERTION doesn't appear anywhere else in the documentation, so I can't think of how supporting a trigger for it could be useful. >> I've noticed that ALTER name OWNER TO role doesn't result in >> any trigger being fired except for tables. >> >> ALTER OPERATOR FAMILY RENAME TO ... doesn't fire command triggers. >> >> ALTER OPERATOR CLASS with RENAME TO or OWNER TO doesn't fire command >> triggers, but with SET SCHEMA it does. > > It seems I've forgotten to add some support here, that happens in > alter.c and is easy enough to check and complete, thanks for the > testing. So would the fix cover many cases at once? >> I'll hold off on testing any further until a new patch is available. > > That should happen soon. Ah, the joys of coding while kids are at home > thanks to school holidays. I can't count how many times I've been killed > by a captain and married to a princess while writing that patch, sorry > about those hiccups here. Being killed by a captain does make things more difficult, yes. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
Thanks for your further testing!
Thom Brown writes:
> Further testing reveals a problem with FTS configurations when using
> the example function provided in the docs:
Could you send me your tests so that I add them to the proper regression
test? I've been lazy on one or two object types and obviously that's
where I have to check some more.
> Also command triggers for DROP CONVERSION aren't working. A glance at
> pg_cmdtrigger shows that the system views the command as "DROP
> CONVERSION_P".
That's easy to fix, that's a typo in gram.y. I'm not seeing other ones
like this though.
- | DROP CONVERSION_P
{ $$ = "DROP CONVERSION_P"; }
+ | DROP CONVERSION_P
{ $$ = "DROP CONVERSION"; }
> What is DROP ASSERTION? It's showing as a valid command for a command
> trigger, but it's not documented.
It's a Not Implemented Feature for which we have the grammar support to
be able to fill a standard compliant checkbox, or something like that.
It could be better for me to remove explicit support for it in the
command triggers patch?
> I've noticed that ALTER name OWNER TO role doesn't result in
> any trigger being fired except for tables.
>
> ALTER OPERATOR FAMILY RENAME TO ... doesn't fire command triggers.
>
> ALTER OPERATOR CLASS with RENAME TO or OWNER TO doesn't fire command
> triggers, but with SET SCHEMA it does.
It seems I've forgotten to add some support here, that happens in
alter.c and is easy enough to check and complete, thanks for the
testing.
> And there's no command trigger available for ALTER VIEW.
Will add.
> I'll hold off on testing any further until a new patch is available.
That should happen soon. Ah, the joys of coding while kids are at home
thanks to school holidays. I can't count how many times I've been killed
by a captain and married to a princess while writing that patch, sorry
about those hiccups here.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 25 February 2012 16:36, Thom Brown wrote: > On 25 February 2012 14:30, Thom Brown wrote: >> On 25 February 2012 13:28, Thom Brown wrote: >>> On 25 February 2012 13:15, Thom Brown wrote: On 25 February 2012 12:42, Thom Brown wrote: > On 25 February 2012 12:07, Thom Brown wrote: >> On 25 February 2012 12:00, Dimitri Fontaine >> wrote: >> >> D'oh, just as I sent some more queries... >> >>> Thom Brown writes: Is there any reason why the list of commands that command triggers can be used with isn't in alphabetical order? Also it appears to show >>> >>> Any reason why? I don't suppose it's really important one way or the >>> other, so I'm waiting on some more voices before working on it. >> >> Just so it's easy to scan. If someone is looking for CREATE CAST, >> they'd kind of expect it near the drop of the CREATE list, but it's >> actually toward the bottom. It just looks random at the moment. >> The ALTER COMMAND TRIGGER page also doesn't show which commands it can be used against. Perhaps, rather than repeat the list, there could be a note to say that a list of valid commands can be found on the CREATE COMMAND TRIGGER page? >>> >>> Well you can only alter a command that you were successful in creating, >>> right? So I'm not sure that's needed here. By that count though, I >>> maybe should remove the supported command list from DROP COMMAND TRIGGER >>> reference page? >> >> Sure, that would be more consistent. You're right, it's not needed. >> It just seemed odd that one of the statements lacked what both others >> had. > > Yet another comment... (I should have really started looking at this > at an earlier stage) > > It seems that if one were to enforce a naming convention for relations > as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be > circumvented by someone using CREATE TABLE name AS... > > test=# CREATE TABLE badname (id int, a int, b text); > ERROR: invalid relation name: badname > test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; > SELECT 1 > > This doesn't even get picked up by ANY COMMAND. CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall. I'd expect ALTER COMMAND TRIGGER to output too for when individual commands are disabled etc. >>> >>> Just found another case where a table can be created without a command >>> trigger firing: >>> >>> SELECT * INTO badname FROM goodname; >> >> Right, hopefully this should be my last piece of list spam for the >> time being. (apologies, I thought I'd just try it out at first, but >> it's ended up being reviewed piecemeal) > > I was wrong.. a couple of corrections to my own response: > >> On CREATE COMMAND TRIGGER page: >> >> “The trigger will be associated with the specified command and will >> execute the specified function function_name when that command is >> run.” >> should be: >> “The trigger will be associated with the specified commands and will >> execute the specified function function_name when those commands are >> run.” > > Actually, perhaps "...when any of those commands..." > >> On ALTER COMMAND TRIGGER page: >> >> “ALTER COMMAND TRIGGER name ON command SET enabled” >> should be: >> “ALTER COMMAND TRIGGER name ON command [, ... ] SET enabled” > > This one is nonsense, so please ignore it. Further testing reveals a problem with FTS configurations when using the example function provided in the docs: test=# CREATE TEXT SEARCH CONFIGURATION test ( PARSER = "default" ); ERROR: invalid relation name: test=# CREATE TEXT SEARCH CONFIGURATION fr_test ( PARSER = "default" ); ERROR: invalid relation name: The 2nd one should work as it matches the naming convention checked in the function. The ALTER and DROP equivalents appear to be fine though. DROP CAST shares a similar issue too: test=# DROP CAST (bigint as int4); ERROR: invalid relation name: � The odd thing about this one is that CREATE CAST shouldn't match on name at all, but it creates a cast successfully, whereas DROP CAST disagrees with the name. Command triggers for CREATE TYPE don't work, but fine for ALTER TYPE and DROP TYPE. Also command triggers for DROP CONVERSION aren't working. A glance at pg_cmdtrigger shows that the system views the command as "DROP CONVERSION_P". What is DROP ASSERTION? It's showing as a valid command for a command trigger, but it's not documented. I've noticed that ALTER name OWNER TO role doesn't result in any trigger being fired except for tables. ALTER OPERATOR FAMILY RENAME TO ... doesn't fire command triggers. ALTER OPERATOR CLASS with RENAME TO or OWNER TO doesn't fire command triggers, but with SET SCHEMA it does. And there's no command trigger available for ALTER VIEW. I'll hold off on testing any further until a new patch is available.
Re: [HACKERS] Command Triggers, patch v11
On 25 February 2012 14:30, Thom Brown wrote: > On 25 February 2012 13:28, Thom Brown wrote: >> On 25 February 2012 13:15, Thom Brown wrote: >>> On 25 February 2012 12:42, Thom Brown wrote: On 25 February 2012 12:07, Thom Brown wrote: > On 25 February 2012 12:00, Dimitri Fontaine > wrote: > > D'oh, just as I sent some more queries... > >> Thom Brown writes: >>> Is there any reason why the list of commands that command triggers can >>> be used with isn't in alphabetical order? Also it appears to show >> >> Any reason why? I don't suppose it's really important one way or the >> other, so I'm waiting on some more voices before working on it. > > Just so it's easy to scan. If someone is looking for CREATE CAST, > they'd kind of expect it near the drop of the CREATE list, but it's > actually toward the bottom. It just looks random at the moment. > >>> The ALTER COMMAND TRIGGER page also doesn't show which commands it can >>> be used against. Perhaps, rather than repeat the list, there could be >>> a note to say that a list of valid commands can be found on the CREATE >>> COMMAND TRIGGER page? >> >> Well you can only alter a command that you were successful in creating, >> right? So I'm not sure that's needed here. By that count though, I >> maybe should remove the supported command list from DROP COMMAND TRIGGER >> reference page? > > Sure, that would be more consistent. You're right, it's not needed. > It just seemed odd that one of the statements lacked what both others > had. Yet another comment... (I should have really started looking at this at an earlier stage) It seems that if one were to enforce a naming convention for relations as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be circumvented by someone using CREATE TABLE name AS... test=# CREATE TABLE badname (id int, a int, b text); ERROR: invalid relation name: badname test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; SELECT 1 This doesn't even get picked up by ANY COMMAND. >>> >>> CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall. I'd >>> expect ALTER COMMAND TRIGGER to output too for when individual >>> commands are disabled etc. >> >> Just found another case where a table can be created without a command >> trigger firing: >> >> SELECT * INTO badname FROM goodname; > > Right, hopefully this should be my last piece of list spam for the > time being. (apologies, I thought I'd just try it out at first, but > it's ended up being reviewed piecemeal) I was wrong.. a couple of corrections to my own response: > On CREATE COMMAND TRIGGER page: > > “The trigger will be associated with the specified command and will > execute the specified function function_name when that command is > run.” > should be: > “The trigger will be associated with the specified commands and will > execute the specified function function_name when those commands are > run.” Actually, perhaps "...when any of those commands..." > On ALTER COMMAND TRIGGER page: > > “ALTER COMMAND TRIGGER name ON command SET enabled” > should be: > “ALTER COMMAND TRIGGER name ON command [, ... ] SET enabled” This one is nonsense, so please ignore it. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 25 February 2012 13:28, Thom Brown wrote: > On 25 February 2012 13:15, Thom Brown wrote: >> On 25 February 2012 12:42, Thom Brown wrote: >>> On 25 February 2012 12:07, Thom Brown wrote: On 25 February 2012 12:00, Dimitri Fontaine wrote: D'oh, just as I sent some more queries... > Thom Brown writes: >> Is there any reason why the list of commands that command triggers can >> be used with isn't in alphabetical order? Also it appears to show > > Any reason why? I don't suppose it's really important one way or the > other, so I'm waiting on some more voices before working on it. Just so it's easy to scan. If someone is looking for CREATE CAST, they'd kind of expect it near the drop of the CREATE list, but it's actually toward the bottom. It just looks random at the moment. >> The ALTER COMMAND TRIGGER page also doesn't show which commands it can >> be used against. Perhaps, rather than repeat the list, there could be >> a note to say that a list of valid commands can be found on the CREATE >> COMMAND TRIGGER page? > > Well you can only alter a command that you were successful in creating, > right? So I'm not sure that's needed here. By that count though, I > maybe should remove the supported command list from DROP COMMAND TRIGGER > reference page? Sure, that would be more consistent. You're right, it's not needed. It just seemed odd that one of the statements lacked what both others had. >>> >>> Yet another comment... (I should have really started looking at this >>> at an earlier stage) >>> >>> It seems that if one were to enforce a naming convention for relations >>> as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be >>> circumvented by someone using CREATE TABLE name AS... >>> >>> test=# CREATE TABLE badname (id int, a int, b text); >>> ERROR: invalid relation name: badname >>> test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; >>> SELECT 1 >>> >>> This doesn't even get picked up by ANY COMMAND. >> >> CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall. I'd >> expect ALTER COMMAND TRIGGER to output too for when individual >> commands are disabled etc. > > Just found another case where a table can be created without a command > trigger firing: > > SELECT * INTO badname FROM goodname; Right, hopefully this should be my last piece of list spam for the time being. (apologies, I thought I'd just try it out at first, but it's ended up being reviewed piecemeal) On CREATE COMMAND TRIGGER page: “The trigger will be associated with the specified command and will execute the specified function function_name when that command is run.” should be: “The trigger will be associated with the specified commands and will execute the specified function function_name when those commands are run.” “A command trigger's function must return void, the only it can aborts the execution of the command is by raising an exception.” should be: “A command trigger's function must return void. It can then only abort the execution of the command by raising an exception.” Remove: “For a constraint trigger, this is also the name to use when modifying the trigger's behavior using SET CONSTRAINTS.” Remove: “That leaves out the following list of non supported commands.” s/exercize/exercise/ “that's the case for VACUUM, CLUSTER CREATE INDEX CONCURRENTLY, and REINDEX DATABASE.” should be: “that's the case for VACUUM, CLUSTER, CREATE INDEX CONCURRENTLY, and REINDEX DATABASE.” I don’t understand this sentence: “Triggers on ANY command support more commands than just this list, and will only provide the command tag argument as NOT NULL.” On ALTER COMMAND TRIGGER page: “ALTER COMMAND TRIGGER name ON command SET enabled” should be: “ALTER COMMAND TRIGGER name ON command [, ... ] SET enabled” On DROP COMMAND TRIGGER page: There’s a mention of CASCADE and RESTRICT. I don’t know of any object which could be dependant on a command trigger, so I don’t see what these are for. An oddity I’ve noticed is that you can add additional commands to an existing command trigger, and you can also have them execute a different function to the other commands referenced in the same trigger. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 25 February 2012 13:15, Thom Brown wrote: > On 25 February 2012 12:42, Thom Brown wrote: >> On 25 February 2012 12:07, Thom Brown wrote: >>> On 25 February 2012 12:00, Dimitri Fontaine wrote: >>> >>> D'oh, just as I sent some more queries... >>> Thom Brown writes: > Is there any reason why the list of commands that command triggers can > be used with isn't in alphabetical order? Also it appears to show Any reason why? I don't suppose it's really important one way or the other, so I'm waiting on some more voices before working on it. >>> >>> Just so it's easy to scan. If someone is looking for CREATE CAST, >>> they'd kind of expect it near the drop of the CREATE list, but it's >>> actually toward the bottom. It just looks random at the moment. >>> > The ALTER COMMAND TRIGGER page also doesn't show which commands it can > be used against. Perhaps, rather than repeat the list, there could be > a note to say that a list of valid commands can be found on the CREATE > COMMAND TRIGGER page? Well you can only alter a command that you were successful in creating, right? So I'm not sure that's needed here. By that count though, I maybe should remove the supported command list from DROP COMMAND TRIGGER reference page? >>> >>> Sure, that would be more consistent. You're right, it's not needed. >>> It just seemed odd that one of the statements lacked what both others >>> had. >> >> Yet another comment... (I should have really started looking at this >> at an earlier stage) >> >> It seems that if one were to enforce a naming convention for relations >> as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be >> circumvented by someone using CREATE TABLE name AS... >> >> test=# CREATE TABLE badname (id int, a int, b text); >> ERROR: invalid relation name: badname >> test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; >> SELECT 1 >> >> This doesn't even get picked up by ANY COMMAND. > > CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall. I'd > expect ALTER COMMAND TRIGGER to output too for when individual > commands are disabled etc. Just found another case where a table can be created without a command trigger firing: SELECT * INTO badname FROM goodname; -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Command Triggers, patch v11
On 25 February 2012 12:42, Thom Brown wrote: > On 25 February 2012 12:07, Thom Brown wrote: >> On 25 February 2012 12:00, Dimitri Fontaine wrote: >> >> D'oh, just as I sent some more queries... >> >>> Thom Brown writes: Is there any reason why the list of commands that command triggers can be used with isn't in alphabetical order? Also it appears to show >>> >>> Any reason why? I don't suppose it's really important one way or the >>> other, so I'm waiting on some more voices before working on it. >> >> Just so it's easy to scan. If someone is looking for CREATE CAST, >> they'd kind of expect it near the drop of the CREATE list, but it's >> actually toward the bottom. It just looks random at the moment. >> The ALTER COMMAND TRIGGER page also doesn't show which commands it can be used against. Perhaps, rather than repeat the list, there could be a note to say that a list of valid commands can be found on the CREATE COMMAND TRIGGER page? >>> >>> Well you can only alter a command that you were successful in creating, >>> right? So I'm not sure that's needed here. By that count though, I >>> maybe should remove the supported command list from DROP COMMAND TRIGGER >>> reference page? >> >> Sure, that would be more consistent. You're right, it's not needed. >> It just seemed odd that one of the statements lacked what both others >> had. > > Yet another comment... (I should have really started looking at this > at an earlier stage) > > It seems that if one were to enforce a naming convention for relations > as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be > circumvented by someone using CREATE TABLE name AS... > > test=# CREATE TABLE badname (id int, a int, b text); > ERROR: invalid relation name: badname > test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b; > SELECT 1 > > This doesn't even get picked up by ANY COMMAND. CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall. I'd expect ALTER COMMAND TRIGGER to output too for when individual commands are disabled etc. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
