On Mon, Mar 30, 2026 at 2:41 PM 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
