public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dagfinn Ilmari Mannsåker <[email protected]>
To: Jacob Champion <[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: Fri, 08 Aug 2025 21:07:42 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAOYmi+=9J0nA7XfUCaEdjUdAb9+L6nLoJCEGRYXZbUtZ_o2k5Q@mail.gmail.com>
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]>
<CAOYmi+=9J0nA7XfUCaEdjUdAb9+L6nLoJCEGRYXZbUtZ_o2k5Q@mail.gmail.com>
Jacob Champion <[email protected]> writes:
> 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?
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]
>> 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.
Fair enough.
> --Jacob
- ilmari
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: <[email protected]>
* 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