Subject: Re: GUC for temporarily disabling event triggers

From: Justin Pryzby <[email-masked]>
Date: Sun, 2 Apr 2023 14:48:52 -0500

On Mon, Mar 06, 2023 at 01:24:56PM +0100, Daniel Gustafsson wrote:
> > On 27 Jan 2023, at 15:00, Mikhail Gribkov <[email-masked]> 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

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 3 Apr 2023 14:45:53 +0200

> On 2 Apr 2023, at 21:48, Justin Pryzby <[email-masked]> wrote:

> 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

---

From: Robert Haas <[email-masked]>
Date: Mon, 3 Apr 2023 09:09:18 -0400

> 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

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 3 Apr 2023 15:15:16 +0200

> On 3 Apr 2023, at 15:09, Robert Haas <[email-masked]> wrote:
> 

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

---

From: Robert Haas <[email-masked]>
Date: Mon, 3 Apr 2023 10:09:01 -0400

> 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.

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 3 Apr 2023 23:35:14 +0200

> On 3 Apr 2023, at 16:09, Robert Haas <[email-masked]> wrote:

> 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

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

---

From: Michael Paquier <[email-masked]>
Date: Wed, 5 Apr 2023 17:10:32 +0900

> 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

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 5 Apr 2023 10:57:18 +0200

> On 5 Apr 2023, at 10:10, Michael Paquier <[email-masked]> wrote:
> 

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.

> 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?

> thread, this would satisfy the main use case you seemed to worry

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

---

From: Robert Haas <[email-masked]>
Date: Wed, 5 Apr 2023 10:30:56 -0400

> 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.

---

From: Tom Lane <[email-masked]>
Date: Wed, 05 Apr 2023 10:43:23 -0400

> occasionally, they should really go home and reconsider their database

+1 ... this seems perhaps overdesigned.

			regards, tom lane