> 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