Re: GUC for temporarily disabling event triggers
> On 25 Sep 2023, at 09:52, Daniel Gustafsson wrote: > >> On 25 Sep 2023, at 09:50, Michael Paquier 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
Re: GUC for temporarily disabling event triggers
> On 25 Sep 2023, at 09:50, Michael Paquier 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
Re: GUC for temporarily disabling event triggers
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 signature.asc Description: PGP signature
Re: GUC for temporarily disabling event triggers
> On 25 Sep 2023, at 01:35, Michael Paquier 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 false. Setting the > value > + to true 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 v10-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: GUC for temporarily disabling event triggers
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 false. Setting the value
+ to true 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
From 2ad4989a71c40b7d6de8fcb095b057377cd05bf5 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson
Date: Fri, 22 Sep 2023 16:39:07 +0200
Subject: [PATCH v9 1/2] Add GUC for temporarily disabling event triggers
In order to troubleshoot misbehaving or buggy event triggers, the
documented advice is to enter single-user mode. In an attempt to
reduce the number of situations where single-user mode is required
(or even recommended) for non-extraordinary maintenance, this GUC
allows to temporarily suspend event triggers.
This was originally extracted from a larger patchset which aimed
at supporting event triggers on login events.
Reviewed-by: Ted Yu
Reviewed-by: Mikhail Gribkov
Reviewed-by: Justin Pryzby
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/9140106e-f9bf-4d85-8fc8-f2d3c094a...@yesql.se
---
src/include/commands/event_trigger.h| 2 ++
src/backend/commands/event_trigger.c| 20 +++--
src/backend/utils/misc/guc_tables.c | 11 ++
src/test/regress/expected/event_trigger.out | 22 +++
src/test/regress/sql/event_trigger.sql | 24 +
doc/src/sgml/config.sgml| 19
doc/src/sgml/ref/create_event_trigger.sgml | 9 +---
7 files changed, 97 insertions(+), 10 deletions(-)
diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index 5ed6ece555..1c925dbf25 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -29,6 +29,8 @@ typedef struct EventTriggerData
CommandTag tag;
} EventTriggerData;
+extern PGDLLIMPORT bool event_triggers;
+
#define AT_REWRITE_ALTER_PERSISTENCE 0x01
#define AT_REWRITE_DEFAULT_VAL 0x02
#define AT_REWRITE_COLUMN_REWRITE 0x04
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index d4b00d1a82..bd812e42d9 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -72,6 +72,9 @@ typedef struct EventTriggerQueryState
static EventTriggerQueryState *currentEventTriggerState = NULL;
+/* GUC parameter */
+bool event_triggers = true;
+
/* Support for dropped objects */
typedef struct SQLDropObject
{
@@ -657,8 +660,11 @@ EventTriggerDDLCommandStart(Node *parsetree)
* wherein event triggers are disabled. (Or we could implement
* heapscan-and-sort logic for that case, but having disaster recovery
* scenarios depend on code that's otherwise untested isn't appetizing.)
+ *
+ * Additionally, event triggers can be disabled with a superuser-only GUC
+ * to make fixing database easier as per 1 above.
*/
- if (!IsUnderPostmaster)
+ if (!IsUnderPostmaster || !event_triggers)
return;
runlist = EventTriggerCommonSetup(parsetree,
@@ -692,9 +698,9 @@ EventTriggerDDLCommandEnd(Node *parsetree)
/*
* See EventTriggerDDLCommandStart for a discussion about why event
- * triggers are disabled in single user mode.
+ * triggers are disabled in single user mode or via GUC.
*/
- if (!IsUnderPostmaster)
+ if (!IsUnderPostmaster || !event_triggers)
return;
/*
@@ -740,9 +746,9 @@ EventTriggerSQLDrop(Node *parsetree)
/*
* See EventTriggerDDLCommandStart for a discussion about why event
- * triggers are disabled in single user mode.
+ * triggers are disabled in single user mode or via a GUC.
*/
- if (!IsUnderPostmaster)
+ if (!IsUnderPostmaster || !event_triggers)
return;
/*
@@ -811,9 +817,9 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason)
/*
* See EventTriggerDDLCommandStart for a discussion about why event
- * triggers are disabled in single user mode.
+ * triggers are disabled in single user mode or via a GUC.
*/
- if (!IsUnderPostmaster)
+ if (!IsUnderPostmaster || !event_triggers)
return;
/*
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index bdb26e2b77..16ec6c5ef0 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -37,6 +37,7 @@
#include "catalog/namespace.h"
#includ
Re: GUC for temporarily disabling event triggers
> On 7 Sep 2023, at 21:02, Robert Haas wrote: > > On Thu, Sep 7, 2023 at 1:57 AM Michael Paquier 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 true. Setting the >> value >> + to false will not disable any event triggers, this >> + is the default value. Only superusers and users with the appropriate >> + SET 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 v9-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: GUC for temporarily disabling event triggers
On Thu, Sep 7, 2023 at 1:57 AM Michael Paquier 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 true. Setting the > value > + to false will not disable any event triggers, this > + is the default value. Only superusers and users with the appropriate > + SET 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
Re: GUC for temporarily disabling event triggers
On Wed, Sep 06, 2023 at 10:23:55PM +0200, Daniel Gustafsson wrote: > > On 6 Sep 2023, at 16:22, Robert Haas 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 true. Setting the value + to false will not disable any event triggers, this + is the default value. Only superusers and users with the appropriate + SET 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 signature.asc Description: PGP signature
Re: GUC for temporarily disabling event triggers
> On 6 Sep 2023, at 16:22, Robert Haas wrote: > On Wed, Sep 6, 2023 at 4:50 AM Daniel Gustafsson 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 v8-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: GUC for temporarily disabling event triggers
On Wed, Sep 6, 2023 at 4:50 AM Daniel Gustafsson wrote: > > On 5 Sep 2023, at 17:29, Robert Haas wrote: > > On Tue, Sep 5, 2023 at 8:12 AM Daniel Gustafsson 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
Re: GUC for temporarily disabling event triggers
> On 5 Sep 2023, at 17:29, Robert Haas wrote: > On Tue, Sep 5, 2023 at 8:12 AM Daniel Gustafsson 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 v7-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: GUC for temporarily disabling event triggers
On Tue, Sep 5, 2023 at 8:12 AM Daniel Gustafsson wrote: > > On 6 Apr 2023, at 00:06, Michael Paquier 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
Re: GUC for temporarily disabling event triggers
> On 6 Apr 2023, at 00:06, Michael Paquier 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 v6-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: GUC for temporarily disabling event triggers
On Wed, Apr 05, 2023 at 10:43:23AM -0400, Tom Lane wrote: > Robert Haas 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 signature.asc Description: PGP signature
Re: GUC for temporarily disabling event triggers
> On 5 Apr 2023, at 16:30, Robert Haas wrote: > > On Wed, Apr 5, 2023 at 4:57 AM Daniel Gustafsson 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
Re: GUC for temporarily disabling event triggers
Robert Haas 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
Re: GUC for temporarily disabling event triggers
On Wed, Apr 5, 2023 at 4:57 AM Daniel Gustafsson 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
Re: GUC for temporarily disabling event triggers
> On 5 Apr 2023, at 10:10, Michael Paquier 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
Re: GUC for temporarily disabling event triggers
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 signature.asc Description: PGP signature
Re: GUC for temporarily disabling event triggers
> On 3 Apr 2023, at 16:09, Robert Haas wrote: > On Mon, Apr 3, 2023 at 9:15 AM Daniel Gustafsson wrote: >>> On 3 Apr 2023, at 15:09, Robert Haas 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
Re: GUC for temporarily disabling event triggers
On Mon, Apr 3, 2023 at 9:15 AM Daniel Gustafsson wrote: > > On 3 Apr 2023, at 15:09, Robert Haas wrote: > > On Mon, Apr 3, 2023 at 8:46 AM Daniel Gustafsson 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
Re: GUC for temporarily disabling event triggers
> On 3 Apr 2023, at 15:09, Robert Haas wrote: > > On Mon, Apr 3, 2023 at 8:46 AM Daniel Gustafsson 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
Re: GUC for temporarily disabling event triggers
On Mon, Apr 3, 2023 at 8:46 AM Daniel Gustafsson 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
Re: GUC for temporarily disabling event triggers
> On 2 Apr 2023, at 21:48, Justin Pryzby 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:
> +ddl_command_start,
> ddl_command_end,
> +table_rewrite and sql_drop.
> +Additionally, all event triggers can be disabled by setting it to
> +all. Setting the value to none
> +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
v5-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch
Description: Binary data
Re: GUC for temporarily disabling event triggers
On Mon, Mar 06, 2023 at 01:24:56PM +0100, Daniel Gustafsson wrote:
> > On 27 Jan 2023, at 15:00, Mikhail Gribkov wrote:
>
> > There is a complete framework for disabling various types of the event
> > triggers separately, but, the list of valid GUC values only include 'all'
> > and 'none'. Why not adding support for all the event trigger types
> > separately? Everything is already there in the patch; the only thing needed
> > is expanding couple of enums. It's cheap in terms of code size and even
> > cheaper 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:
+ddl_command_start,
ddl_command_end,
+table_rewrite and sql_drop.
+Additionally, all event triggers can be disabled by setting it to
+all. Setting the value to none
+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
Re: GUC for temporarily disabling event triggers
> On 7 Mar 2023, at 16:02, Mikhail Gribkov 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
Re: GUC for temporarily disabling event triggers
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 for master branch. Thus doesn't seem to be this patch issue. The new status of this patch is: Ready for Committer
Re: GUC for temporarily disabling event triggers
> On 27 Jan 2023, at 15:00, Mikhail Gribkov wrote: > There is a complete framework for disabling various types of the event > triggers separately, but, the list of valid GUC values only include 'all' and > 'none'. Why not adding support for all the event trigger types separately? > Everything is already there in the patch; the only thing needed is expanding > couple of enums. It's cheap in terms of code size and even cheaper 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 v4-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: GUC for temporarily disabling event triggers
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 values only include 'all' and 'none'. Why not adding support for all the event trigger types separately? Everything is already there in the patch; the only thing needed is expanding couple of enums. It's cheap in terms of code size and even cheaper 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
Re: GUC for temporarily disabling event triggers
On Thu, Jan 12, 2023 at 12:26 PM Daniel Gustafsson wrote: > > On 11 Jan 2023, at 17:38, vignesh C wrote: > > > > On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson wrote: > >> > >>> On 3 Nov 2022, at 21:47, Daniel Gustafsson 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, `this GUC allows to temporarily suspending event triggers.` 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
Re: GUC for temporarily disabling event triggers
> On 11 Jan 2023, at 17:38, vignesh C wrote: > > On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson wrote: >> >>> On 3 Nov 2022, at 21:47, Daniel Gustafsson 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/ v3-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: GUC for temporarily disabling event triggers
On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson wrote: > > > On 3 Nov 2022, at 21:47, Daniel Gustafsson 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
Re: GUC for temporarily disabling event triggers
> On 3 Nov 2022, at 21:47, Daniel Gustafsson 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/ v2-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
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/ v1-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
