Thread: Use of systable_beginscan_ordered in event trigger patch
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.
Accordingly, that function isn't supposed to be used in any places
where you cannot avoid its use during recovery of core system indexes.
I am not sure to what extent its use in the TOAST support compromises
that position, but for sure the fact that it's called from
EventTriggerDDLCommandStart has broken the concept completely.
If pg_event_trigger_evtname_index becomes corrupt, you can kiss your
database goodbye, because you have no hope whatsoever of issuing the
commands needed to reindex it.
Maybe it's time to bite the bullet and implement a heapscan-and-sort
code path for systable_beginscan_ordered to use when
ignore_system_indexes is set. But that's a fair amount of work.
The path of least resistance would be to make the event trigger stuff
not depend on this function.
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.
regards, tom lane
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
On Tue, Aug 28, 2012 at 12:47 PM, Andres Freund <andres@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
Tom Lane <tgl@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
Tom Lane <tgl@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
Attachment
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 <dimitri@2ndQuadrant.fr> writes: > Tom Lane <tgl@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 -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Dimitri Fontaine <dimitri@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
Tom Lane <tgl@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 RAISEEXCEPTION '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
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@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
Tom Lane <tgl@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
On Wed, Dec 12, 2012 at 3:51 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Tom Lane <tgl@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
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Dec 12, 2012 at 3:51 PM, Dimitri Fontaine
> <dimitri@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
On Thu, Dec 13, 2012 at 6:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Dec 12, 2012 at 3:51 PM, Dimitri Fontaine >> <dimitri@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
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Dec 13, 2012 at 6:25 PM, Tom Lane <tgl@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
On Fri, Dec 14, 2012 at 2:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Dec 13, 2012 at 6:25 PM, Tom Lane <tgl@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
Robert Haas <robertmhaas@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 <tgl@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
On Fri, Dec 14, 2012 at 3:46 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Robert Haas <robertmhaas@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 <tgl@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
Robert Haas <robertmhaas@gmail.com> writes: > ISTM that a PGC_SUSER GUC, as I proposed previously, would serve this > need adequately, without the cost of more cruft in gram.y. I can't help but think about the experiments you did some time ago about splitting the grammar into differents sub-grammars (for lack of a better term). If I remember correctly, your result showed no performance gain from separating away Queries and DML on the one side from the rest, DDL and DCL and such like. IIRC, you didn't have a regression either. Now, what about splitting those grammars in order to freely add any new production rules with or without new keywords for administrative commands, without a blink of though about the main parser grammar. I guess that the "traffic cop" would need to have a decent fast path to very quickly get to use the right parser, and I suppose you did already implement that in your previous experiment. If that's sensible as a way forward, that can also be the basis for allowing extensions to implement their own command set too. The trick would be how to implement extensible grammar routing. That would come way after we have the initial split, though. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Now, what about splitting those grammars in order to freely add any new
> production rules with or without new keywords for administrative
> commands, without a blink of though about the main parser grammar.
Let me explain to you why there will never be a situation where we can
consider new keywords to be zero-cost.
$ size src/backend/parser/gram.o text data bss dec hex filename952864 104 0 952968 e8a88
src/backend/parser/gram.o
$ size src/backend/postgres text data bss dec hex filename
6815102 123416 239356 7177874 6d8692 src/backend/postgres
That is, the grammar tables already amount to 14% of the total bulk of
the server executable. (The above numbers exclude debug symbols BTW.)
That bloat is not free; for one thing, it's competing for L1 cache with
all the actual code in the backend. And the main cause of it is that
we have lots-and-lots of keywords, because the parser tables are
basically number-of-tokens wide by number-of-states high. (In HEAD
there are 433 tokens known to the grammar, all but 30 of which are
keywords, and 4367 states.)
Splitting the grammar into multiple grammars is unlikely to do much to
improve this --- in fact, it could easily make matters worse due to
duplication. Rather, we just have to be careful about adding new
keywords. In this connection, I quite like the fact that recent syntax
extensions such as EXPLAIN (...options...) have managed to avoid making
the option names into grammar keywords at all.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Let me explain to you why there will never be a situation where we can > consider new keywords to be zero-cost. Thanks for taking the time to do this. > Splitting the grammar into multiple grammars is unlikely to do much to > improve this --- in fact, it could easily make matters worse due to > duplication. Rather, we just have to be careful about adding new > keywords. In this connection, I quite like the fact that recent syntax > extensions such as EXPLAIN (...options...) have managed to avoid making > the option names into grammar keywords at all. I'm still pretty unhappy about this state of affairs. I would like to have a fast path and a "you can go crazy here" path. Apparently the solutions to reduce the footprint involve hand made recursive decent parsers which are harder to maintain. I've read a little about the packrat parsing techniques, but far from enough to understand how much they could help us solve the binary bloat problem we have here while not making it harder to maintain our grammar. Maybe some other techniques are available… Ideas? Or should I just bite the bullet? -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support