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