since-v3.diff.txt

text/plain

Filename: since-v3.diff.txt
Type: text/plain
Part: 0
Message: Re: [PATCH] OAuth: fix performance bug with stuck multiplexer events
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 <thomas.munro@gmail.com>
    +    Co-authored-by: Thomas Munro <thomas.munro@gmail.com>
    +    Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
    +    Backpatch-through: 18
    +    Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
     
      ## 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 <thomas.munro@gmail.com>
    +    Backpatch-through: 18
    +    Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
    +
      ## 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 <thomas.munro@gmail.com>
    +    Backpatch-through: 18
    +    Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
    +
      ## 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/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
    +
      ## 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 <ilmari@ilmari.org>
    +    Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
    +
      ## 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) ##