Re: Improve OAuth discovery logging

> On Mar 17, 2026, at 08:24, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Mon, Mar 16, 2026 at 12:45 PM Zsolt Parragi
> <zsolt.parragi@percona.com> wrote:
>> I tried to figure out if this is fine or not, but isn't it the same as
>> the existing ereport(ERROR, ...) calls everywhere in the sasl/scram
>> code?
> 
> Those are *also* not good, IMHO; they're what I had in mind when I
> said "it's unusual/invisible". (ERROR is upgraded to FATAL here, so
> they're also misleading.) OAuth inherited a few of those from SCRAM to
> avoid divergent behavior for protocol violations, but I don't really
> want to lock that usage into the SASL architecture by myself,
> especially not for normal operation. CheckSASLAuth should ideally have
> control over the logic flow.
> 
> (It might be nice to make it possible to throw ERRORs from inside
> authentication code without bypassing the top level. Then maybe we
> could square that with our treatment of logdetail et al. But we'd have
> to decide how we want protocol errors to interact with the hook.)
> 
> On Mon, Mar 16, 2026 at 11:14 AM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>> I'm working on a three-patch set to add FATAL_CLIENT_ONLY, the new
>> abandoned state, and the log fix making use of both.
> 
> Attached as v8.
> 
> --Jacob
> <v8-0001-Add-FATAL_CLIENT_ONLY-to-ereport-elog.patch><v8-0003-oauth-Don-t-log-discovery-connections-by-default.patch><v8-0002-sasl-Allow-backend-mechanisms-to-abandon-exchange.patch>

A few review comments:

1 - 0001
```
@@ -3800,6 +3801,7 @@ send_message_to_server_log(ErrorData *edata)
 				syslog_level = LOG_WARNING;
 				break;
 			case FATAL:
+			case FATAL_CLIENT_ONLY:
 				syslog_level = LOG_ERR;
```

As is_log_level_output() returns false against FATAL_CLIENT_ONLY, so that FATAL_CLIENT_ONLY should not reach send_message_to_server_log(). Should we assert edata->elevel != FATAL_CLIENT_ONLY?

2 - 0002
```
+		if (!abandoned)
+		{
+			/*
+			 * Programmer error: caller needs to track the abandoned state for
+			 * this mechanism.
+			 */
+			elog(ERROR, "SASL exchange was abandoned, but CheckSASLAuth isn't tracking it");
+		}
```

As far as I understand, for a programmer error, Assert should be used. Why do we use elog(ERROR) here?

3 - 0002 - auth.c
```
	cdetail = psprintf(_("Connection matched file \"%s\" line %d: \"%s\""),
					   port->hba->sourcefile, port->hba->linenumber,
					   port->hba->rawline);
	if (logdetail)
		logdetail = psprintf("%s\n%s", logdetail, cdetail);
	else
		logdetail = cdetail;

	ereport(elevel,
			(errcode(errcode_return),
			 errmsg(errstr, port->user_name),
			 logdetail ? errdetail_log("%s", logdetail) : 0));
```

This comment is not against the code introduced by this patch, just as this patch is touching the code block.

Based on https://www.postgresql.org/docs/current/error-style-guide.html, a detail message should end with a period.

003 LGTM.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/