Re: On login trigger: take three

Hi!

Thank you for the review.

On Tue, Oct 10, 2023 at 7:37 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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.

Should I do this in a separate patch?

> > @@ -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?

Yes, no extra lookups. Fixed.

> > /* 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?

When backend unsets the flag, it acquires the lock first. If lock is
acquired successfully then no other backends hold it. If the
concurrent backend have already inserted an event trigger then we will
detect it by rechecking event list under lock. If the concurrent
backend inserted/enabled event trigger and waiting for the lock, then
it will set the flag once we release the lock.

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

Done.

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

The approach with number of login event trigger obviously will also
work. But this version with lazy flag cleaning avoids need to handle
every unobvious case we delete the event triggers (cascading deletion
etc).

------
Regards,
Alexander Korotkov