public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: Dagfinn Ilmari Mannsåker <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Subject: Re: [PATCH] OAuth: fix performance bug with stuck multiplexer events
Date: Thu, 7 Aug 2025 11:11:46 -0700
Message-ID: <CAOYmi+=9J0nA7XfUCaEdjUdAb9+L6nLoJCEGRYXZbUtZ_o2k5Q@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com>
<CAOYmi+mRhhzGUvrcdickepAnsdaGbqhNcboNe4-YvgtkSzGNbQ@mail.gmail.com>
<CA+hUKGLyS-GK_rkENaVbFVTa4VJ+reJxWtt+q4gmgNUXhptfYA@mail.gmail.com>
<CAOYmi+k1q3feeMxfbaJA=+hx+XnOFQA0z2JU+0igA7fTUZTmoA@mail.gmail.com>
<CA+hUKG+H1gwDh96jn5jB6Q3HyXrSC9x2y=uQJAthT8NLs6GN_Q@mail.gmail.com>
<CAOYmi+n1xRNCDnwZzigXVk8V=+sr7ZzuGpJ0tAyozX-zQT19Gg@mail.gmail.com>
<CAOYmi+mvk8Y7btYJhBzOGiNTY3cCpYZKjhA4-TP2Lkb=zOr4oQ@mail.gmail.com>
<[email protected]>
On Thu, Aug 7, 2025 at 9:35 AM Dagfinn Ilmari Mannsåker
<[email protected]> wrote:
> I haven't read the meat of the patch, but I have some comments on the
> tests:
Thanks for the review!
> > +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.
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?
> Also, indirect object syntax (print {$fh} ...) is ugly and
> old-fashioned, it's nicer to call it as a method on the filehandle.
That is much nicer; I'll do that.
> 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?
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
view thread (25+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] OAuth: fix performance bug with stuck multiplexer events
In-Reply-To: <CAOYmi+=9J0nA7XfUCaEdjUdAb9+L6nLoJCEGRYXZbUtZ_o2k5Q@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox