GUC for temporarily disabling event triggers

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]http://cfbot.cputube.org/patch_41_4013.log, 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,

`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@gmail.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@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 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@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