Re: On login trigger: take three

On Wed, Sep 8, 2021 at 10:56 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> I took a look at this, and while I like the proposed feature I think the patch
> has a bit more work required.
>

Thanks for reviewing the patch.
I am not the original patch author (who no longer seems active) but
I've been contributing a bit and keeping the patch alive because I
think it's a worthwhile feature.

>
> +
> +-- 2) Initialize some user session data
> +   CREATE TEMP TABLE session_storage (x float, y integer);
> The example in the documentation use a mix of indentations, neither of which is
> the 2-space indentation used elsewhere in the docs.
>

Fixed, using 2-space indentation.
(to be honest, the indentation seems inconsistent elsewhere in the
docs e.g. I'm seeing a nearby case of 2-space on 1st indent, 6-space
on 2nd indent)

>
> +       /* Get the command tag. */
> +       tag = parsetree ? CreateCommandTag(parsetree) : CMDTAG_CONNECT;
> This is hardcoding knowledge that currently only this trigger wont have a
> parsetree, and setting the tag accordingly.  This should instead check the
> event and set CMDTAG_UNKNOWN if it isn't the expected one.
>

I updated that, but maybe not exactly how you expected?

>
> +       /* database has on-login triggers */
> +       bool            dathaslogontriggers;
> This patch uses three different names for the same thing: client connection,
> logontrigger and login trigger.  Since this trigger only fires after successful
> authentication it’s not accurate to name it client connection, as that implies
> it running on connections rather than logins.  The nomenclature used in the
> tree is "login" so the patch should be adjusted everywhere to use that.
>

Fixed.

>
> +/* Hook for plugins to get control at start of session */
> +client_connection_hook_type client_connection_hook = EventTriggerOnConnect;
> I don't see the reason for adding core functionality by hooks.  Having a hook
> might be a useful thing on its own (to be discussed in a new thread, not hidden
> here), but core functionality should not depend on it IMO.
>

Fair enough, I removed the hook.

>
> +       {"enable_client_connection_trigger", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
> +               gettext_noop("Enables the client_connection event trigger."),
> +               gettext_noop("In case of errors in the ON client_connection EVENT TRIGGER procedure, "
>  ..and..
> +       /*
> +        * Try to ignore error for superuser to make it possible to login even in case of errors
> +        * during trigger execution
> +        */
> +       if (!is_superuser)
> +               PG_RE_THROW();
> This patch adds two ways for superusers to bypass this event trigger in case of
> it being faulty, but for every other event trigger we've documented to restart
> in single-user mode and fixing it there.  Why does this need to be different?
> This clearly has a bigger chance of being a footgun but I don't see that as a
> reason to add a GUC and a bypass that other footguns lack.
>

OK, I removed those bypasses. We'll see what others think.

>
> +       elog(NOTICE, "client_connection trigger failed with message: %s", error->message);
> Calling elog() in a PG_CATCH() block isn't allowed is it?
>

I believe it is allowed (e.g. there's a case in libpq), but I removed
this anyway as part of the superuser bypass.

>
> +       /* Runtlist is empty: clear dathaslogontriggers flag
> +        */
> s/Runtlist/Runlist/ and also commenting style.
>

Fixed.

>
> The logic for resetting the pg_database flag when firing event trigger in case
> the trigger has gone away is messy and a problem waiting to happen IMO.  For
> normal triggers we don't bother with that on the grounds of it being racy, and
> instead perform relhastrigger removal during VACUUM.  Another approach is using
> a counter as propose upthread, since updating that can be made safer.  The
> current coding also isn't instructing concurrent backends to perform relcache
> rebuild.
>

I think there are pros and cons of each possible approach, but I think
I prefer the current way (which I have tweaked a bit) for similar
reasons to those explained by the original patch author when debating
whether to use reference-counting instead, in the discussion upthread
(e.g. it keeps it all in one place). Also, it seems to be more inline
with the documented reason why that pg_database flag was added in the
first place. I have debugged a few concurrent scenarios with the
current mechanism in place. If you still dislike the logic for
resetting the flag, please elaborate on the issues you foresee and one
of the alternative approaches can be tried.

I've attached the updated patch.

Regards,
Greg Nancarrow
Fujitsu Australia