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
Attachments:
fix_set_query_timer_restart.patchapplication/octet-stream; name=fix_set_query_timer_restart.patchDownload
diff --git a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
index 924b57ab21..764f0a5380 100644
--- a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
+++ b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
@@ -301,7 +301,7 @@ sub set_query_timer_restart
{
my $self = shift;
- $self->{query_timer_restart} = shift if @_;
+ $self->{query_timer_restart} = 1;
return $self->{query_timer_restart};
}
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
Attachments:
v2-0001-Fix-set_query_timer_restart-didn-t-work-without-a.patchapplication/octet-stream; name=v2-0001-Fix-set_query_timer_restart-didn-t-work-without-a.patchDownload
From ebbfa758ef09fc22db872c50c241db173c893b95 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <msawada@postgresql.orig>
Date: Wed, 29 Nov 2023 17:01:57 +0900
Subject: [PATCH v2] Fix set_query_timer_restart() didn't work without
argument.
The set_query_timer_restart() required an argument to define a value
to query_timer_restart, but none of the existing callers passes an
argument to this function.
This changes the function to set a value without an argument.
Backpatch through 16 where this was introduced.
Reviewed-by: Bharath Rupireddy, Tom Lane
Discussion: https://postgr.es/m/CAD21AoA0B6VKe_5A9nZi8i5umwSN-zJJuPVNht9DaOZ9SJumMA@mail.gmail.com
Backpatch-through: 16
---
src/test/perl/PostgreSQL/Test/BackgroundPsql.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
index 924b57ab21..764f0a5380 100644
--- a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
+++ b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
@@ -301,7 +301,7 @@ sub set_query_timer_restart
{
my $self = shift;
- $self->{query_timer_restart} = shift if @_;
+ $self->{query_timer_restart} = 1;
return $self->{query_timer_restart};
}
--
2.31.1
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