> On Mar 17, 2026, at 08:24, Jacob Champion <[email protected]>
> wrote:
>
> On Mon, Mar 16, 2026 at 12:45 PM Zsolt Parragi
> <[email protected]> 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
> <[email protected]> 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/
