On 12.11.24 22:47, Jacob Champion wrote:
On Fri, Nov 8, 2024 at 1:21 AM Peter Eisentraut <pe...@eisentraut.org> wrote:
I find the way the installation options are structured a bit odd. I
would have expected --with-libcurl and -Dlibcurl (or --with-curl and
-Dcurl). These build options usually just say, use this library.
It's patterned directly off of -Dssl/--with-ssl (which I liberally
borrowed from) because the builtin client implementation used to have
multiple options for the library in use. I can change it if needed,
but I thought it'd be helpful for future devs if I didn't undo the
generalization.
Personally, I'm not even a fan of the -Dssl/--with-ssl system. I'm more
attached to --with-openssl. But if you want to stick with that, a more
suitable naming would be something like, say, --with-httplib=curl, which
means, use curl for all your http needs. Because if we later add other
functionality that can use some http, I don't think we want to enable or
disable them all individually, or even mix different http libraries for
different features. In practice, curl is a widely available and
respected library, so I'd expect packagers to be just turn it all on
without much further consideration.
I'm confused by the use of PG_MAX_AUTH_TOKEN_LENGTH in the
pg_be_oauth_mech definition. What does that mean?
Just that Bearer tokens can be pretty long, so we don't want to limit
them to 1k like SCRAM does. 64k is probably overkill, but I've seen
anecdotal reports of tens of KBs and it seemed reasonable to match
what we're doing for GSS tokens.
Ah, ok, I totally misread that code. Could you maybe write this definition
+/* Mechanism declaration */
+const pg_be_sasl_mech pg_be_oauth_mech = {
+ oauth_get_mechanisms,
+ oauth_init,
+ oauth_exchange,
+
+ PG_MAX_AUTH_TOKEN_LENGTH,
+};
with designated initializers:
const pg_be_sasl_mech pg_be_oauth_mech = {
.get_mechanisms = oauth_get_mechanisms,
.init = oauth_init,
.exchange = oauth_exchange,
.max_message_length = PG_MAX_AUTH_TOKEN_LENGTH,
};
The CURL_IGNORE_DEPRECATION thing needs clarification. Is that in
progress?
Thanks for the nudge, I've started a thread:
https://curl.se/mail/lib-2024-11/0028.html
It looks like this has been clarified, so let's put that URL into a code
comment.
This is only used once, in append_urlencoded(), and there are other
ways to communicate errors, for example returning a bool.
I'd rather not introduce two parallel error indicators for the caller
to have to check for that particular part. But I can change over to
using the (identical!) termPQExpBuffer. I felt like the other API
signaled the intent a little better, though.
I think it's better to not drill a new hole into an established API for
such a limited use. So termPQExpBuffer() seems better for now. If it
later turns out, many callers are using termPQExpBuffer() for fake error
handling purposes, then that can be considered independently.
On Cirrus CI Windows task, this test reports SKIP. Can't tell why,
because the log is not kept. I suppose you expect this to work on
Windows (but see my comment below)
No, builtin client support does not exist on Windows. If/when it's
added, the 001_server tests will need to be ported.
Could you put some kind of explicit conditional or a comment in there.
Right now, it's not possible to tell that Windows is not supported.
