Re: On login trigger: take three - Mailing list pgsql-hackers

> On 19 Jul 2021, at 15:25, Greg Nancarrow <gregn4422@gmail.com> wrote:

> Attached a rebased patch (minor updates to the test code).

I took a look at this, and while I like the proposed feature I think the patch
has a bit more work required.


+    END IF;
+
+-- 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.


+    /* 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.


+    /* 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.


+/* 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.


+    {"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.


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


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


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.

--
Daniel Gustafsson        https://vmware.com/