On 07.01.25 23:24, Jacob Champion wrote:
On Thu, Jan 2, 2025 at 2:11 AM Peter Eisentraut <pe...@eisentraut.org> wrote:
There is a mix of using "URL" and "URI" throughout the patch. I tried
to look up in the source material (RFCs) what the correct use would
be, but even they are mixing it in nonobvious ways. Maybe this is
just hopelessly confused, or maybe there is a system that I don't
recognize.
Ugh, yeah. I think my "system" was whether the RFC I read most
recently had used "URL" or "URI" more in the text.
In an ideal world, I'd just switch to "URL" to avoid confusion. There
are some URNs in use as part of OAuth (e.g.
`urn:ietf:params:oauth:grant-type:device_code`) but I don't think I
refer to those as URIs anyway. And more esoteric forms of URI (data:)
are not allowed.
However... there are some phrases, like "Well-Known URI", where that's
just the Name of the Thing. Similarly, when the wire protocol itself
uses "URI" (say, in JSON field names), I'd rather be consistent to
make searching easier.
Is it enough to prefer "URL" in the user-facing documentation (at
least, when it doesn't conflict with other established naming
conventions), and accept both in the code?
The above explanation makes sense to me. I don't know what you mean by
"accept in the code". I would agree with "tolerate some inconsistency"
in the code but not with, like, create alias names for all the interface
names.
* src/interfaces/libpq/libpq-fe.h
+#ifdef WIN32
+#include <winsock2.h> /* for SOCKET */
+#endif
Including a system header like that seems a bit unpleasant. AFAICT,
you only need it for this:
+ PostgresPollingStatusType (*async) (PGconn *conn,
+ struct _PGoauthBearerRequest
*request,
+ SOCKTYPE * altsock);
But that function could already get the altsock handle via
conn->altsock. So maybe that is a way to avoid the whole socket type
dance in this header.
It'd also couple clients against libpq-int.h, so they'd have to
remember to recompile every release. I'm worried that'd cause a bunch
of ABI problems...
Couldn't that function use PQsocket() to get at the actual socket from
the PGconn handle?
* src/test/modules/oauth_validator/fail_validator.c
+{
+ elog(FATAL, "fail_validator: sentinel error");
+ pg_unreachable();
+}
This pg_unreachable() is probably not necessary after elog(FATAL).
Cirrus completes successfully with that, but MSVC starts complaining:
warning C4715: 'fail_token': not all control paths return a value
Is that expected?
Ah yes, because MSVC doesn't support the noreturn attribute. (See
<https://www.postgresql.org/message-id/flat/pxr5b3z7jmkpenssra5zroxi7qzzp6eswuggokw64axmdixpnk%40zbwxuq7gbbcw>.)
So ok to leave as you had it for now.
