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

> On 15 Feb 2022, at 11:07, Greg Nancarrow <gregn4422@gmail.com> wrote:

> I've attached a re-based version (no functional changes from the
> previous) to fix cfbot failures.

Thanks for adopting this patch, I took another look at it today and have some
comments.

+        This flag is used internally by <productname>PostgreSQL</productname> and should not be manually changed by
DBAor application. 
I think we should reword this to something like "This flag is used internally
by <productname>PostgreSQL</productname> and should not be altered or relied
upon for monitoring".  We really don't want anyone touching or interpreting
this value since there is no guarantee that it will be accurate.


+    new_record[Anum_pg_database_dathasloginevt - 1] = BoolGetDatum(datform->dathasloginevt);
Since the corresponding new_record_repl valus is false, this won't do anything
and can be removed.  We will use the old value anyways.


+    if (strcmp(eventname, "login") == 0)
I think this block warrants a comment on why it only applies to login triggers.


+        db = (Form_pg_database) GETSTRUCT(tuple);
+        if (!db->dathasloginevt)
+        {
+            db->dathasloginevt = true;
+            CatalogTupleUpdate(pg_db, &tuple->t_self, tuple);
+        }
+        else
+            CacheInvalidateRelcacheByTuple(tuple);
This needs a CommandCounterIncrement inside the if () { .. } block right?


+    /* Get the command tag. */
+    tag = (event == EVT_Login) ? CMDTAG_LOGIN : CreateCommandTag(parsetree);
To match project style I think this should be an if-then-else block.  Also,
while it's tempting to move this before the assertion block in order to reuse
it there, it does mean that we are calling this in a hot codepath before we
know if it's required.  I think we should restore the previous programming with
a separate CreateCommandTag call inside the assertion block and move this one
back underneath the fast-path exit.


+                    CacheInvalidateRelcacheByTuple(tuple);
+            }
+            table_close(pg_db, RowExclusiveLock);
+            heap_freetuple(tuple);
+        }
+    }
+    CommitTransactionCommand();
Since we are commiting the transaction just after closing the table, is the
relcache invalidation going to achieve much?  I guess registering the event
doesn't hurt much?


+        /*
+         * There can be a race condition: a login event trigger may
+         * have been added after the pg_event_trigger table was
+         * scanned, and we don't want to erroneously clear the
+         * dathasloginevt flag in this case. To be sure that this
+         * hasn't happened, repeat the scan under the pg_database
+         * table lock.
+         */
+        AcceptInvalidationMessages();
+        runlist = EventTriggerCommonSetup(NULL,
+                          EVT_Login, "login",
+                          &trigdata);
It seems conceptually wrong to allocate this in the TopTransactionContext when
we only generate the runlist to throw it away.  At the very least I think we
should free the returned list.


+    if (runlist == NULL)    /* list is still empty, so clear the
This should check for NIL and not NULL.


Have you done any benchmarking on this patch?  With this version I see a small
slowdown on connection time of ~1.5% using pgbench for the case where there are
no login event triggers defined.  With a login event trigger calling an empty
plpgsql function there is a ~30% slowdown (corresponding to ~1ms on my system).
When there is a login event trigger defined all bets are off however, since the
function called can be arbitrarily complex and it's up to the user to measure
and decide - but the bare overhead we impose is still of interest of course.

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