Обсуждение: GUC for temporarily disabling event triggers
In the thread discussing the login event trigger patch it was argued that we want to avoid recommending single-user mode for troubleshooting tasks, and a GUC for temporarily disabling event triggers was proposed. Since the login event trigger patch lost momentum, I've broken out the GUC part into a separate patch to see if there is interest in that part alone, to chip away at situations requiring single-user mode. The patch adds a new GUC, ignore_event_trigger with two option values, 'all' and 'none' (the login event patch had 'login' as well). This can easily be expanded to have the different types of events, or pared down to a boolean on/off. I think it makes more sense to make it more fine-grained but I think there is merit in either direction. If there is interest in this I'm happy to pursue a polished version of this patch. -- Daniel Gustafsson https://vmware.com/
Вложения
> On 3 Nov 2022, at 21:47, Daniel Gustafsson <daniel@yesql.se> wrote: > The patch adds a new GUC, ignore_event_trigger with two option values, 'all' > and 'none' (the login event patch had 'login' as well). The attached v2 fixes a small bug which caused testfailures the CFBot. -- Daniel Gustafsson https://vmware.com/
Вложения
On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson <daniel@yesql.se> wrote: > > > On 3 Nov 2022, at 21:47, Daniel Gustafsson <daniel@yesql.se> wrote: > > > The patch adds a new GUC, ignore_event_trigger with two option values, 'all' > > and 'none' (the login event patch had 'login' as well). > > The attached v2 fixes a small bug which caused testfailures the CFBot. The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID 5f6401f81cb24bd3930e0dc589fc4aa8b5424cdc === === applying patch ./v2-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch patching file doc/src/sgml/config.sgml Hunk #1 succeeded at 9480 (offset 117 lines). ..... patching file src/backend/utils/misc/postgresql.conf.sample Hunk #1 FAILED at 701. 1 out of 1 hunk FAILED -- saving rejects to file src/backend/utils/misc/postgresql.conf.sample.rej [1] - http://cfbot.cputube.org/patch_41_4013.log Regards, Vignesh
> On 11 Jan 2023, at 17:38, vignesh C <vignesh21@gmail.com> wrote: > > On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson <daniel@yesql.se> wrote: >> >>> On 3 Nov 2022, at 21:47, Daniel Gustafsson <daniel@yesql.se> wrote: >> >>> The patch adds a new GUC, ignore_event_trigger with two option values, 'all' >>> and 'none' (the login event patch had 'login' as well). >> >> The attached v2 fixes a small bug which caused testfailures the CFBot. > > The patch does not apply on top of HEAD as in [1], please post a rebased patch: The attached rebased v3 fixes the conflict. -- Daniel Gustafsson https://vmware.com/
Вложения
On Thu, Jan 12, 2023 at 12:26 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> On 11 Jan 2023, at 17:38, vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 3 Nov 2022, at 21:47, Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> The patch adds a new GUC, ignore_event_trigger with two option values, 'all'
>>> and 'none' (the login event patch had 'login' as well).
>>
>> The attached v2 fixes a small bug which caused testfailures the CFBot.
>
> The patch does not apply on top of HEAD as in [1], please post a rebased patch:
The attached rebased v3 fixes the conflict.
--
Daniel Gustafsson https://vmware.com/
Hi,
It would be better to mention the name of GUC in the description.
Typo: suspending -> suspend
w.r.t. guc `ignore_event_trigger`, since it is supposed to disable event triggers for a short period of time, is there mechanism to turn it off (IGNORE_EVENT_TRIGGER_ALL) automatically ?
Cheers
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Hi Daniel, I have reviewed the patch and I liked it (well I did liked it already since it was a part of login trigger patch previously). All tests are passed and the manual experiments with all types of event triggers are also passed. Everything is fine and I think It can be marked as Ready for Committer, although I have one final question. There is a complete framework for disabling various types of the event triggers separately, but, the list of valid GUC valuesonly include 'all' and 'none'. Why not adding support for all the event trigger types separately? Everything is alreadythere in the patch; the only thing needed is expanding couple of enums. It's cheap in terms of code size and evencheaper in terms of performance. And moreover - it would be a good example for anyone adding new trigger types. The new status of this patch is: Waiting on Author
> On 27 Jan 2023, at 15:00, Mikhail Gribkov <youzhick@gmail.com> wrote: > There is a complete framework for disabling various types of the event triggers separately, but, the list of valid GUCvalues only include 'all' and 'none'. Why not adding support for all the event trigger types separately? Everything isalready there in the patch; the only thing needed is expanding couple of enums. It's cheap in terms of code size and evencheaper in terms of performance. And moreover - it would be a good example for anyone adding new trigger types. I can't exactly recall my reasoning, but I do think you're right that if we're to have this GUC it should support the types of existing EVTs. The updated v4 implements that as well as a rebase on top of master and fixing a typo discovered upthread. -- Daniel Gustafsson
Вложения
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed I like it now. * The patch does what it intends to do; * The implementation way is clear; * All test are passed; * No additional problems catched - at least by my eyes; I think it can be marked as Ready for Committer N.B. In fact I've encountered couple of failed tests during installcheck-world, although the same fails are there even formaster branch. Thus doesn't seem to be this patch issue. The new status of this patch is: Ready for Committer
> On 7 Mar 2023, at 16:02, Mikhail Gribkov <youzhick@gmail.com> wrote: > * The patch does what it intends to do; > * The implementation way is clear; > * All test are passed; > * No additional problems catched - at least by my eyes; > > I think it can be marked as Ready for Committer This patch has been RFC for some time, and has been all green in the CFbot. I would like to go ahead with it this cycle since it gives a tool for admins to avoid single-user mode - which is something we want to move away from. Even though login event triggers aren't going in (which is where this originated), there are still lots of ways to break things with other ev triggers. Any objections? -- Daniel Gustafsson
On Mon, Mar 06, 2023 at 01:24:56PM +0100, Daniel Gustafsson wrote: > > On 27 Jan 2023, at 15:00, Mikhail Gribkov <youzhick@gmail.com> wrote: > > > There is a complete framework for disabling various types of the event triggers separately, but, the list of valid GUCvalues only include 'all' and 'none'. Why not adding support for all the event trigger types separately? Everything isalready there in the patch; the only thing needed is expanding couple of enums. It's cheap in terms of code size and evencheaper in terms of performance. And moreover - it would be a good example for anyone adding new trigger types. > > I can't exactly recall my reasoning, but I do think you're right that if we're > to have this GUC it should support the types of existing EVTs. The updated v4 > implements that as well as a rebase on top of master and fixing a typo > discovered upthread. > + gettext_noop("Disable event triggers for the duration of the session."), Why does is it say "for the duration of the session" ? It's possible to disable ignoring, and within the same session. GUCs are typically "for the duration of the session" .. but they don't say so (and don't need to). + elog(ERROR, "unsupport event trigger: %d", event); typo: unsupported + Allows to temporarily disable event triggers from executing in order => Allow temporarily disabling execution of event triggers .. + to troubleshoot and repair faulty event triggers. The value matches + the type of event trigger to be ignored: + <literal>ddl_command_start</literal>, <literal>ddl_command_end</literal>, + <literal>table_rewrite</literal> and <literal>sql_drop</literal>. + Additionally, all event triggers can be disabled by setting it to + <literal>all</literal>. Setting the value to <literal>none</literal> + will ensure that all event triggers are enabled, this is the default It doesn't technically "ensure that they're enabled", since they can be disabled by ALTER. Better to say that it "doesn't disable any even triggers". -- Justin
> On 2 Apr 2023, at 21:48, Justin Pryzby <pryzby@telsasoft.com> wrote: > + gettext_noop("Disable event triggers for the duration of the session."), > > Why does is it say "for the duration of the session" ? > > It's possible to disable ignoring, and within the same session. > GUCs are typically "for the duration of the session" .. but they don't > say so (and don't need to). > > + elog(ERROR, "unsupport event trigger: %d", event); > > typo: unsupported > > + Allows to temporarily disable event triggers from executing in order > > => Allow temporarily disabling execution of event triggers .. > > + to troubleshoot and repair faulty event triggers. The value matches > + the type of event trigger to be ignored: > + <literal>ddl_command_start</literal>, <literal>ddl_command_end</literal>, > + <literal>table_rewrite</literal> and <literal>sql_drop</literal>. > + Additionally, all event triggers can be disabled by setting it to > + <literal>all</literal>. Setting the value to <literal>none</literal> > + will ensure that all event triggers are enabled, this is the default > > It doesn't technically "ensure that they're enabled", since they can be > disabled by ALTER. Better to say that it "doesn't disable any even triggers". All comments above addressed in the attached v5, thanks for review! -- Daniel Gustafsson
Вложения
On Mon, Apr 3, 2023 at 8:46 AM Daniel Gustafsson <daniel@yesql.se> wrote: > All comments above addressed in the attached v5, thanks for review! I continue to think it's odd that the sense of this is inverted as compared with row_security. -- Robert Haas EDB: http://www.enterprisedb.com
> On 3 Apr 2023, at 15:09, Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Apr 3, 2023 at 8:46 AM Daniel Gustafsson <daniel@yesql.se> wrote: >> All comments above addressed in the attached v5, thanks for review! > > I continue to think it's odd that the sense of this is inverted as > compared with row_security. I'm not sure I follow. Do you propose that the GUC enables classes of event triggers, the default being "all" (or similar) and one would remove the type of EVT for which debugging is needed? That doesn't seem like a bad idea, just one that hasn't come up in the discussion (and I didn't think about). -- Daniel Gustafsson
On Mon, Apr 3, 2023 at 9:15 AM Daniel Gustafsson <daniel@yesql.se> wrote: > > On 3 Apr 2023, at 15:09, Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Apr 3, 2023 at 8:46 AM Daniel Gustafsson <daniel@yesql.se> wrote: > >> All comments above addressed in the attached v5, thanks for review! > > > > I continue to think it's odd that the sense of this is inverted as > > compared with row_security. > > I'm not sure I follow. Do you propose that the GUC enables classes of event > triggers, the default being "all" (or similar) and one would remove the type of > EVT for which debugging is needed? That doesn't seem like a bad idea, just one > that hasn't come up in the discussion (and I didn't think about). Right. Although to be fair, that idea doesn't sound as good if we're going to have settings other than "on" or "off". If this is just disable_event_triggers = on | off, then why not flip the sense around and make it event_triggers = off | on, just as we do for row_security? But if we're going to allow specific types of event triggers to be disabled, and we think it's likely that people will want to disable one specific type of event trigger while leaving the others alone, that might not be very convenient, because you could end up having to list all the things you do want instead of the one thing you don't want. On the third hand, in other contexts, I've often advocating for giving options a positive sense (what are we going to do?) rather than a negative sense (what are we not going to do?). For example, the TIMING option to EXPLAIN was originally proposed with a name like DISABLE_TIMING or something, and the value inverted, and I said let's not do that. And similarly in other places. A case where I didn't follow that principle is VACUUM (DISABLE_PAGE_SKIPPING) which now seems like a wart to me. Why isn't it VACUUM (PAGE_SKIPPING) again with the opposite value? I'm not sure what the best thing to do is here, I just think it deserves some thought. -- Robert Haas EDB: http://www.enterprisedb.com
> On 3 Apr 2023, at 16:09, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Apr 3, 2023 at 9:15 AM Daniel Gustafsson <daniel@yesql.se> wrote: >>> On 3 Apr 2023, at 15:09, Robert Haas <robertmhaas@gmail.com> wrote: >>> I continue to think it's odd that the sense of this is inverted as >>> compared with row_security. >> >> I'm not sure I follow. Do you propose that the GUC enables classes of event >> triggers, the default being "all" (or similar) and one would remove the type of >> EVT for which debugging is needed? That doesn't seem like a bad idea, just one >> that hasn't come up in the discussion (and I didn't think about). > > Right. Although to be fair, that idea doesn't sound as good if we're > going to have settings other than "on" or "off". Yeah. The patch as it stands allow for disabling specific types rather than all-or-nothing, which is why the name was "ignore". > I'm not sure what the best thing to do is here, I just think it > deserves some thought. Absolutely, the discussion is much appreciated. Having done some thinking I think I'm still partial to framing it as a disabling GUC rather than an enabling; with the act of setting it being "As an admin I want to skip execution of all evt's of type X". -- Daniel Gustafsson
On Mon, Apr 03, 2023 at 11:35:14PM +0200, Daniel Gustafsson wrote: > Yeah. The patch as it stands allow for disabling specific types rather than > all-or-nothing, which is why the name was "ignore". FWIW, I agree with Robert's points here: - disable_event_triggers or ignore_event_triggers = off leads to a double-negative meaning, which is a positive. Depending on one's native language that can be confusing. - Being able to write a list of event triggers working would be much more interesting than just individual elements. - There may be an argument for negated patterns? Say, a "!sql_drop,!ddl_command_start" would cause sql_drop and ddl_command_start to be disabled with all the others enabled, and one should not ne able to mix negated and non-negated patterns. A few days before the end of the commit fest, perhaps you'd better head towards having only an event_trigger = on | off or all | none and consider expanding that later on? From what I get at the top of the thread, this would satisfy the main use case you seemed to worry about to begin with. -- Michael
Вложения
> On 5 Apr 2023, at 10:10, Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Apr 03, 2023 at 11:35:14PM +0200, Daniel Gustafsson wrote: >> Yeah. The patch as it stands allow for disabling specific types rather than >> all-or-nothing, which is why the name was "ignore". > > FWIW, I agree with Robert's points here: > - disable_event_triggers or ignore_event_triggers = off leads to a > double-negative meaning, which is a positive. Depending on one's > native language that can be confusing. I agree that ignore=off would be suboptimal, but the patch doesn't have that but instead ignore_event_trigger=none for that case, which I personally don't think carries the same issue. > - Being able to write a list of event triggers working would be much > more interesting than just individual elements. > - There may be an argument for negated patterns? Say, > a "!sql_drop,!ddl_command_start" would cause sql_drop and > ddl_command_start to be disabled with all the others enabled, and one > should not ne able to mix negated and non-negated patterns. I'm not convinced that it's in our interest to offer a GUC to configure the cluster by selectively turning off SQL features. The ones we have for planner tuning which is a different beast. At the very least it should be in a thread covering that topic, as it might be a bit hidden here. The use case envisioned here is to allow an admin to log in to a database with a broken EVT without having to use single user mode. Basically, it should be a convenient way of temporarily halting the execution of buggy code, not a vehicle for permanent cluster config (even though in light of the above para it's clear that it can be misused like that). Maybe there should be a log event highlighting that the cluster is running with an EVT type ignored? And/or logging the names of the EVT's that otherwise would've been executed? > A few days before the end of the commit fest, perhaps you'd better > head towards having only an event_trigger = on | off or all | none and > consider expanding that later on? From what I get at the top of the > thread, this would satisfy the main use case you seemed to worry > about to begin with. If there are concerns with any part of the patch at this point, and the comments above indicate that, I'd say it's better to push this to the v17 cycle. -- Daniel Gustafsson
On Wed, Apr 5, 2023 at 4:57 AM Daniel Gustafsson <daniel@yesql.se> wrote: > > - Being able to write a list of event triggers working would be much > > more interesting than just individual elements. > > - There may be an argument for negated patterns? Say, > > a "!sql_drop,!ddl_command_start" would cause sql_drop and > > ddl_command_start to be disabled with all the others enabled, and one > > should not ne able to mix negated and non-negated patterns. > > I'm not convinced that it's in our interest to offer a GUC to configure the > cluster by selectively turning off SQL features. The ones we have for planner > tuning which is a different beast. At the very least it should be in a thread > covering that topic, as it might be a bit hidden here. But, isn't that exactly what you're proposing? I mean if this was just event_triggers = on | off it would be exactly like row_security and as far as I'm concerned there would be nothing to debate. But it sounded like you wanted something finer-grained that could disable certain kinds of event triggers. That's also what MIchael is proposing, just with different syntax. In other words, where you would say ignore_event_triggers = sql_drop, he'd say event_triggers = !sql_drop. Maybe we should back up and ask why we need more than "on" and "off". If somebody is using this feature in any form more than very occasionally, they should really go home and reconsider their database schema. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > Maybe we should back up and ask why we need more than "on" and "off". > If somebody is using this feature in any form more than very > occasionally, they should really go home and reconsider their database > schema. +1 ... this seems perhaps overdesigned. regards, tom lane
> On 5 Apr 2023, at 16:30, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Apr 5, 2023 at 4:57 AM Daniel Gustafsson <daniel@yesql.se> wrote: >>> - Being able to write a list of event triggers working would be much >>> more interesting than just individual elements. >>> - There may be an argument for negated patterns? Say, >>> a "!sql_drop,!ddl_command_start" would cause sql_drop and >>> ddl_command_start to be disabled with all the others enabled, and one >>> should not ne able to mix negated and non-negated patterns. >> >> I'm not convinced that it's in our interest to offer a GUC to configure the >> cluster by selectively turning off SQL features. The ones we have for planner >> tuning which is a different beast. At the very least it should be in a thread >> covering that topic, as it might be a bit hidden here. > > But, isn't that exactly what you're proposing? Yeah, but it didn't really strike me until after typing and sending that email. > I mean if this was just event_triggers = on | off it would be exactly > like row_security and as far as I'm concerned there would be nothing > to debate. Which is what v1-v3 did until I changed it based on review input, but I agree with the reasoning here so will revert back (with some internal changes too). Moving this to the next CF for another stab at it when the tree re-opens. -- Daniel Gustafsson
On Wed, Apr 05, 2023 at 10:43:23AM -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Maybe we should back up and ask why we need more than "on" and "off". >> If somebody is using this feature in any form more than very >> occasionally, they should really go home and reconsider their database >> schema. > > +1 ... this seems perhaps overdesigned. Yes. If you begin with an "on"/"off" switch, it could always be extended later if someone makes a case for it, with a grammar like one I mentioned upthread, or even something else. If there is no strong case for more than a boolean for now, simpler is better. -- Michael
Вложения
> On 6 Apr 2023, at 00:06, Michael Paquier <michael@paquier.xyz> wrote: > If there is no strong > case for more than a boolean for now, simpler is better. The attached version of the patch replaces it with a boolean flag for turning off all event triggers, and I also renamed it to the debug_xxx "GUC namespace" which seemed more appropriate. -- Daniel Gustafsson
Вложения
On Tue, Sep 5, 2023 at 8:12 AM Daniel Gustafsson <daniel@yesql.se> wrote: > > On 6 Apr 2023, at 00:06, Michael Paquier <michael@paquier.xyz> wrote: > > If there is no strong > > case for more than a boolean for now, simpler is better. > > The attached version of the patch replaces it with a boolean flag for turning > off all event triggers, and I also renamed it to the debug_xxx "GUC namespace" > which seemed more appropriate. I don't care for the debug_xxx renaming, myself. I think that should be reserved for things where we believe there's no reason to ever use it in production/real life, or for things whose purpose is to emit debugging messages. Neither is the case here. -- Robert Haas EDB: http://www.enterprisedb.com
> On 5 Sep 2023, at 17:29, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Sep 5, 2023 at 8:12 AM Daniel Gustafsson <daniel@yesql.se> wrote: >> The attached version of the patch replaces it with a boolean flag for turning >> off all event triggers, and I also renamed it to the debug_xxx "GUC namespace" >> which seemed more appropriate. > > I don't care for the debug_xxx renaming, myself. I think that should > be reserved for things where we believe there's no reason to ever use > it in production/real life, or for things whose purpose is to emit > debugging messages. Neither is the case here. Fair enough, how about disable_event_trigger instead as per the attached? -- Daniel Gustafsson
Вложения
On Wed, Sep 6, 2023 at 4:50 AM Daniel Gustafsson <daniel@yesql.se> wrote: > > On 5 Sep 2023, at 17:29, Robert Haas <robertmhaas@gmail.com> wrote: > > On Tue, Sep 5, 2023 at 8:12 AM Daniel Gustafsson <daniel@yesql.se> wrote: > > >> The attached version of the patch replaces it with a boolean flag for turning > >> off all event triggers, and I also renamed it to the debug_xxx "GUC namespace" > >> which seemed more appropriate. > > > > I don't care for the debug_xxx renaming, myself. I think that should > > be reserved for things where we believe there's no reason to ever use > > it in production/real life, or for things whose purpose is to emit > > debugging messages. Neither is the case here. > > Fair enough, how about disable_event_trigger instead as per the attached? I usually prefer to give things a positive sense, talking about whether things are enabled rather than disabled. I'd do event_triggers = off | on, like we have for row_security. YMMV, though. -- Robert Haas EDB: http://www.enterprisedb.com
> On 6 Sep 2023, at 16:22, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Sep 6, 2023 at 4:50 AM Daniel Gustafsson <daniel@yesql.se> wrote: >> Fair enough, how about disable_event_trigger instead as per the attached? > > I usually prefer to give things a positive sense, talking about > whether things are enabled rather than disabled. I'd do event_triggers > = off | on, like we have for row_security. YMMV, though. Fair enough, I don't have strong opinions and I do agree that making this work like row_security is a good thing for consistency. Done in the attached. -- Daniel Gustafsson
Вложения
On Wed, Sep 06, 2023 at 10:23:55PM +0200, Daniel Gustafsson wrote: > > On 6 Sep 2023, at 16:22, Robert Haas <robertmhaas@gmail.com> wrote: >> I usually prefer to give things a positive sense, talking about >> whether things are enabled rather than disabled. I'd do event_triggers >> = off | on, like we have for row_security. YMMV, though. > > Fair enough, I don't have strong opinions and I do agree that making this work > like row_security is a good thing for consistency. Done in the attached. This point has been raised a couple of months ago: https://www.postgresql.org/message-id/ZC0s%2BBRMqRupDInQ%40paquier.xyz +SET event_triggers = 'on'; +CREATE POLICY pguc ON event_trigger_test USING (FALSE); +DROP POLICY pguc ON event_trigger_test; This provides checks for the start, end and drop events. Shouldn't table_rewrite also be covered? + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE I am a bit surprised by these two additions. Setting this GUC at file-level can be useful, as is documenting it in the control file if it provides some control of how a statement behaves, no? + Allow temporarily disabling execution of event triggers in order to + troubleshoot and repair faulty event triggers. All event triggers will + be disabled by setting it to <literal>true</literal>. Setting the value + to <literal>false</literal> will not disable any event triggers, this + is the default value. Only superusers and users with the appropriate + <literal>SET</literal> privilege can change this setting. Event triggers are disabled if setting this GUC to false, while true, the default, allows event triggers. The values are reversed in this description. -- Michael
Вложения
On Thu, Sep 7, 2023 at 1:57 AM Michael Paquier <michael@paquier.xyz> wrote: > + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE > > I am a bit surprised by these two additions. Setting this GUC at > file-level can be useful, as is documenting it in the control file if > it provides some control of how a statement behaves, no? Yeah, I don't think these options should be used. > + Allow temporarily disabling execution of event triggers in order to > + troubleshoot and repair faulty event triggers. All event triggers will > + be disabled by setting it to <literal>true</literal>. Setting the value > + to <literal>false</literal> will not disable any event triggers, this > + is the default value. Only superusers and users with the appropriate > + <literal>SET</literal> privilege can change this setting. > > Event triggers are disabled if setting this GUC to false, while true, > the default, allows event triggers. The values are reversed in this > description. Woops. -- Robert Haas EDB: http://www.enterprisedb.com
> On 7 Sep 2023, at 21:02, Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, Sep 7, 2023 at 1:57 AM Michael Paquier <michael@paquier.xyz> wrote: >> + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE >> >> I am a bit surprised by these two additions. Setting this GUC at >> file-level can be useful, as is documenting it in the control file if >> it provides some control of how a statement behaves, no? > > Yeah, I don't think these options should be used. Removed. >> + Allow temporarily disabling execution of event triggers in order to >> + troubleshoot and repair faulty event triggers. All event triggers will >> + be disabled by setting it to <literal>true</literal>. Setting the value >> + to <literal>false</literal> will not disable any event triggers, this >> + is the default value. Only superusers and users with the appropriate >> + <literal>SET</literal> privilege can change this setting. >> >> Event triggers are disabled if setting this GUC to false, while true, >> the default, allows event triggers. The values are reversed in this >> description. > > Woops. Fixed. Since the main driver for this is to reduce the usage/need for single-user mode I also reworded the patch slightly. Instead of phrasing this as an alternative to single-user mode, I reversed it such that single-user mode is an alternative to this GUC. -- Daniel Gustafsson
Вложения
On Fri, Sep 22, 2023 at 05:29:01PM +0200, Daniel Gustafsson wrote: > Since the main driver for this is to reduce the usage/need for single-user mode > I also reworded the patch slightly. Instead of phrasing this as an alternative > to single-user mode, I reversed it such that single-user mode is an alternative > to this GUC. Okay. + be disabled by setting it to <literal>false</literal>. Setting the value + to <literal>true</literal> will not disable any event triggers, this This uses a double negation. Perhaps just "Setting this value to true allows all event triggers to run." 003_check_guc.pl has detected a failure because event_triggers is missing in postgresql.conf.sample while it is not marked with GUC_NOT_IN_SAMPLE anymore. Keeping the docs consistent with the sample file, I would suggest the attached on top of your v9. -- Michael
Вложения
> On 25 Sep 2023, at 01:35, Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Sep 22, 2023 at 05:29:01PM +0200, Daniel Gustafsson wrote: >> Since the main driver for this is to reduce the usage/need for single-user mode >> I also reworded the patch slightly. Instead of phrasing this as an alternative >> to single-user mode, I reversed it such that single-user mode is an alternative >> to this GUC. > > Okay. > > + be disabled by setting it to <literal>false</literal>. Setting the value > + to <literal>true</literal> will not disable any event triggers, this > > This uses a double negation. Perhaps just "Setting this value to true > allows all event triggers to run." Fair enough, although I used "fire" instead of "run" which is consistent with the event trigger documentation. > 003_check_guc.pl has detected a failure because event_triggers is > missing in postgresql.conf.sample while it is not marked with > GUC_NOT_IN_SAMPLE anymore. > > Keeping the docs consistent with the sample file, I would suggest the > attached on top of your v9. Ah, yes. The attached v10 has the above to fixes. -- Daniel Gustafsson
Вложения
On Mon, Sep 25, 2023 at 09:19:35AM +0200, Daniel Gustafsson wrote: > Fair enough, although I used "fire" instead of "run" which is consistent with > the event trigger documentation. Okay by me. -- Michael
Вложения
> On 25 Sep 2023, at 09:50, Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Sep 25, 2023 at 09:19:35AM +0200, Daniel Gustafsson wrote: >> Fair enough, although I used "fire" instead of "run" which is consistent with >> the event trigger documentation. > > Okay by me. Great, I'll go ahead and apply this version then. Thanks for review! -- Daniel Gustafsson
> On 25 Sep 2023, at 09:52, Daniel Gustafsson <daniel@yesql.se> wrote: > >> On 25 Sep 2023, at 09:50, Michael Paquier <michael@paquier.xyz> wrote: >> >> On Mon, Sep 25, 2023 at 09:19:35AM +0200, Daniel Gustafsson wrote: >>> Fair enough, although I used "fire" instead of "run" which is consistent with >>> the event trigger documentation. >> >> Okay by me. > > Great, I'll go ahead and apply this version then. Thanks for review! And applied, closing the CF entry. -- Daniel Gustafsson