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

Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> writes:

> On Thu, Aug 7, 2025 at 9:35 AM Dagfinn Ilmari Mannsåker
> <ilmari(at)ilmari(dot)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 (=>) 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