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

Hi,

On 2022-02-15 21:07:15 +1100, Greg Nancarrow wrote:
> Subject: [PATCH v25] Add a new "login" event and login event trigger support.
> 
> The login event occurs when a user logs in to the system.

I think this needs a HUGE warning in the docs about most event triggers
needing to check pg_is_in_recovery() or breaking hot standby. And certainly
the example given needs to include an pg_is_in_recovery() check.


> +   <para>
> +     The <literal>login</literal> event occurs when a user logs in to the
> +     system.
> +     Any bugs in a trigger procedure for this event may prevent successful
> +     login to the system. Such bugs may be fixed after first restarting the
> +     system in single-user mode (as event triggers are disabled in this mode).
> +     See the <xref linkend="app-postgres"/> reference page for details about
> +     using single-user mode.
> +   </para>

I'm strongly against adding any new dependencies on single user mode.

A saner approach might be a superuser-only GUC that can be set as part of the
connection data (e.g. PGOPTIONS='-c ignore_login_event_trigger=true').


> @@ -293,6 +297,27 @@ insert_event_trigger_tuple(const char *trigname, const char *eventname, Oid evtO
>      CatalogTupleInsert(tgrel, tuple);
>      heap_freetuple(tuple);
>  
> +    if (strcmp(eventname, "login") == 0)
> +    {
> +        Form_pg_database db;
> +        Relation    pg_db = table_open(DatabaseRelationId, RowExclusiveLock);
> +
> +        /* Set dathasloginevt flag in pg_database */
> +        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);
> +        }
> +        else
> +            CacheInvalidateRelcacheByTuple(tuple);
> +        table_close(pg_db, RowExclusiveLock);
> +        heap_freetuple(tuple);
> +    }
> +
>      /* Depend on owner. */
>      recordDependencyOnOwner(EventTriggerRelationId, trigoid, evtOwner);

Maybe I am confused, but isn't that CacheInvalidateRelcacheByTuple call
*entirely* bogus? CacheInvalidateRelcacheByTuple() expects a pg_class tuple,
but you're passing in a pg_database tuple?  And what is relcache invalidation
even supposed to achieve here?


I think this should mention that ->dathasloginevt is unset on login when
appropriate.



> +/*
> + * Fire login event triggers.
> + */
> +void
> +EventTriggerOnLogin(void)
> +{
> +    List       *runlist;
> +    EventTriggerData trigdata;
> +
> +    /*
> +     * See EventTriggerDDLCommandStart for a discussion about why event
> +     * triggers are disabled in single user mode.
> +     */
> +    if (!IsUnderPostmaster || !OidIsValid(MyDatabaseId))
> +        return;
> +
> +    StartTransactionCommand();
> +
> +    if (DatabaseHasLoginEventTriggers())
> +    {
> +        runlist = EventTriggerCommonSetup(NULL,
> +                                          EVT_Login, "login",
> +                                          &trigdata);
> +
> +        if (runlist != NIL)
> +        {
> +            /*
> +             * Make sure anything the main command did will be visible to the
> +             * event triggers.
> +             */
> +            CommandCounterIncrement();

"Main command"?

It's not clear to my why a CommandCounterIncrement() could be needed here -
which previous writes do you need to make visible?



> +            /*
> +             * Runlist is empty: clear dathasloginevt flag
> +             */
> +            Relation    pg_db = table_open(DatabaseRelationId, RowExclusiveLock);
> +            HeapTuple    tuple = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId));
> +            Form_pg_database db;
> +
> +            if (!HeapTupleIsValid(tuple))
> +                elog(ERROR, "cache lookup failed for database %u", MyDatabaseId);
> +
> +            db = (Form_pg_database) GETSTRUCT(tuple);
> +            if (db->dathasloginevt)
> +            {
> +                /*
> +                 * 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);

This doesn't work. RowExclusiveLock doesn't conflict with another
RowExclusiveLock.

What is the AcceptInvalidationMessages() intending to do here?


> +                if (runlist == NULL)    /* list is still empty, so clear the
> +                                         * flag */
> +                {
> +                    db->dathasloginevt = false;
> +                    CatalogTupleUpdate(pg_db, &tuple->t_self, tuple);
> +                }
> +                else
> +                    CacheInvalidateRelcacheByTuple(tuple);

Copy of the bogus relcache stuff.


Greetings,

Andres Freund