Date: Sun, 2 Apr 2023 14:48:52 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Mikhail Gribkov <youzhick@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: GUC for temporarily disabling event triggers
Message-ID: <ZCncJGbFLex2f9GP@telsasoft.com>
References: <9140106E-F9BF-4D85-8FC8-F2D3C094A6D9@yesql.se>
 <CALte62x7c9R0ohKcQSr5N9PFB-D6pCb_THhwWzTscR7ZB6OHvg@mail.gmail.com>
 <167482801126.1114.1095311628314619931.pgcf@coridan.postgresql.org>
 <8D26ACC2-7669-460D-833A-582EAD6CC09E@yesql.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8D26ACC2-7669-460D-833A-582EAD6CC09E@yesql.se>
Archived-At: <https://www.postgresql.org/message-id/ZCncJGbFLex2f9GP%40telsasoft.com>
Precedence: bulk

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