Re: 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(at)yesql(dot)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(at)yesql(dot)se> wrote:
>
> > On 3 Nov 2022, at 21:47, Daniel Gustafsson <daniel(at)yesql(dot)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(at)gmail(dot)com> wrote:
>
> On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>
>>> On 3 Nov 2022, at 21:47, Daniel Gustafsson <daniel(at)yesql(dot)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(at)yesql(dot)se> wrote:

> > On 11 Jan 2023, at 17:38, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >>
> >>> On 3 Nov 2022, at 21:47, Daniel Gustafsson <daniel(at)yesql(dot)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,

`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

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

> On 27 Jan 2023, at 15:00, Mikhail Gribkov <youzhick(at)gmail(dot)com> 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

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

> On 7 Mar 2023, at 16:02, Mikhail Gribkov <youzhick(at)gmail(dot)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(at)gmail(dot)com> 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:
+ <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(at)telsasoft(dot)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(at)yesql(dot)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(at)gmail(dot)com> wrote:
>
> On Mon, Apr 3, 2023 at 8:46 AM Daniel Gustafsson <daniel(at)yesql(dot)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(at)yesql(dot)se> wrote:
> > On 3 Apr 2023, at 15:09, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Mon, Apr 3, 2023 at 8:46 AM Daniel Gustafsson <daniel(at)yesql(dot)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(at)gmail(dot)com> wrote:
> On Mon, Apr 3, 2023 at 9:15 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>> On 3 Apr 2023, at 15:09, Robert Haas <robertmhaas(at)gmail(dot)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(at)paquier(dot)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(at)yesql(dot)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(at)gmail(dot)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(at)gmail(dot)com> wrote:
>
> On Wed, Apr 5, 2023 at 4:57 AM Daniel Gustafsson <daniel(at)yesql(dot)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(at)gmail(dot)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(at)paquier(dot)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(at)yesql(dot)se> wrote:
> > On 6 Apr 2023, at 00:06, Michael Paquier <michael(at)paquier(dot)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(at)gmail(dot)com> wrote:
> On Tue, Sep 5, 2023 at 8:12 AM Daniel Gustafsson <daniel(at)yesql(dot)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(at)yesql(dot)se> wrote:
> > On 5 Sep 2023, at 17:29, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Tue, Sep 5, 2023 at 8:12 AM Daniel Gustafsson <daniel(at)yesql(dot)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(at)gmail(dot)com> wrote:
> On Wed, Sep 6, 2023 at 4:50 AM Daniel Gustafsson <daniel(at)yesql(dot)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(at)gmail(dot)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:
/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(at)paquier(dot)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(at)gmail(dot)com> wrote:
>
> On Thu, Sep 7, 2023 at 1:57 AM Michael Paquier <michael(at)paquier(dot)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(at)paquier(dot)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(at)paquier(dot)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(at)yesql(dot)se> wrote:
>
>> On 25 Sep 2023, at 09:50, Michael Paquier <michael(at)paquier(dot)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