Re: On login trigger: take three

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Greg Nancarrow <gregn4422(at)gmail(dot)com>
Cc: Ivan Panchenko <wao(at)mail(dot)ru>, Teodor Sigaev <teodor(at)sigaev(dot)ru>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Subject: Re: On login trigger: take three
Date: 2022-03-11 11:22:10
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

> 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

+ 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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Nitin Jadhav 2022-03-11 11:29:11 Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)
Previous Message PG Bug reporting form 2022-03-11 11:11:54 BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands