Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.12\))
Subject: Re: Fix OAuth validator docs for error_detail on internal errors
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <28A71A53-EB55-4A29-8C20-BBAF1BC82D2A@gmail.com>
Date: Fri, 5 Jun 2026 21:31:50 +0200
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
 Jacob Champion <jacob.champion@enterprisedb.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <CDBC3C10-0F67-4254-8462-03F8C25E0143@yesql.se>
References: <0281836A-F5FF-41A5-9EE1-656C1FAAC6B2@gmail.com>
 <D9F1340C-22C2-43A5-B790-5D5EB379394A@yesql.se>
 <28A71A53-EB55-4A29-8C20-BBAF1BC82D2A@gmail.com>
To: Chao Li <li.evan.chao@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/CDBC3C10-0F67-4254-8462-03F8C25E0143%40yesql.se>
Precedence: bulk

> On 5 Jun 2026, at 00:21, Chao Li <li.evan.chao@gmail.com> wrote:
>> On Jun 5, 2026, at 04:19, Daniel Gustafsson <daniel@yesql.se> wrote:
>>=20
>>> On 4 Jun 2026, at 14:33, Chao Li <li.evan.chao@gmail.com> wrote:
>>=20
>>> =E2=80=9CAny result parameters are ignored=E2=80=9D is no longer =
accurate; it should be something like =E2=80=9Cany result parameters =
except result->error_detail are ignored=E2=80=9D. This patch just makes =
that tiny doc fix.
>>=20
>> That's true, but error_detail is explained in detail in the next =
paragraph so
>> I'm not sure this change is needed.
>=20
> Agreed. Adding the =E2=80=9Cexception for result->error_detail=E2=80=9D =
sounds a bit redundant with the next paragraph. But =E2=80=9Cany result =
parameters are ignored=E2=80=9D also seems to conflict with the next =
paragraph, so I think we can just delete that part.
>=20
> ValidatorModuleResult has three fields, so the logic is:
>=20
> * The first paragraph talks about authorized and authn_id when the =
validator succeeds.
> * The second paragraph talks about the validator=E2=80=99s return =
values.
> * The third paragraph talks about result->error_detail when the =
validator fails.
>=20
>> Another thing we don't explicitly document which seems more =
interesting is that
>> authn_id is used even in case of failure if log_connections is =
enabled.  Maybe
>> that deserves a mention?
>=20
> This is a good point. I added that in v2.

This version looks good to me, the authn_id sentence is a bit long so I =
might
do some careful rewording before pushing.

--
Daniel Gustafsson