Re: On login trigger: take three

Hi,

On 2022-03-13 23:31:03 +0100, Daniel Gustafsson wrote:
> > On 12 Mar 2022, at 03:46, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> >> + <para>
> >> + The <literal>login</literal> event occurs when a user logs in to the
> >> + system.
> >> + Any bugs in a trigger procedure for this event may prevent successful
> >> + login to the system. Such bugs may be fixed after first restarting the
> >> + system in single-user mode (as event triggers are disabled in this mode).
> >> + See the <xref linkend="app-postgres"/> reference page for details about
> >> + using single-user mode.
> >> + </para>
> >
> > I'm strongly against adding any new dependencies on single user mode.
> >
> > A saner approach might be a superuser-only GUC that can be set as part of the
> > connection data (e.g. PGOPTIONS='-c ignore_login_event_trigger=true').
>
> This, and similar approaches, has been discussed in this thread. I'm not a fan
> of holes punched with GUC's like this, but if you plan on removing single-user
> mode (as I recall seeing in an initdb thread) altogether then that kills the
> discussion. So.

Even if we end up not removing single user mode, I think it's not OK to add
new failure modes that require single user mode to resolve after not-absurd
operations (I'm ok with needing single user mode if somebody does delete from
pg_authid). It's too hard to reach for most.

We already have GUCs like row_security, so it doesn't seem insane to add one
that disables login event triggers. What is the danger that you see?

> Since we already recommend single-user mode to handle broken event triggers, we
> should IMO do something to cover all of them rather than risk ending up with
> one disabling GUC per each event type. Right now we have this on the CREATE
> EVENT TRIGGER reference page:

IMO the other types of event triggers make it a heck of a lot harder to get
yourself into a situation that you can't get out of...

> "Event triggers are disabled in single-user mode (see postgres). If an
> erroneous event trigger disables the database so much that you can't even
> drop the trigger, restart in single-user mode and you'll be able to do
> that."
> Something like a '-c ignore_event_trigger=<event|all>' GUC perhaps?

Did you mean login instead of event?

Something like it would work for me. It probably should be
GUC_DISALLOW_IN_FILE?

Greetings,

Andres Freund