Re: On login trigger: take three

Hi,

On 2023-10-10 08:18:46 +0300, Alexander Korotkov wrote:
> @@ -968,7 +969,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
>
> if (!get_db_info(dbtemplate, ShareLock,
> &src_dboid, &src_owner, &src_encoding,
> - &src_istemplate, &src_allowconn,
> + &src_istemplate, &src_allowconn, &src_hasloginevt,
> &src_frozenxid, &src_minmxid, &src_deftablespace,
> &src_collate, &src_ctype, &src_iculocale, &src_icurules, &src_locprovider,
> &src_collversion))

This isn't your fault, but this imo has become unreadable. Think we ought to
move the information about a database to a struct.

> @@ -296,6 +306,13 @@ insert_event_trigger_tuple(const char *trigname, const char *eventname, Oid evtO
> CatalogTupleInsert(tgrel, tuple);
> heap_freetuple(tuple);
>
> + /*
> + * Login event triggers have an additional flag in pg_database to allow
> + * faster lookups in hot codepaths. Set the flag unless already True.
> + */
> + if (strcmp(eventname, "login") == 0)
> + SetDatatabaseHasLoginEventTriggers();

It's not really faster lookups, it's no lookups, right?

> /* Depend on owner. */
> recordDependencyOnOwner(EventTriggerRelationId, trigoid, evtOwner);
>
> @@ -357,6 +374,39 @@ filter_list_to_array(List *filterlist)
> return PointerGetDatum(construct_array_builtin(data, l, TEXTOID));
> }
>
> +/*
> + * Set pg_database.dathasloginevt flag for current database indicating that
> + * current database has on login triggers.
> + */
> +void
> +SetDatatabaseHasLoginEventTriggers(void)
> +{
> + /* Set dathasloginevt flag in pg_database */
> + Form_pg_database db;
> + Relation pg_db = table_open(DatabaseRelationId, RowExclusiveLock);
> + HeapTuple tuple;
> +
> + /*
> + * Use shared lock to prevent a conflit with EventTriggerOnLogin() trying
> + * to reset pg_database.dathasloginevt flag. Note that we use
> + * AccessShareLock allowing setters concurently.
> + */
> + LockSharedObject(DatabaseRelationId, MyDatabaseId, 0, AccessShareLock);

That seems like a very odd approach - how does this avoid concurrency issues
with one backend setting and another unsetting the flag? And outside of that,
won't this just lead to concurrently updated tuples?

> + tuple = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId));
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for database %u", MyDatabaseId);
> + db = (Form_pg_database) GETSTRUCT(tuple);
> + if (!db->dathasloginevt)
> + {
> + db->dathasloginevt = true;
> + CatalogTupleUpdate(pg_db, &tuple->t_self, tuple);
> + CommandCounterIncrement();
> + }
> + table_close(pg_db, RowExclusiveLock);
> + heap_freetuple(tuple);
> +}
> +
> /*
> * ALTER EVENT TRIGGER foo ENABLE|DISABLE|ENABLE ALWAYS|REPLICA
> */
> @@ -391,6 +441,10 @@ AlterEventTrigger(AlterEventTrigStmt *stmt)
>
> CatalogTupleUpdate(tgrel, &tup->t_self, tup);
>
> + if (namestrcmp(&evtForm->evtevent, "login") == 0 &&
> + tgenabled != TRIGGER_DISABLED)
> + SetDatatabaseHasLoginEventTriggers();
> +
> InvokeObjectPostAlterHook(EventTriggerRelationId,
> trigoid, 0);
>
> @@ -557,7 +611,7 @@ filter_event_trigger(CommandTag tag, EventTriggerCacheItem *item)
> static List *
> EventTriggerCommonSetup(Node *parsetree,
> EventTriggerEvent event, const char *eventstr,
> - EventTriggerData *trigdata)
> + EventTriggerData *trigdata, bool unfiltered)
> {
> CommandTag tag;
> List *cachelist;
> @@ -582,10 +636,15 @@ EventTriggerCommonSetup(Node *parsetree,
> {
> CommandTag dbgtag;
>
> - dbgtag = CreateCommandTag(parsetree);
> + if (event == EVT_Login)
> + dbgtag = CMDTAG_LOGIN;
> + else
> + dbgtag = CreateCommandTag(parsetree);
> +
> if (event == EVT_DDLCommandStart ||
> event == EVT_DDLCommandEnd ||
> - event == EVT_SQLDrop)
> + event == EVT_SQLDrop ||
> + event == EVT_Login)
> {
> if (!command_tag_event_trigger_ok(dbgtag))
> elog(ERROR, "unexpected command tag \"%s\"", GetCommandTagName(dbgtag));
> @@ -604,7 +663,10 @@ EventTriggerCommonSetup(Node *parsetree,
> return NIL;
>
> /* Get the command tag. */
> - tag = CreateCommandTag(parsetree);
> + if (event == EVT_Login)
> + tag = CMDTAG_LOGIN;
> + else
> + tag = CreateCommandTag(parsetree);

Seems this bit should instead be in a function, given that you have it in
multiple places.

>
> +/*
> + * Fire login event triggers if any are present. The dathasloginevt
> + * pg_database flag is left when an event trigger is dropped, to avoid
> + * complicating the codepath in the case of multiple event triggers. This
> + * function will instead unset the flag if no trigger is defined.
> + */
> +void
> +EventTriggerOnLogin(void)
> +{
> + List *runlist;
> + EventTriggerData trigdata;
> +
> + /*
> + * See EventTriggerDDLCommandStart for a discussion about why event
> + * triggers are disabled in single user mode or via a GUC. We also need a
> + * database connection (some background workers doesn't have it).
> + */
> + if (!IsUnderPostmaster || !event_triggers ||
> + !OidIsValid(MyDatabaseId) || !MyDatabaseHasLoginEventTriggers)
> + return;
> +
> + StartTransactionCommand();
> + runlist = EventTriggerCommonSetup(NULL,
> + EVT_Login, "login",
> + &trigdata, false);
> +
> + if (runlist != NIL)
> + {
> + /*
> + * Event trigger execution may require an active snapshot.
> + */
> + PushActiveSnapshot(GetTransactionSnapshot());
> +
> + /* Run the triggers. */
> + EventTriggerInvoke(runlist, &trigdata);
> +
> + /* Cleanup. */
> + list_free(runlist);
> +
> + PopActiveSnapshot();
> + }
> + /*
> + * There is no active login event trigger, but our pg_database.dathasloginevt was set.
> + * Try to unset this flag. We use the lock to prevent concurrent
> + * SetDatatabaseHasLoginEventTriggers(), but we don't want to hang the
> + * connection waiting on the lock. Thus, we are just trying to acquire
> + * the lock conditionally.
> + */
> + else if (ConditionalLockSharedObject(DatabaseRelationId, MyDatabaseId,
> + 0, AccessExclusiveLock))

Eek. Why are we doing it this way? I think this is a seriously bad
idea. Maybe it's obvious to you, but it seems much more reasonable to make the
pg_database column an integer and count the number of login event
triggers. When 0, then we don't need to look for login event triggers.

Greetings,

Andres Freund