Re: [HACKERS] Use of systable_beginscan_ordered in event trigger patch
On Thu, Dec 13, 2012 at 6:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Wed, Dec 12, 2012 at 3:51 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert, does that ring a bell to you? I'm going to crawl the archives tomorrow if not… Yeah, I'm pretty sure you can't set event triggers of any kind on event triggers. You proposed to allow some stuff that would affect every command, but I yelled and screamed about that until we got rid of it, 'cuz it just seemed way too dangerous. In that case the docs should probably mention it more prominently; the example in CREATE EVENT TRIGGER is misleadingly described, for sure. I suspect there are still ways to shoot yourself in the foot with a broken event trigger, but it's not quite as trivial as I thought. I'm smart enough not to doubt you, but I'd sure appreciate a hint as to what you're worried about before we start building more on top of it. I don't want to sink a lot of work into follow-on commits and then discover during beta we have to rip it all out or disable it. It seems to me that if you can always drop an event trigger without interference from the event trigger system, you should at least be able to shut it off if you don't like what it's doing. I'm a little worried that there could be ways to crash the server or corrupt data, but I don't know what they are. ISTM that, at least for the firing point we have right now, it's not much different than executing the event trigger code before you execute the DDL command, and therefore it should be safe. But I'm very aware that I might be wrong, hence the extremely conservative first commit. -- 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] Use of systable_beginscan_ordered in event trigger patch
Robert Haas robertmh...@gmail.com writes: On Thu, Dec 13, 2012 at 6:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: I suspect there are still ways to shoot yourself in the foot with a broken event trigger, but it's not quite as trivial as I thought. I'm smart enough not to doubt you, but I'd sure appreciate a hint as to what you're worried about before we start building more on top of it. I don't want to sink a lot of work into follow-on commits and then discover during beta we have to rip it all out or disable it. It seems to me that if you can always drop an event trigger without interference from the event trigger system, you should at least be able to shut it off if you don't like what it's doing. I doubt that not firing on DROP EVENT TRIGGER, per se, will be sufficient to guarantee that you can execute such a drop. Even if that's true in the current state of the code, we're already hearing requests for event triggers on lower-level events, eg http://archives.postgresql.org/pgsql-hackers/2012-12/msg00314.php Sooner or later there will be one somewhere in the code path involved in doing a heap_delete on pg_event_trigger, or one of the subsidiary catalogs such as pg_depend, or maybe one that just manages to fire someplace during backend startup, or who knows what. Hence, I want a kill switch for all event triggers that will most certainly work, and the just-committed patch provides one. 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] Use of systable_beginscan_ordered in event trigger patch
On Fri, Dec 14, 2012 at 2:00 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Dec 13, 2012 at 6:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: I suspect there are still ways to shoot yourself in the foot with a broken event trigger, but it's not quite as trivial as I thought. I'm smart enough not to doubt you, but I'd sure appreciate a hint as to what you're worried about before we start building more on top of it. I don't want to sink a lot of work into follow-on commits and then discover during beta we have to rip it all out or disable it. It seems to me that if you can always drop an event trigger without interference from the event trigger system, you should at least be able to shut it off if you don't like what it's doing. I doubt that not firing on DROP EVENT TRIGGER, per se, will be sufficient to guarantee that you can execute such a drop. Even if that's true in the current state of the code, we're already hearing requests for event triggers on lower-level events, eg http://archives.postgresql.org/pgsql-hackers/2012-12/msg00314.php Yep, true. Sooner or later there will be one somewhere in the code path involved in doing a heap_delete on pg_event_trigger, or one of the subsidiary catalogs such as pg_depend, or maybe one that just manages to fire someplace during backend startup, or who knows what. Yeah. :-( Hence, I want a kill switch for all event triggers that will most certainly work, and the just-committed patch provides one. I'm definitely not disputing the need for that patch. -- 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] Use of systable_beginscan_ordered in event trigger patch
Robert Haas robertmh...@gmail.com writes: Robert, does that ring a bell to you? I'm going to crawl the archives tomorrow if not… Yeah, I'm pretty sure you can't set event triggers of any kind on event triggers. You proposed to allow some stuff that would affect every command, but I yelled and screamed about that until we got rid of it, 'cuz it just seemed way too dangerous. I meant about the way the documentation is phrased to introduce the example, which is somewhat wrong because not all commands are concerned, quite a bunch of them will not be disabled by such a command trigger. Tom Lane t...@sss.pgh.pa.us writes: Sooner or later there will be one somewhere in the code path involved in doing a heap_delete on pg_event_trigger, or one of the subsidiary catalogs such as pg_depend, or maybe one that just manages to fire someplace during backend startup, or who knows what. You're right that we need to be careful here, in ways that I didn't foresee. The first thing I can think of is to disable such low level events on system catalogs, of course. Hence, I want a kill switch for all event triggers that will most certainly work, and the just-committed patch provides one. We absolutely need that, and running Event Triggers in standalone mode seems counter productive to me anyway. That said maybe we need to be able to have a per-session leave me alone mode of operation. What do you think of ALTER EVENT TRIGGER DISABLE ALL; -- just tried, no conflict I don't think we need the ENABLE ALL variant, and it would not be symetric anyway as you would want to be able to only enable those event triggers that were already enabled before, and it seems to me that cancelling a statement is best done with ROLLBACK; or ROLLBACK TO SAVEPOINT …;. 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] Use of systable_beginscan_ordered in event trigger patch
On Fri, Dec 14, 2012 at 3:46 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@gmail.com writes: Robert, does that ring a bell to you? I'm going to crawl the archives tomorrow if not… Yeah, I'm pretty sure you can't set event triggers of any kind on event triggers. You proposed to allow some stuff that would affect every command, but I yelled and screamed about that until we got rid of it, 'cuz it just seemed way too dangerous. I meant about the way the documentation is phrased to introduce the example, which is somewhat wrong because not all commands are concerned, quite a bunch of them will not be disabled by such a command trigger. Tom Lane t...@sss.pgh.pa.us writes: Sooner or later there will be one somewhere in the code path involved in doing a heap_delete on pg_event_trigger, or one of the subsidiary catalogs such as pg_depend, or maybe one that just manages to fire someplace during backend startup, or who knows what. You're right that we need to be careful here, in ways that I didn't foresee. The first thing I can think of is to disable such low level events on system catalogs, of course. Hence, I want a kill switch for all event triggers that will most certainly work, and the just-committed patch provides one. We absolutely need that, and running Event Triggers in standalone mode seems counter productive to me anyway. That said maybe we need to be able to have a per-session leave me alone mode of operation. What do you think of ALTER EVENT TRIGGER DISABLE ALL; -- just tried, no conflict I don't think we need the ENABLE ALL variant, and it would not be symetric anyway as you would want to be able to only enable those event triggers that were already enabled before, and it seems to me that cancelling a statement is best done with ROLLBACK; or ROLLBACK TO SAVEPOINT …;. ISTM that a PGC_SUSER GUC, as I proposed previously, would serve this need adequately, without the cost of more cruft in gram.y. Am I wrong? -- 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] Use of systable_beginscan_ordered in event trigger patch
On Wed, Dec 12, 2012 at 3:51 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Tom Lane t...@sss.pgh.pa.us writes: Maybe we could just append to it how to remove such an event trigger, which is easy to do because you can't target an event trigger related command with event triggers, so the following is not disabled: DROP EVENT TRIGGER abort_ddl; Oh really? The explanation of the example certainly doesn't say that. I remember than in my proposals somewhere we had support for a very limited form of command triggers for any command in the system. Of course as that included transaction control commands we made that better. I'm quite tired so maybe my memory is playing tricks to me, but I kind of remember that we also had quite a discussion about just that example and its phrasing in the docs and did came out with something satisfactory. Robert, does that ring a bell to you? I'm going to crawl the archives tomorrow if not… Yeah, I'm pretty sure you can't set event triggers of any kind on event triggers. You proposed to allow some stuff that would affect every command, but I yelled and screamed about that until we got rid of it, 'cuz it just seemed way too dangerous. -- 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] Use of systable_beginscan_ordered in event trigger patch
Robert Haas robertmh...@gmail.com writes: On Wed, Dec 12, 2012 at 3:51 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert, does that ring a bell to you? I'm going to crawl the archives tomorrow if not Yeah, I'm pretty sure you can't set event triggers of any kind on event triggers. You proposed to allow some stuff that would affect every command, but I yelled and screamed about that until we got rid of it, 'cuz it just seemed way too dangerous. In that case the docs should probably mention it more prominently; the example in CREATE EVENT TRIGGER is misleadingly described, for sure. I suspect there are still ways to shoot yourself in the foot with a broken event trigger, but it's not quite as trivial as I thought. 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] Use of systable_beginscan_ordered in event trigger patch
Tom Lane t...@sss.pgh.pa.us writes: Yeah, I had forgotten about that loose end, but we definitely need something of the sort. Committed with additional documentation. (I put something in the CREATE EVENT TRIGGER ref page, but do we need anything about it in chapter 37?) Thanks! I guess we could add a note at the end of the Overview of Event Trigger Behavior section. Then maybe we should explain in that section that you can't set an event trigger to fire on event trigger commands. BTW, is the example in the CREATE EVENT TRIGGER ref page meant as an IQ test for unwary readers? You do know there are people who will copy-and-paste just about any example that's put in front of them. Perhaps we just want to make sure they internalize the advice about how to get out of a broken-event-trigger situation. For those following at home, the example is: CREATE OR REPLACE FUNCTION abort_any_command() RETURNS event_trigger LANGUAGE plpgsql AS $$ BEGIN RAISE EXCEPTION 'command % is disabled', tg_tag; END; $$; CREATE EVENT TRIGGER abort_ddl ON ddl_command_start EXECUTE PROCEDURE abort_any_command(); Maybe we could just append to it how to remove such an event trigger, which is easy to do because you can't target an event trigger related command with event triggers, so the following is not disabled: DROP EVENT TRIGGER abort_ddl; 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] Use of systable_beginscan_ordered in event trigger patch
Dimitri Fontaine dimi...@2ndquadrant.fr writes: Tom Lane t...@sss.pgh.pa.us writes: BTW, is the example in the CREATE EVENT TRIGGER ref page meant as an IQ test for unwary readers? Maybe we could just append to it how to remove such an event trigger, which is easy to do because you can't target an event trigger related command with event triggers, so the following is not disabled: DROP EVENT TRIGGER abort_ddl; Oh really? The explanation of the example certainly doesn't say that. 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] Use of systable_beginscan_ordered in event trigger patch
Tom Lane t...@sss.pgh.pa.us writes: Maybe we could just append to it how to remove such an event trigger, which is easy to do because you can't target an event trigger related command with event triggers, so the following is not disabled: DROP EVENT TRIGGER abort_ddl; Oh really? The explanation of the example certainly doesn't say that. I remember than in my proposals somewhere we had support for a very limited form of command triggers for any command in the system. Of course as that included transaction control commands we made that better. I'm quite tired so maybe my memory is playing tricks to me, but I kind of remember that we also had quite a discussion about just that example and its phrasing in the docs and did came out with something satisfactory. Robert, does that ring a bell to you? I'm going to crawl the archives tomorrow if not… 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] Use of systable_beginscan_ordered in event trigger patch
Hi, I don't remember that we fixed that case, I did attach a patch in the previous email, what do you think? Dimitri Fontaine dimi...@2ndquadrant.fr writes: Tom Lane t...@sss.pgh.pa.us writes: Or maybe we should disable event triggers altogether in standalone mode? Would something as simple as the attached work for doing that? (passes make check and I did verify manually that postmaster --single is happy with it and skipping Event Triggers). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/src/backend/commands/event_trigger.c --- b/src/backend/commands/event_trigger.c *** *** 567,572 EventTriggerDDLCommandStart(Node *parsetree) --- 567,585 EventTriggerDatatrigdata; /* + * Event Triggers are completely disabled in standalone mode so as not to + * prevent fixing a problematic situation. + * + * To enable Event Triggers in standalone mode we would have to stop using + * systable_beginscan_ordered so that it's still possible to rebuild + * corrupt indexes (thanks to ignore_system_indexes). One way to do that is + * implementing a heapscan-and-sort code path to use when + * ignore_system_indexes is set. + */ + if (!IsUnderPostmaster) + return; + + /* * We want the list of command tags for which this procedure is actually * invoked to match up exactly with the list that CREATE EVENT TRIGGER * accepts. This debugging cross-check will throw an error if this -- 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] Use of systable_beginscan_ordered in event trigger patch
Dimitri Fontaine dimi...@2ndquadrant.fr writes: I don't remember that we fixed that case, I did attach a patch in the previous email, what do you think? Yeah, I had forgotten about that loose end, but we definitely need something of the sort. Committed with additional documentation. (I put something in the CREATE EVENT TRIGGER ref page, but do we need anything about it in chapter 37?) BTW, is the example in the CREATE EVENT TRIGGER ref page meant as an IQ test for unwary readers? You do know there are people who will copy-and-paste just about any example that's put in front of them. Perhaps we just want to make sure they internalize the advice about how to get out of a broken-event-trigger situation. Kidding aside, I think we need either a big WARNING, or a different example. 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] Use of systable_beginscan_ordered in event trigger patch
Tom Lane t...@sss.pgh.pa.us writes: Or maybe we should disable event triggers altogether in standalone mode? Would something as simple as the attached work for doing that? (passes make check and I did verify manually that postmaster --single is happy with it and skipping Event Triggers). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/src/backend/commands/event_trigger.c --- b/src/backend/commands/event_trigger.c *** *** 567,572 EventTriggerDDLCommandStart(Node *parsetree) --- 567,585 EventTriggerData trigdata; /* + * Event Triggers are completely disabled in standalone mode so as not to + * prevent fixing a problematic situation. + * + * To enable Event Triggers in standalone mode we would have to stop using + * systable_beginscan_ordered so that it's still possible to rebuild + * corrupt indexes (thanks to ignore_system_indexes). One way to do that is + * implementing a heapscan-and-sort code path to use when + * ignore_system_indexes is set. + */ + if (!IsUnderPostmaster) + return; + + /* * We want the list of command tags for which this procedure is actually * invoked to match up exactly with the list that CREATE EVENT TRIGGER * accepts. This debugging cross-check will throw an error if this -- 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] Use of systable_beginscan_ordered in event trigger patch
Tom Lane t...@sss.pgh.pa.us writes: I find $SUBJECT fairly scary, because systable_beginscan_ordered() is dependent on having a working, non-corrupt index. If you are trying to run the backend with ignore_system_indexes so that you can rebuild corrupt indexes, uses of systable_beginscan_ordered() represent places where you can't turn that off, and are entirely at the mercy of the indexes being good. Ooops. Didn't see that, thanks for noticing! Or maybe we should disable event triggers altogether in standalone mode? +1 I can think of plenty of ways that a broken event trigger could cause enough havoc that you'd wish there was a way to suppress it, at least for long enough to drop it again. I fail to see how enabling Event Triggers in standalone mode would help you get out of the situation that lead you there. It's a last resort facility where you want the bare PostgreSQL behavior, I think. Now that you mention it. 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] Use of systable_beginscan_ordered in event trigger patch
On Tuesday, August 28, 2012 06:39:50 PM Tom Lane wrote: Or maybe we should disable event triggers altogether in standalone mode? I can think of plenty of ways that a broken event trigger could cause enough havoc that you'd wish there was a way to suppress it, at least for long enough to drop it again. +1 Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] Use of systable_beginscan_ordered in event trigger patch
On Tue, Aug 28, 2012 at 12:47 PM, Andres Freund and...@2ndquadrant.com wrote: On Tuesday, August 28, 2012 06:39:50 PM Tom Lane wrote: Or maybe we should disable event triggers altogether in standalone mode? I can think of plenty of ways that a broken event trigger could cause enough havoc that you'd wish there was a way to suppress it, at least for long enough to drop it again. +1 +1. I initially suggested a PGC_SUSET GUC to disable event triggers, and I'm still not entirely convinced that we shouldn't have one. Maybe we could just force it to disabled in standalone mode. -- 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
