Re: On login trigger: take three

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