MIME-Version: 1.0
References: 
 <CAN4CZFMmDZMH56O9vb_g7vHqAk8ryWFxBMV19C39PFghENg8kA@mail.gmail.com>
 <CAOYmi+k_et3yXpJ8op71-95j7OYg-kX5bWLgW9YTV_5G7f+O1A@mail.gmail.com>
In-Reply-To: 
 <CAOYmi+k_et3yXpJ8op71-95j7OYg-kX5bWLgW9YTV_5G7f+O1A@mail.gmail.com>
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Mon, 30 Mar 2026 16:26:08 -0700
Message-ID: 
 <CAOYmi+kivcSnazEJA=KWknd3azGYnU3mMq9SUvht5Zq74qNcYQ@mail.gmail.com>
Subject: Re: [oauth] Split and extend PGOAUTHDEBUG
To: Zsolt Parragi <zsolt.parragi@percona.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAOYmi%2BkivcSnazEJA%3DKWknd3azGYnU3mMq9SUvht5Zq74qNcYQ%40mail.gmail.com>
Precedence: bulk

On Mon, Mar 30, 2026 at 2:41=E2=80=AFPM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> v2, attached, rebases this over 993368113. The big change is the
> removal of `custom-ca`; there were a couple of other tweaks to get
> both commits compiling independently.

Now for review. 0001:

I like how the UNSAFE: split works in practice. Implementation-wise, I
think I'd prefer that the debug flags be implemented as bits in a
uint32, and then we can cut down on the boilerplate.
fe-auth-oauth-debug.c is IMO too much code for what the feature is
providing. We should ideally be able to include this in a header from
both locations it's used.

I think `fast-retry` needs to be moved under UNSAFE and renamed to
something that doesn't sound "good". `dos-interval` maybe?

nitpick: `poll-counts` and `print-plugin-errors` choose different
naming conventions, and we're not referring to the poll() API for the
former. `call-count`? `dlopen`?

We need to test that ignored unsafe options are in fact ignored (the
parsed flag is currently being accumulated anyway, in contradiction to
the warning message).

I have a sample patch locally for these suggestions, if you'd like.

--

I'm not a fan of 0002; I don't think it provides enough useful
functionality to offset the cost. You said

> validators authors should be able to verify that mismatched
> configurations are rejected properly by the validator.

but "mismatched configuration" is kind of meaningless here: the
validator interacts with a token, not a client. Real-world validator
implementations already need to test against all manner of broken
tokens -- creating one that's signed by the wrong party shouldn't
really be harder to create than one that's signed by the right party
-- and the code to send a custom token from libpq is minimal (<40
lines).

--Jacob