MIME-Version: 1.0
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>
 <87ldnvgkdq.fsf@wibble.ilmari.org>
In-Reply-To: <87ldnvgkdq.fsf@wibble.ilmari.org>
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Thu, 7 Aug 2025 11:11:46 -0700
Message-ID: 
 <CAOYmi+=9J0nA7XfUCaEdjUdAb9+L6nLoJCEGRYXZbUtZ_o2k5Q@mail.gmail.com>
Subject: Re: [PATCH] OAuth: fix performance bug with stuck multiplexer events
To: =?UTF-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
Cc: Thomas Munro <thomas.munro@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Daniel Gustafsson <daniel@yesql.se>,
	Peter Eisentraut <peter@eisentraut.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAOYmi%2B%3D9J0nA7XfUCaEdjUdAb9%2BL6nLoJCEGRYXZbUtZ_o2k5Q%40mail.gmail.com>
Precedence: bulk

On Thu, Aug 7, 2025 at 9:35=E2=80=AFAM Dagfinn Ilmari Manns=C3=A5ker
<ilmari@ilmari.org> 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 (=3D>) 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