Re: [PoC] Federated Authn/z with OAUTHBEARER - Mailing list pgsql-hackers

> On 22 Apr 2025, at 01:19, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> v8 also makes the following changes:

Thanks for this version, a few small comments:

+  if oauth_flow_supported
+    cdata.set('USE_LIBCURL', 1)
+  elif libcurlopt.enabled()
+    error('client OAuth is not supported on this platform')
+  endif
We already know that libcurlopt.enabled() is true here so maybe just doing
if-else-endif would make it more readable and save readers thinking it might
have changed?  Also, "client OAuth" reads a bit strange, how about "client-side
OAuth" or "OAuth flow module"?


-       appendPQExpBufferStr(&conn->errorMessage,
-                            libpq_gettext(actx->errctx));
-       appendPQExpBufferStr(&conn->errorMessage, ": ");
+       appendPQExpBufferStr(errbuf, libpq_gettext(actx->errctx));
+       appendPQExpBufferStr(errbuf, ": ");
I think we should take this opportunity to turn this into a appendPQExpBuffer()
with a format string instead of two calls.


+       len = errbuf->len;
+       if (len >= 2 && errbuf->data[len - 2] == '\n')
Now that the actual variable, errbuf->len, is short and very descriptive I
wonder if we shouldn't just use this as it makes the code even clearer IMO.

--
Daniel Gustafsson