On Wed, Apr 1, 2026 at 2:35 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> +/* all safe and unsafe flags, for the legacy UNSAFE behavior */
> +#define OAUTHDEBUG_UNSAFE_ALL ((uint32) ~0)
>
> The name of this variable is a bit confusing, it's not only about
> unsafe settings. I understand why you added the unsafe to it, but
> before checking the value/comment I thought this will be the bitmask
> of all unsafe options.
I agree.
> On the other hand I don't have a better idea
> other than simply using ALL.
OAUTHDEBUG_LEGACY_UNSAFE?
> +oauth_get_debug_flags(void)
> +{
> + uint32 flags = 0;
> + const char *env = getenv("PGOAUTHDEBUG");
> ...
>
> One of the reasons why I implemented this in a C file is because I
> wanted to avoid reparsing and warning spam/duplication.
I think I'm missing something; how does the choice of .c/.h change
things? There's no static tracking in v1 of the patchset (nor should
there be without locking or atomics, which is not maintenance I really
want to introduce for a debug feature).
> Reparsing
> shouldn't be a major issue since this is a debug feature, but this
> approach causes a warning to print twice in a few corner cases.
Which new corner cases? v1 also prints duplicates (e.g. with
`UNSAFE:blah,http`). I didn't intend to introduce any new calls to
oauth_get_debug_flags() over those already done in v1/v2; if I did
that's a bug.
--Jacob
