Re: On login trigger: take three

On 22.12.2020 21:19, Pavel Stehule wrote:
>
>
> út 22. 12. 2020 v 12:42 odesílatel Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru <mailto:k(dot)knizhnik(at)postgrespro(dot)ru>> napsal:
>
>
>
> On 22.12.2020 12:25, Pavel Stehule wrote:
>>
>> regress tests fails
>>
>>      sysviews                     ... FAILED    112 ms
>> test event_trigger                ... FAILED (test process exited
>> with exit code 2)      447 ms
>> test fast_default                 ... FAILED  392 ms
>> test stats                        ... FAILED  626 ms
>> ============== shutting down postmaster     ==============
>>
>
> Sorry, fixed.
>
>
> no problem
>
> I had to fix doc
>
> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index 6ff783273f..7aded1848f 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -1008,7 +1008,7 @@ include_dir 'conf.d'
>          trigger when a client connects. This parameter is switched on
> by default.
>          Errors in trigger code can prevent user to login to the system.
>          I this case disabling this parameter in connection string can
> solve the problem:
> -        <literal>psql "dbname=postgres options='-c
> enable_client_connection_trigger=false'".
> +        <literal>psql "dbname=postgres options='-c
> enable_client_connection_trigger=false'"</literal>.
>         </para>
>        </listitem>
>       </varlistentry>
>
> I am thinking again about enable_client_connection_trigger, and
> although it can look useless (because error is ignored for superuser),
> it can be useful for some debugging and administration purposes.
> Probably we don't want to start the client_connection trigger from
> backup tools, maybe from some monitoring tools. Maybe the possibility
> to set this GUC can be dedicated to some special role (like
> pg_signal_backend).
>
> +     <varlistentry id="guc-enable-client-connection-trigger"
> xreflabel="enable_client_connection_trigger">
> +  <term><varname>enable_client_connection_trigger</varname>
> (<type>boolean</type>)
> +      <indexterm>
> + <primary><varname>enable_client_connection_trigger</varname>
> configuration parameter</primary>
> +      </indexterm>
> +      </term>
> +      <listitem>
> +       <para>
> +        Enables firing the <literal>client_connection</literal>
> +        trigger when a client connects. This parameter is switched on
> by default.
> +        Errors in trigger code can prevent user to login to the system.
> +        I this case disabling this parameter in connection string can
> solve the problem:
> +        <literal>psql "dbname=postgres options='-c
> enable_client_connection_trigger=false'"</literal>.
> +       </para>
> +      </listitem>
> +     </varlistentry>
>
> There should be note, so only superuser can change this value
>
> There is should be tab-complete support
>
> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> index 3a43c09bf6..08f00d8fc4 100644
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -2970,7 +2970,8 @@ psql_completion(const char *text, int start, int
> end)
>         COMPLETE_WITH("ON");
>     /* Complete CREATE EVENT TRIGGER <name> ON with event_type */
>     else if (Matches("CREATE", "EVENT", "TRIGGER", MatchAny, "ON"))
> -       COMPLETE_WITH("ddl_command_start", "ddl_command_end", "sql_drop");
> +       COMPLETE_WITH("ddl_command_start", "ddl_command_end",
> +                     "client_connection", "sql_drop");
>
>     /*
>      * Complete CREATE EVENT TRIGGER <name> ON <event_type>.  EXECUTE
> FUNCTION

Thank you.
I have applied all your fixes in on_connect_event_trigger-12.patch.

Concerning enable_client_connection_trigger GUC, I think that it is
really useful: it is the fastest and simplest way to disable login
triggers in case
of some problems with them (not only for superuser itself, but for all
users). Yes, it can be also done using "ALTER EVENT TRIGGER DISABLE".
But assume that you have a lot of databases with different login
policies enforced by on-login event triggers. And you want temporary
disable them all, for example for testing purposes.
In this case GUC is most convenient way to do it.

Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company