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

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