Обсуждение: BackgroundPsql's set_query_timer_restart() may not work

Hi,

While adding some TAP tests, I realized that set_query_timer_restart()
in BackgroundPsql may not work. Specifically, it seems not to work
unless we pass an argument to the function. Here is the test script I
used:

use strict;
use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;

my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;

$PostgreSQL::Test::Utils::timeout_default = 5;
my $bg_psql = $node->background_psql('postgres', on_error_stop => 1);

$bg_psql->query_safe("select pg_sleep(3)");
$bg_psql->set_query_timer_restart();
$bg_psql->query_safe("select pg_sleep(3)");
$bg_psql->quit;
is(1,1,"dummy");

$node->stop;
done_testing();


If calling set_query_timer_restart() works properly, this test would
pass since we reset the query timeout before executing the second
pg_sleep(3). However, this test fail on my environment unless I use
set_query_timer_restart(1) (i.e. passing something to the function).

Currently authentication/t/001_password.pl is the sole user of
set_query_timer_restart() function. I think we should define a value
to query_timer_restart in set_query_timer_restart() function even if
no argument is passed, like the patch attached, or we should change
the caller to pass 1 to the function.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Nov 28, 2023 at 6:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Hi,
>
> While adding some TAP tests, I realized that set_query_timer_restart()
> in BackgroundPsql may not work. Specifically, it seems not to work
> unless we pass an argument to the function. Here is the test script I
> used:
>
> If calling set_query_timer_restart() works properly, this test would
> pass since we reset the query timeout before executing the second
> pg_sleep(3). However, this test fail on my environment unless I use
> set_query_timer_restart(1) (i.e. passing something to the function).

Right.

> Currently authentication/t/001_password.pl is the sole user of
> set_query_timer_restart() function. I think we should define a value
> to query_timer_restart in set_query_timer_restart() function even if
> no argument is passed, like the patch attached, or we should change
> the caller to pass 1 to the function.

It is added by commit 664d7575 and I agree that calling the function
without argument doesn't reset the query_timer_restart as intended.

A nitpick on the patch - how about honoring the passed-in parameter
with something like $self->{query_timer_restart} = 1 if !defined
$self->{query_timer_restart}; instead of just setting it to 1 (a value
other than undef) $self->{query_timer_restart} = 1;?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> A nitpick on the patch - how about honoring the passed-in parameter
> with something like $self->{query_timer_restart} = 1 if !defined
> $self->{query_timer_restart}; instead of just setting it to 1 (a value
> other than undef) $self->{query_timer_restart} = 1;?

I wondered about that too, but the evidence of existing callers is
that nobody cares.  If we did make the code do something like that,
(a) I don't think your fragment is right, and (b) we'd need to rewrite
the function's comment to explain it.  I'm not seeing a reason to
think it's worth spending effort on.

            regards, tom lane

On Tue, Nov 28, 2023 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> > A nitpick on the patch - how about honoring the passed-in parameter
> > with something like $self->{query_timer_restart} = 1 if !defined
> > $self->{query_timer_restart}; instead of just setting it to 1 (a value
> > other than undef) $self->{query_timer_restart} = 1;?
>
> I wondered about that too, but the evidence of existing callers is
> that nobody cares.  If we did make the code do something like that,
> (a) I don't think your fragment is right, and (b) we'd need to rewrite
> the function's comment to explain it.  I'm not seeing a reason to
> think it's worth spending effort on.

Hm. I don't mind doing just the $self->{query_timer_restart} = 1; like
in Sawada-san's patch.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Nov 29, 2023 at 4:30 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Nov 28, 2023 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> > > A nitpick on the patch - how about honoring the passed-in parameter
> > > with something like $self->{query_timer_restart} = 1 if !defined
> > > $self->{query_timer_restart}; instead of just setting it to 1 (a value
> > > other than undef) $self->{query_timer_restart} = 1;?
> >
> > I wondered about that too, but the evidence of existing callers is
> > that nobody cares.  If we did make the code do something like that,
> > (a) I don't think your fragment is right, and (b) we'd need to rewrite
> > the function's comment to explain it.  I'm not seeing a reason to
> > think it's worth spending effort on.

Agreed.

> Hm. I don't mind doing just the $self->{query_timer_restart} = 1; like
> in Sawada-san's patch.

Okay, I've attached the patch that I'm going to push through v16,
barring any objections.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wed, Nov 29, 2023 at 1:49 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Nov 29, 2023 at 4:30 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Tue, Nov 28, 2023 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> > > > A nitpick on the patch - how about honoring the passed-in parameter
> > > > with something like $self->{query_timer_restart} = 1 if !defined
> > > > $self->{query_timer_restart}; instead of just setting it to 1 (a value
> > > > other than undef) $self->{query_timer_restart} = 1;?
> > >
> > > I wondered about that too, but the evidence of existing callers is
> > > that nobody cares.  If we did make the code do something like that,
> > > (a) I don't think your fragment is right, and (b) we'd need to rewrite
> > > the function's comment to explain it.  I'm not seeing a reason to
> > > think it's worth spending effort on.
>
> Agreed.
>
> > Hm. I don't mind doing just the $self->{query_timer_restart} = 1; like
> > in Sawada-san's patch.
>
> Okay, I've attached the patch that I'm going to push through v16,
> barring any objections.

How about the commit message summary 'Fix TAP function
set_query_timer_restart() issue without argument.'? Also, it's good to
specify the commit 664d7575 that introduced the TAP function in the
commit message description.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Nov 29, 2023 at 7:48 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Nov 29, 2023 at 1:49 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Nov 29, 2023 at 4:30 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Tue, Nov 28, 2023 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > >
> > > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> > > > > A nitpick on the patch - how about honoring the passed-in parameter
> > > > > with something like $self->{query_timer_restart} = 1 if !defined
> > > > > $self->{query_timer_restart}; instead of just setting it to 1 (a value
> > > > > other than undef) $self->{query_timer_restart} = 1;?
> > > >
> > > > I wondered about that too, but the evidence of existing callers is
> > > > that nobody cares.  If we did make the code do something like that,
> > > > (a) I don't think your fragment is right, and (b) we'd need to rewrite
> > > > the function's comment to explain it.  I'm not seeing a reason to
> > > > think it's worth spending effort on.
> >
> > Agreed.
> >
> > > Hm. I don't mind doing just the $self->{query_timer_restart} = 1; like
> > > in Sawada-san's patch.
> >
> > Okay, I've attached the patch that I'm going to push through v16,
> > barring any objections.
>
> How about the commit message summary 'Fix TAP function
> set_query_timer_restart() issue without argument.'? Also, it's good to
> specify the commit 664d7575 that introduced the TAP function in the
> commit message description.

Thanks! I've incorporated your comment and pushed the patch.

Regards,


--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com