Re: On login trigger: take three

> On 15 Feb 2022, at 11:07, Greg Nancarrow <gregn4422(at)gmail(dot)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 DBA or 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/