Subject: [PATCH] OAuth: fix performance bug with stuck multiplexer events

From: Jacob Champion <[email-masked]>
Date: Thu, 26 Jun 2025 16:33:35 -0700

Hi all,

The current implementation of the OAuth flow is overly eager to signal
readiness when there's nothing to do, so sometimes we burn CPU for no
reason. This was discussed briefly back in [1], but I'll summarize
here so no one has to dig through that megathread.

= Background =

A major interaction between libpq and libcurl happens through a
callback -- register_socket() in oauth-curl.c -- which links the
sockets that Curl cares about into a single descriptor that our
clients can wait on. This descriptor, which is either an epoll set or
a kqueue, is called the multiplexer.

Curl has wide latitude to make this interaction as simple or as
complicated as it wants, and the complexity additionally depends on
the transport in use. For a simple HTTP request/response with older
Curls, the callback order might look like

- wait for reads on fd 5
- done with fd 5

But for a modern dual-stack system speaking HTTPS, you might get something like

- wait for writes on fd 6, time out after 200ms
- wait for writes on fd 7
- done with fd 6, cancel the timeout
- wait for reads only on fd 7, time out after five minutes
- wait for both reads and writes on fd 7
...
- done with fd 7, cancel the timeout

= Getting Stuck =

The Linux/epoll implementation handles the latter case well (with one
caveat, discussed later), but with the BSD/kqueue implementation, the
multiplexer can get "stuck open" on stale events that Curl no longer
cares about, and clients of libpq can loop on PQconnectPoll()
pointlessly.

The flow still works -- there's no bug in functionality, just in
performance -- but that also makes it hard to detect in tests. And for
simpler network cases (the ones in our tests), the bug is nearly
invisible, because Curl will unregister descriptors and clear the
stuck events before they have any real effect.

0001, attached, fixes this by counting the number of outstanding event
registrations and draining up to that many events from the kqueue
after libcurl gives control back to us. This ensures that any stale
events will be knocked back down. (This design was originally
suggested by Thomas Munro in a related conversation about kqueue
corner cases; thanks Thomas!)

If you follow the flow from start to finish, you may notice the
following oddity: I took great pains not to track which events had
been set, instead telling libcurl "something happened, figure it out"
-- and now in this patchset I'm pulling all of those events off in a
big wave only to discard them. As punishment for trying to keep the
epoll side of things simple, I'm now having to add unnecessary
complexity to the kqueue side. Seems like I'll eventually need to
balance the two out again.

= Timeouts =

There is a similar "stale event" bug for the timer, with both epoll
and kqueue: if Curl does not disable a timeout explicitly after it
fires, that timer's descriptor will simply remain signaled until
something else happens to set it.

Unfortunately, we can't solve this bug in the same place as 0001. If
we clear stale timers after calling into Curl, that would introduce a
race: Curl may have set a short timeout that has already passed, and
we need that event to remain signaled so that the client will call us
back.

So expired timers have to be cleared _before_ calling back into Curl,
which I have done in 0002. As part of that, I've strengthened the
timer_expired() API so that it always returns false when the timer is
disabled. It was previously a don't-care, since we only called it when
we knew the timer must be running, so the kqueue and epoll
implementations used to give different answers.

= Testing =

It was hard to notice any of this with the current tests, because
they're mostly focused on ensuring that the flow works, not on
internal efficiency. I tried to find a way to internally detect when a
file descriptor had gone stale by accident, but I think it's hard to
do that without false positives if we don't know what Curl is doing.

In the end, I decided to test a heuristic: we expect roughly 5-15
calls for these simple localhost flows, so if we see something more
than an order of magnitude above that, something is wrong. (When "bad
behavior" happens in the CI, I see hundreds or thousands of calls.)
This is implemented in 0003 by simply counting the number of calls and
printing them at the end of the flow in debug mode.

I strengthened a bunch of postconditions in the first two patches, so
to try to prevent decay (and keep other maintainers from having to
remember all this), I've added a unit test executable that pins many
internal expectations. It's essentially a version of the libpq-oauth
plugin which has been given a main() function and TAP macros. This is
done in 0004.

I feel confident that these new tests are useful, because:

- The new unit tests caught that my initial register_socket attempt,
written on macOS, did not work on the other BSDs. (EV_RECEIPT works
differently there.)
- If I revert the changes to register_socket, the new unit tests fail
on all BSDs in the CI, and the call-count test fails on NetBSD and
OpenBSD.
- If I comment out the new call to drain_timer_events(), the
call-count test fails on macOS.
- If I revert the changes to timer_expired, the new unit tests fail on
Linux. (There are no user-visible effects of that change, because it
doesn't lead to more calls.)

Unfortunately, if I just comment out the new call to
drain_socket_events(), nothing fails in the CI. :( But my personal
test suite (which makes use of TLS plus dual-stack sockets) does
notice the regression in the call count immediately, with some tests
using tens of thousands of calls for a single client flow without the
fix. I hope that's good enough for now, because I'm not sure if I can
safely push the additional TLS infrastructure into the oauth_validator
tests for 18.

My plan, if this code seems reasonable, is to backport 0001-0003, but
keep the larger 0004 on HEAD only until it has proven to be stable.
It's a big new suite and I want to make sure it's not flapping on some
buildfarm animal. Eventually I'll backport that too.

WDYT?

Thanks,
--Jacob

[1] https://postgr.es/m/CAOYmi%2Bn4EDOOUL27_OqYT2-F2rS6S%2B3mK-ppWb2Ec92UEoUbYA%40mail.gmail.com

---

From: Jacob Champion <[email-masked]>
Date: Mon, 28 Jul 2025 13:52:42 -0700

Hi all,

> buildfarm animal. Eventually I'll backport that too.

Any thoughts on the approach? Too big/too scary/too BSD-specific?

A small bit of self-review: a comment I wrote in the tests suggested
that the choice of readable/writable events was up to the multiplexer
implementation, but it *must* choose readable, due to the hardcoded
use of PGRES_POLLING_READING throughout the current code. Updated in
v2.

Thanks,
--Jacob
1:  ddb7875bb58 = 1:  379c12b5d26 oauth: Remove stale events from the kqueue multiplexer
2:  a871ce498ea = 2:  f30317d7265 oauth: Remove expired timers from the multiplexer
3:  5033b6d51c1 = 3:  d243d28964d oauth: Track total call count during a client flow
4:  f4a640f7995 ! 4:  ca6fd237653 oauth: Add unit tests for multiplexer handling
    @@ src/interfaces/libpq-oauth/test-oauth-curl.c (new)
     + *
     + * NB: our current multiplexer implementations (epoll/kqueue) are *readable*
     + * when the underlying libcurl sockets are *writable*. This behavior is pinned
    -+ * here to record that expectation, but it's not a required part of the API. If
    -+ * you've added a new implementation that doesn't have that behavior, feel free
    -+ * to modify this test.
    ++ * here to record that expectation; PGRES_POLLING_READING is hardcoded
    ++ * throughout the flow and would need to be changed if a new multiplexer does
    ++ * something different.
     + */
     +#define mux_is_ready(MUX, DEADLINE, TEST) \
     +	do { \

---

From: Thomas Munro <[email-masked]>
Date: Tue, 5 Aug 2025 02:53:18 +1200

> use of PGRES_POLLING_READING throughout the current code. Updated in

[FYI, I'm looking into this and planning to post a review in 1-2 days...]

---

From: Jacob Champion <[email-masked]>
Date: Mon, 4 Aug 2025 08:24:48 -0700

> [FYI, I'm looking into this and planning to post a review in 1-2 days...]

Thanks so much!

--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Thu, 7 Aug 2025 03:03:57 +1200

> > [FYI, I'm looking into this and planning to post a review in 1-2 days...]

0001:

So, the problem is that poll(kqueue_fd) reports POLLIN if any events
are queued, but level-triggered events are only rechecked and possibly
cancelled if you actually call kevent().  Hmm, what if you just called
kevent() with an output array of size one?

 * If it returns 0, that means it has rechecked all queued
level-triggered events and booted them all out because they are no
longer true.  poll(fd) won't report POLLIN until one of them is queued
again.

 * If it returns 1, then it stopped on the first level-triggered event
that it rechecked and found to be still true.  Who cares if there are
more that didn't get rechecked?  poll(fd) will report POLLIN either
way, and that's what you want.

---

From: Jacob Champion <[email-masked]>
Date: Wed, 6 Aug 2025 09:13:32 -0700

> way, and that's what you want.

I think the weaker guarantee might be sufficient. I was trying to get
a stronger primitive in place so that we wouldn't have to worry about
it down the line, but it is a lot of code to pay...

One sharp edge of that strategy is caught by the new tests, which is
that if you call drain_socket_events() and then unset the timer, your
multiplexer is still readable until you call drain_socket_events() yet
again. At the moment, our code only ever calls those two in the
opposite order (due to the race condition pointed out in 0002); we'd
just have to keep that in mind. Maybe "drain" would no longer be the
verb to use there.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Wed, 6 Aug 2025 16:55:44 -0700

On Wed, Aug 6, 2025 at 9:13 AM Jacob Champion
<[email-masked]> wrote:
> Maybe "drain" would no longer be the
> verb to use there.

I keep describing this as "combing" the queue when I talk about it in
person, so v3-0001 renames this new operation to comb_multiplexer().
And the CI (plus the more strenuous TLS tests) confirms that the
callback count is still stable with this weaker guarantee, so I've
gotten rid of the event-counting code.

Now that I'm no longer counting events, I can collapse the changes to
register_socket(). I can't revert those changes entirely, because then
we regress the case where Curl switches a socket from IN to OUT (this
is enforced by the new unit tests). But I'm not sure that the existing
comment adequately explained that fix anyway, and I didn't remember to
call it out in my initial email, so I've split it out into v3-0002.
It's much smaller.

The tests (now in 0005) have been adjusted for the new "combing"
behavior, and I've added a case to ensure that multiple stale events
are swept up by a single call to comb_multiplexer().

Thanks!
--Jacob
1:  379c12b5d26 < -:  ----------- oauth: Remove stale events from the kqueue multiplexer
-:  ----------- > 1:  c5cdccfe374 oauth: Remove stale events from the kqueue multiplexer
-:  ----------- > 2:  7725e0c173b oauth: Ensure unused socket registrations are removed
2:  f30317d7265 ! 3:  6ccf7a5d156 oauth: Remove expired timers from the multiplexer
    @@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
     +					 * edge-triggered) timeouts, and ours are level-triggered
     +					 * via the mux.
     +					 *
    -+					 * This can't be combined with the drain_socket_events()
    -+					 * call below: we might accidentally clear a short timeout
    -+					 * that was both set and expired during the call to
    ++					 * This can't be combined with the comb_multiplexer() call
    ++					 * below: we might accidentally clear a short timeout that
    ++					 * was both set and expired during the call to
     +					 * drive_request().
     +					 */
     +					if (!drain_timer_events(actx, NULL))
3:  d243d28964d ! 4:  2be993b8f07 oauth: Track total call count during a client flow
    @@ Metadata
      ## Commit message ##
         oauth: Track total call count during a client flow
     
    -    Tracking down the bugs that led to the addition of drain_socket_events()
    +    Tracking down the bugs that led to the addition of comb_multiplexer()
         and drain_timer_events() was difficult, because an inefficient flow is
         not visibly different from one that is working properly. To help
         maintainers notice when something has gone wrong, track the number of
    @@ src/interfaces/libpq-oauth/oauth-curl.c: struct async_ctx
      	bool		used_basic_auth;	/* did we send a client secret? */
      	bool		debugging;		/* can we give unsafe developer assistance? */
     +	int			dbg_num_calls;	/* (debug mode) how many times were we called? */
    + };
      
    - #if defined(HAVE_SYS_EVENT_H)
    - 	int			nevents;		/* how many events are we waiting on? */
    + /*
     @@ src/interfaces/libpq-oauth/oauth-curl.c: PostgresPollingStatusType
      pg_fe_run_oauth_flow(PGconn *conn)
      {
    @@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow(PGconn *conn)
      	result = pg_fe_run_oauth_flow_impl(conn);
      
     +	/*
    -+	 * To assist with finding bugs in drain_socket_events() and
    ++	 * To assist with finding bugs in comb_multiplexer() and
     +	 * drain_timer_events(), when we're in debug mode, track the total number
     +	 * of calls to this function and print that at the end of the flow.
     +	 *
4:  ca6fd237653 ! 5:  50257bf32eb oauth: Add unit tests for multiplexer handling
    @@ src/interfaces/libpq-oauth/test-oauth-curl.c (new)
     +
     +		/*
     +		 * Draining the pipe should unset the multiplexer again, once the old
    -+		 * event is drained.
    ++		 * event is cleared.
     +		 */
     +		Assert(drain_pipe(rfd, 1));
    -+		Assert(drain_socket_events(actx));
    ++		Assert(comb_multiplexer(actx));
     +		mux_is_not_ready(actx->mux, "when fd is drained");
     +
     +		/* Undo any unidirectional emulation. */
    @@ src/interfaces/libpq-oauth/test-oauth-curl.c (new)
     +			ssize_t		written;
     +
     +			/*
    -+			 * Fill the pipe. Once the old writable event is drained, the mux
    ++			 * Fill the pipe. Once the old writable event is cleared, the mux
     +			 * should not be ready.
     +			 */
     +			Assert((written = fill_pipe(wfd)) > 0);
     +			printf("# pipe buffer is full at %zd bytes\n", written);
     +
    -+			Assert(drain_socket_events(actx));
    ++			Assert(comb_multiplexer(actx));
     +			mux_is_not_ready(actx->mux, "when fd buffer is full");
     +
     +			/* Drain the pipe again. */
    @@ src/interfaces/libpq-oauth/test-oauth-curl.c (new)
     +
     +		/* Make sure an expired timer doesn't interfere with event draining. */
     +		{
    ++			bool		expired;
    ++
     +			/* Make the rfd appear unidirectional if necessary. */
     +			if (bidirectional)
     +				Assert((bidi_pipe_size = fill_pipe(rfd)) > 0);
    @@ src/interfaces/libpq-oauth/test-oauth-curl.c (new)
     +			 * Draining the pipe should unset the multiplexer again, once the
     +			 * old event is drained and the timer is reset.
     +			 *
    -+			 * Order matters to avoid false negatives. First drain the socket,
    -+			 * then unset the timer. We're trying to catch the case where the
    -+			 * pending timer expiration event takes the place of one of the
    -+			 * socket events we're attempting to drain.
    ++			 * Order matters, since comb_multiplexer() doesn't have to remove
    ++			 * stale events when active events exist. Follow the call sequence
    ++			 * used in the code: drain the timer expiration, drain the pipe,
    ++			 * then clear the stale events.
     +			 */
    ++			Assert(drain_timer_events(actx, &expired));
     +			Assert(drain_pipe(rfd, 1));
    -+			Assert(drain_socket_events(actx));
    -+			Assert(set_timer(actx, -1));
    ++			Assert(comb_multiplexer(actx));
     +
    ++			is(expired, 1, "drain_timer_events() reports expiration");
     +			is(timer_expired(actx), 0, "timer is no longer expired");
     +			mux_is_not_ready(actx->mux, "when fd is drained and timer reset");
     +
    @@ src/interfaces/libpq-oauth/test-oauth-curl.c (new)
     +			if (bidirectional)
     +				Assert(drain_pipe(wfd, bidi_pipe_size));
     +		}
    ++
    ++		/* Ensure comb_multiplexer() can handle multiple stale events. */
    ++		{
    ++			int			rfd2,
    ++						wfd2;
    ++
    ++			/* Create a second local pipe. */
    ++			Assert(pipe(pipefd) == 0);
    ++			rfd2 = pipefd[0];
    ++			wfd2 = pipefd[1];
    ++
    ++			/* Make both rfds appear unidirectional if necessary. */
    ++			if (bidirectional)
    ++			{
    ++				Assert((bidi_pipe_size = fill_pipe(rfd)) > 0);
    ++				Assert(fill_pipe(rfd2) == bidi_pipe_size);
    ++			}
    ++
    ++			/* Register for read events on both fds, and make them readable. */
    ++			Assert(register_socket(NULL, rfd, in_event, actx, NULL) == 0);
    ++			Assert(register_socket(NULL, rfd2, in_event, actx, NULL) == 0);
    ++
    ++			Assert(write(wfd, "x", 1) == 1);
    ++			Assert(write(wfd2, "x", 1) == 1);
    ++
    ++			mux_is_ready(actx->mux, deadline, "when two fds are readable");
    ++
    ++			/*
    ++			 * Drain both fds. comb_multiplexer() should then ensure that the
    ++			 * mux is no longer readable.
    ++			 */
    ++			Assert(drain_pipe(rfd, 1));
    ++			Assert(drain_pipe(rfd2, 1));
    ++			Assert(comb_multiplexer(actx));
    ++			mux_is_not_ready(actx->mux, "when two fds are drained");
    ++
    ++			/* Stop listening. */
    ++			Assert(register_socket(NULL, rfd, CURL_POLL_REMOVE, actx, NULL) == 0);
    ++			Assert(register_socket(NULL, rfd2, CURL_POLL_REMOVE, actx, NULL) == 0);
    ++
    ++			/* Undo any unidirectional emulation. */
    ++			if (bidirectional)
    ++			{
    ++				Assert(drain_pipe(wfd, bidi_pipe_size));
    ++				Assert(drain_pipe(wfd2, bidi_pipe_size));
    ++			}
    ++
    ++			close(rfd2);
    ++			close(wfd2);
    ++		}
     +	}
     +
     +	close(rfd);

---

From: Thomas Munro <[email-masked]>
Date: Thu, 7 Aug 2025 13:45:30 +1200

> gotten rid of the event-counting code.

I was about to hit send on an email suggesting "reset_multiplexer()",
and an attempt at distilling the explanation to a short paragraph, but
your names and explanations are also good so please feel 100% free to
ignore these suggestions.

"Unlike epoll descriptors, kqueue descriptors only transition from
readable to unreadable when kevent() is called and finds nothing,
after removing level-triggered conditions that have gone away.  We
therefore need a dummy kevent() call after operations might have been
performed on the monitored sockets or timer_fd.  Any event returned is
ignored here, but it also remains queued (being level-triggered) and
leaves the descriptor readable.  This is a no-op for epoll
descriptors."

Reviewing the timer stuff, it is again tempting to try to use an
EVFILT_TIMER directly, but I like your approach: it's nice to be able
to mirror the Linux coding, with this minor kink ironed out.

FWIW I re-read the kqueue paper's discussion of the goals of making
kqueue descriptors themselves monitorable/pollable, and it seems it
was mainly intended for hierarchies of kqueues, like your timer_fd,
with the specific aim of expressing priorities.  It doesn't talk about
giving them to code that doesn't know it has a kqueue fd (the client)
and never calls kevent() and infers the events instead (libcurl).
That said, the fact that the filter function for kqueue fds just
checks queue size > 0 without running the filter functions for the
queued events does seem like a bit of an abstraction leak from this
vantage point.  At least it's easy enough to work around in the
kqueue-managing middleman code once you understand it.

> call it out in my initial email, so I've split it out into v3-0002.

Much nicer!  Yeah, that all makes sense.

> are swept up by a single call to comb_multiplexer().

This all looks pretty good to me.  I like the C TAP test.  PostgreSQL
needs more of this.

s/signalled/signaled/ (= US spelling) in a couple of places.

---

From: Thomas Munro <[email-masked]>
Date: Thu, 7 Aug 2025 14:43:17 +1200

On Thu, Aug 7, 2025 at 1:45 PM Thomas Munro <[email-masked]> wrote:
> I like the C TAP test.  PostgreSQL
> needs more of this.

I should add, I didn't look closely at that part since you said it's
not in scope for back-patching.  I'd like to, though, later.

I wonder if you would be interested in this attempt at centralised
infrastructure for unit testing our C code over here.  I'm not
suggesting it for your immediate problem, just noting the overlap:

https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com

Basically I would like to be able to dump easy-to-write files into the
tree that say stuff like this ↓ and have the build scripts find them,
build them and test them without all the module boilerplate stuff or a
running server (though that aspect is obviously not relevant for your
frontend case).  Like you find in googletest or various xunit-style
systems in other projects, but integrated with our TAP universe (or
whatever replaces it if we escape from Perl).  But I never got the
configure part of it working.

 PG_BEGIN_TESTS();
 ...
 PG_EXPECT_EQ(pg_preadv(fd, iov, 2, 11), 0);
 PG_EXPECT_EQ(pg_pread(fd, buffer, 10, 0), 10);
 PG_EXPECT_EQ_STR(buffer, "helloworld");
 ...
 PG_END_TESTS();

---

From: Dagfinn Ilmari Mannsåker <[email-masked]>
Date: Thu, 07 Aug 2025 17:35:29 +0100

Jacob Champion <[email-masked]> writes:

> From 50257bf32eb2b0972e5139ac4a79367372c77385 Mon Sep 17 00:00:00 2001
> From: Jacob Champion <[email-masked]>
> Date: Wed, 5 Mar 2025 15:04:34 -0800
> Subject: [PATCH v3 5/5] oauth: Add unit tests for multiplexer handling

I haven't read the meat of the patch, but I have some comments on the
tests:

> +IPC::Run::run ['oauth_tests'],
> +  '>', IPC::Run::new_chunker, sub { print {$out} $_[0] },
> +  '2>', IPC::Run::new_chunker, sub { print {$err} $_[0] }
> +  or die "oauth_tests returned $?";

We've recently switched to using fat commas (=>) between options and
their arguments, and that includes the file redirections in IPC::Run.
Although not semantically meaningful, I'd also be tempted to put parens
around the argument list for each redirect, so it's clear that they go
together.

Also, indirect object syntax (print {$fh} ...) is ugly and
old-fashioned, it's nicer to call it as a method on the filehandle.

So I'd write the above as:

    IPC::Run::run ['oauth_tests'],
      '>' => (IPC::Run::new_chunker, sub { $out->print($_[0]) }),
      '2>' => (IPC::Run::new_chunker, sub { $err->print($_[0]) })
     or die "oauth_tests returned $?";

As for the C TAP tests, there's already a bunch of TAP-outputting
infrastructure in pg_regress.c.  Would it make sense to factor that out
into a common library?

- ilmari

---

From: Jacob Champion <[email-masked]>
Date: Thu, 7 Aug 2025 11:11:02 -0700

> leaves the descriptor readable.  This is a no-op for epoll

I really like this; I'm working it into the doc comment.

> and never calls kevent() and infers the events instead (libcurl).

Interesting! It would be nice if they papered over this for us, but I
guess that's water under the bridge.

> s/signalled/signaled/ (= US spelling) in a couple of places.

Ah. Will fix(?) or else lobby the dictionary companies.

Thank you so much for the reviews!

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 7 Aug 2025 11:11:46 -0700

> I haven't read the meat of the patch, but I have some comments on the

Thanks for the review!

> around the argument list for each redirect, so it's clear that they go

I have two concerns:
- If I don't put parentheses around the list, the fat comma is
actively misleading.
- As far as I can tell, IPC::Run neither documents nor tests the
ability to pass a list here. (But the tests are a bit of a maze, so
please correct me if there is one.) My fear is that I'll be coupling
against an implementation detail if I write it that way.

So I'm leaning towards keeping it as-is, unless you know of a reason
that the list syntax is guaranteed to work, with the understanding
that it does diverge from what you authored in 19c6e92b1. But I don't
think any of those examples use filters, so I don't feel too bad about
the difference yet?

> old-fashioned, it's nicer to call it as a method on the filehandle.

That is much nicer; I'll do that.

> infrastructure in pg_regress.c.  Would it make sense to factor that out

Maybe if we got to rule-of-three, but I'd rather not make either
implementation compromise for the sake of the other. IMO, this is a
situation where a bad abstraction would be much costlier than the
duplication: TAP is lightweight, and I think the needs of a unit test
suite and the needs of a characterization test collector are very
different.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 7 Aug 2025 11:19:21 -0700

> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com

I am all for better testability at the function level, and it even
looks like 0002 solved a problem that stopped me from getting rid of
the Perl shim...

The "test without a running server" part also might have some overlap
with my fuzzing work. So I'll take a closer look later; thanks!

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 7 Aug 2025 13:04:17 -0700

> Thank you so much for the reviews!

Here is v4, with the feedback from both of you. 0001-0004 are planned
for backport; 0005 is slated for master only. Thanks again for the
reviews!

--Jacob
1:  c5cdccfe374 ! 1:  a515435d3b4 oauth: Remove stale events from the kqueue multiplexer
    @@ Commit message
         after drive_request(), before we return control to the client to wait.
     
         Suggested-by: Thomas Munro <[email-masked]>
    +    Co-authored-by: Thomas Munro <[email-masked]>
    +    Reviewed-by: Thomas Munro <[email-masked]>
    +    Backpatch-through: 18
    +    Discussion: https://postgr.es/m/[email-masked]
     
      ## src/interfaces/libpq-oauth/oauth-curl.c ##
     @@ src/interfaces/libpq-oauth/oauth-curl.c: register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx,
      #endif
      }
      
    -+/*-------
    ++/*
     + * If there is no work to do on any of the descriptors in the multiplexer, then
     + * this function must ensure that the multiplexer is not readable.
     + *
    -+ * As a motivating example, consider the following sequence of events:
    -+ * 1. libcurl tries to write data to the send buffer, but it fills up.
    -+ * 2. libcurl registers CURL_POLL_OUT on the socket and returns control to the
    -+ *    client to wait.
    -+ * 3. The kernel partially drains the send buffer. The socket becomes writable,
    -+ *    and the client wakes up and calls back into the flow.
    -+ * 4. libcurl continues writing data to the send buffer, but it fills up again.
    -+ *    The socket is no longer writable.
    -+ *
    -+ * At this point, an epoll-based mux no longer signals readiness, so nothing
    -+ * further needs to be done. But a kqueue-based mux will continue to signal
    -+ * "ready" until either the EVFILT_WRITE registration is dropped for the socket,
    -+ * or the old socket-writable event is read from the queue. Since Curl isn't
    -+ * guaranteed to do the former, we must do the latter here.
    ++ * Unlike epoll descriptors, kqueue descriptors only transition from readable to
    ++ * unreadable when kevent() is called and finds nothing, after removing
    ++ * level-triggered conditions that have gone away. We therefore need a dummy
    ++ * kevent() call after operations might have been performed on the monitored
    ++ * sockets or timer_fd. Any event returned is ignored here, but it also remains
    ++ * queued (being level-triggered) and leaves the descriptor readable. This is a
    ++ * no-op for epoll descriptors.
     + */
     +static bool
     +comb_multiplexer(struct async_ctx *actx)
    @@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
     +					 * Make sure that stale events don't cause us to come back
     +					 * early. (Currently, this can occur only with kqueue.) If
     +					 * this is forgotten, the multiplexer can get stuck in a
    -+					 * signalled state and we'll burn CPU cycles pointlessly.
    ++					 * signaled state and we'll burn CPU cycles pointlessly.
     +					 */
     +					if (!comb_multiplexer(actx))
     +						goto error_return;
2:  7725e0c173b ! 2:  a34be19f17f oauth: Ensure unused socket registrations are removed
    @@ Commit message
         number of events added, the number of events pulled off the queue, and
         the lengths of the kevent arrays.
     
    +    Reviewed-by: Thomas Munro <[email-masked]>
    +    Backpatch-through: 18
    +    Discussion: https://postgr.es/m/[email-masked]
    +
      ## src/interfaces/libpq-oauth/oauth-curl.c ##
     @@ src/interfaces/libpq-oauth/oauth-curl.c: register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx,
      
3:  6ccf7a5d156 ! 3:  7408778d579 oauth: Remove expired timers from the multiplexer
    @@ Commit message
         the timer was known to be set, but both implementations now use the
         kqueue logic.
     
    +    Reviewed-by: Thomas Munro <[email-masked]>
    +    Backpatch-through: 18
    +    Discussion: https://postgr.es/m/[email-masked]
    +
      ## src/interfaces/libpq-oauth/oauth-curl.c ##
     @@ src/interfaces/libpq-oauth/oauth-curl.c: set_timer(struct async_ctx *actx, long timeout)
      
4:  2be993b8f07 ! 4:  8241255e84c oauth: Track total call count during a client flow
    @@ Commit message
         future work to add TLS support to the oauth_validator test server should
         strengthen it as well.
     
    +    Backpatch-through: 18
    +    Discussion: https://postgr.es/m/[email-masked]
    +
      ## src/interfaces/libpq-oauth/oauth-curl.c ##
     @@ src/interfaces/libpq-oauth/oauth-curl.c: struct async_ctx
      	bool		user_prompted;	/* have we already sent the authz prompt? */
5:  50257bf32eb ! 5:  337124064f3 oauth: Add unit tests for multiplexer handling
    @@ Commit message
         suite for the socket and timer handling code. This is all based on TAP
         and driven by our existing Test::More infrastructure.
     
    +    Reviewed-by: Dagfinn Ilmari Mannsåker <[email-masked]>
    +    Discussion: https://postgr.es/m/[email-masked]
    +
      ## src/interfaces/libpq-oauth/Makefile ##
     @@ src/interfaces/libpq-oauth/Makefile: uninstall:
      	rm -f '$(DESTDIR)$(libdir)/$(stlib)'
    @@ src/interfaces/libpq-oauth/t/001_oauth.pl (new)
     +my $err = $builder->failure_output;
     +
     +IPC::Run::run ['oauth_tests'],
    -+  '>', IPC::Run::new_chunker, sub { print {$out} $_[0] },
    -+  '2>', IPC::Run::new_chunker, sub { print {$err} $_[0] }
    ++  '>', IPC::Run::new_chunker, sub { $out->print($_[0]) },
    ++  '2>', IPC::Run::new_chunker, sub { $err->print($_[0]) }
     +  or die "oauth_tests returned $?";
     
      ## src/interfaces/libpq-oauth/test-oauth-curl.c (new) ##

---

From: Thomas Munro <[email-masked]>
Date: Fri, 8 Aug 2025 22:38:57 +1200

> for backport; 0005 is slated for master only. Thanks again for the

LGTM.

---

From: Jacob Champion <[email-masked]>
Date: Fri, 8 Aug 2025 09:21:09 -0700

> LGTM.

Committed!

Thanks,
--Jacob

---

From: Dagfinn Ilmari Mannsåker <[email-masked]>
Date: Fri, 08 Aug 2025 21:07:42 +0100

> think any of those examples use filters, so I don't feel too bad about

When I said "not semantically meaningful" I meant that the parens don't
change what gets passed to the function.  In Perl, parens only serve to
override precedence and as visual grouping, they don't affect the
structure of the data at all.

To demonstrate:

    $ perl -MJSON::PP=encode_json -E 'say encode_json([1, 2, 3])'
    [1,2,3]

    $ perl -MJSON::PP=encode_json -E 'say encode_json([1 => (2, 3)])'
    [1,2,3]

> suite and the needs of a characterization test collector are very

Fair enough.

- ilmari

---

From: Jacob Champion <[email-masked]>
Date: Fri, 8 Aug 2025 13:52:09 -0700

>     $ perl -MJSON::PP=encode_json -E 'say encode_json([1 => (2, 3)])'

I swear, this language.

But:

    $ perl -MJSON::PP=encode_json -E 'say encode_json(1,2)'
    Too many arguments for JSON::PP::encode_json at -e line 1, near "2)
    $ perl -MJSON::PP=encode_json -E 'say encode_json((1,2))'
    2

So what's going on there? (Google is not very helpful for these sorts
of Perl problems; I don't even know how to describe this.)

I had to revert the test for unrelated reasons [1], so if this is
indeed guaranteed to be safe then I can make the change in my next
attempt.

Thanks!
--Jacob

[1] https://postgr.es/m/CAOYmi%2BnCkoh3zB%2BGkZad44%3DFNskwUg6F1kmuxqQZzng7Zgj5tw%40mail.gmail.com

---

From: Dagfinn Ilmari Mannsåker <[email-masked]>
Date: Fri, 08 Aug 2025 22:16:22 +0100

> of Perl problems; I don't even know how to describe this.)

That's because encode_json has a prototype[1], which changes how the
argument list is parsed: no longer just as a flat list of values like a
normal function.  Specifically, it has a prototype of '$', which means
it only takes one argument, which is evaluated in scalar context.  So
the first example is a syntax error, but in the second example the
parenthesised expression is the single argument.  Becuse it's in scalar
context, the comma is actually the scalar comma operator, not the list
element separator, so the return value is the right-hand side of the
comma (just like in C), not the length of the would-be list.
Onfusingly, they are both 2 here (which is why 1,2,3,... are bad values
to use when exploring presedence/context issues in Perl, sorry for doing
that in my example).  More clearly (fsvo):

    $ perl -MJSON::PP=encode_json -E 'say encode_json((11,12))'
    12

- ilmari

[1] https://perldoc.perl.org/perlsub#Prototypes

---

From: Jacob Champion <[email-masked]>
Date: Fri, 8 Aug 2025 14:31:49 -0700

> comma (just like in C), not the length of the would-be list.

ron-swanson-throws-away-computer.gif

Well, thank you for the explanation. I'll make that change.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 12 Aug 2025 15:11:44 -0700

> Well, thank you for the explanation. I'll make that change.

Done in v5.

v5-0001 is planned for backport to 18 once the freeze lifts. It
ensures that -lm is part of the link line for libpq-oauth, since the
module uses floor(). I probably wouldn't have ever noticed, except
that the new test executable, which uses the same link flags,
complained on Clang [1].

(In that thread, I incorrectly said the problem was with "Meson
animals". The Meson side is fine, and both alligator and bushmaster
use Autoconf, so I'm not sure how I ended up with that idea.)

v5-0002 should fix the more general buildfarm failure that caused the
revert. The farm finds the new t/ subdirectory and starts running Make
on src/interfaces/libpq-oauth directly, bypassing the skip logic in
src/interfaces/Makefile. So I've wrapped the "standard" top-level
targets that build and install things in a conditional. The targets
that clean things up have been left alone, at Tom's suggestion in [1].

Thanks,
--Jacob

[1] https://postgr.es/m/CAOYmi%2Bm%3DxY0P_uAzAP_884uF-GhQ3wrineGwc9AEnb6fYxVqVQ%40mail.gmail.com
1:  a515435d3b4 < -:  ----------- oauth: Remove stale events from the kqueue multiplexer
2:  a34be19f17f < -:  ----------- oauth: Ensure unused socket registrations are removed
3:  7408778d579 < -:  ----------- oauth: Remove expired timers from the multiplexer
4:  8241255e84c < -:  ----------- oauth: Track total call count during a client flow
-:  ----------- > 1:  c9962268ef0 oauth: Always link with -lm for floor()
5:  337124064f3 ! 2:  2e36b329c76 oauth: Add unit tests for multiplexer handling
    @@ Commit message
         suite for the socket and timer handling code. This is all based on TAP
         and driven by our existing Test::More infrastructure.
     
    +    This commit is a replay of 1443b6c0e, which was reverted due to
    +    buildfarm failures. Compared with that, this version protects the build
    +    targets in the Makefile with a with_libcurl conditional, and it tweaks
    +    the code style in 001_oauth.pl.
    +
         Reviewed-by: Dagfinn Ilmari Mannsåker <[email-masked]>
         Discussion: https://postgr.es/m/[email-masked]
     
      ## src/interfaces/libpq-oauth/Makefile ##
    -@@ src/interfaces/libpq-oauth/Makefile: uninstall:
    - 	rm -f '$(DESTDIR)$(libdir)/$(stlib)'
    - 	rm -f '$(DESTDIR)$(libdir)/$(shlib)'
    +@@ src/interfaces/libpq-oauth/Makefile: SHLIB_EXPORTS = exports.txt
    + # Disable -bundle_loader on macOS.
    + BE_DLLLIBS =
    + 
    +-# By default, a library without an SONAME doesn't get a static library, so we
    +-# add it to the build explicitly.
    +-all: all-lib all-static-lib
    +-
    + # Shared library stuff
    + include $(top_srcdir)/src/Makefile.shlib
    + 
    +@@ src/interfaces/libpq-oauth/Makefile: include $(top_srcdir)/src/Makefile.shlib
    + %_shlib.o: %.c %.o
    + 	$(CC) $(CFLAGS) $(CFLAGS_SL) $(CPPFLAGS) $(CPPFLAGS_SHLIB) -c $< -o $@
      
     +.PHONY: all-tests
     +all-tests: oauth_tests$(X)
    @@ src/interfaces/libpq-oauth/Makefile: uninstall:
     +oauth_tests$(X): test-oauth-curl.o oauth-utils.o $(WIN32RES) | submake-libpgport submake-libpq
     +	$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(SHLIB_LINK) -o $@
     +
    ++#
    ++# Top-Level Targets
    ++#
    ++# The existence of a t/ folder induces the buildfarm to run Make directly on
    ++# this subdirectory, bypassing the recursion skip in src/interfaces/Makefile.
    ++# Wrap the standard build targets in a with_libcurl conditional to avoid
    ++# building OAuth code on platforms that haven't requested it. (The "clean"-style
    ++# targets remain available.)
    ++#
    ++
    ++ifeq ($(with_libcurl), yes)
    ++
    ++# By default, a library without an SONAME doesn't get a static library, so we
    ++# add it to the build explicitly.
    ++all: all-lib all-static-lib
    ++
    + # Ignore the standard rules for SONAME-less installation; we want both the
    + # static and shared libraries to go into libdir.
    + install: all installdirs $(stlib) $(shlib)
    +@@ src/interfaces/libpq-oauth/Makefile: install: all installdirs $(stlib) $(shlib)
    + installdirs:
    + 	$(MKDIR_P) '$(DESTDIR)$(libdir)'
    + 
     +check: all-tests
     +	$(prove_check)
     +
     +installcheck: all-tests
     +	$(prove_installcheck)
     +
    ++endif # with_libcurl
    ++
    + uninstall:
    + 	rm -f '$(DESTDIR)$(libdir)/$(stlib)'
    + 	rm -f '$(DESTDIR)$(libdir)/$(shlib)'
    + 
      clean distclean: clean-lib
      	rm -f $(OBJS) $(OBJS_STATIC) $(OBJS_SHLIB)
     +	rm -f test-oauth-curl.o oauth_tests$(X)
    @@ src/interfaces/libpq-oauth/t/001_oauth.pl (new)
     +my $err = $builder->failure_output;
     +
     +IPC::Run::run ['oauth_tests'],
    -+  '>', IPC::Run::new_chunker, sub { $out->print($_[0]) },
    -+  '2>', IPC::Run::new_chunker, sub { $err->print($_[0]) }
    ++  '>' => (IPC::Run::new_chunker, sub { $out->print($_[0]) }),
    ++  '2>' => (IPC::Run::new_chunker, sub { $err->print($_[0]) })
     +  or die "oauth_tests returned $?";

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 12 Aug 2025 18:21:01 -0400

> [1] https://postgr.es/m/CAOYmi%2Bm%3DxY0P_uAzAP_884uF-GhQ3wrineGwc9AEnb6fYxVqVQ%40mail.gmail.com

I don't think that's quite going to work. The buildfarm will now get a 
"target not found"  in the without-curl case, I suspect. I think you'll 
need an alternative definition of the check target.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Jacob Champion <[email-masked]>
Date: Tue, 12 Aug 2025 15:25:47 -0700

> need an alternative definition of the check target.

We're still including Makefile.global, so the targets are still there;
they just won't add the libpq-oauth specific pieces. So `make check -C
src/interfaces/libpq-oauth` still works on my machine. Or have I
misunderstood the problem?

--Jacob

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 13 Aug 2025 07:34:38 -0400

> misunderstood the problem?

Oh, I see, and because the default definition of check doesn't call 
prove_check nothing is done. OK, sorry for the noise.

cheers

andrew

---

From: Jacob Champion <[email-masked]>
Date: Tue, 7 Apr 2026 09:05:29 -0700

> that clean things up have been left alone, at Tom's suggestion in [1].

To avoid surprising anyone: It seems like this suite has remained
stable and fast, so I plan to backpatch it to 18 to keep the coverage
up as the implementations in 19+ diverge.

Thanks,
--Jacob