Re: Improve OAuth discovery logging - Mailing list pgsql-hackers

> 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/