Re: Add two missing tests in 035_standby_logical_decoding.pl

From: "Drouvot, Bertrand" <bertranddrouvot(dot)pg(at)gmail(dot)com>
To: vignesh C <vignesh21(at)gmail(dot)com>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Add two missing tests in 035_standby_logical_decoding.pl
Date: 2023-04-26 08:14:18
Message-ID: 4ec4a071-8f0a-5536-58d5-cb815ef0905f@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 4/26/23 6:06 AM, vignesh C wrote:
> On Tue, 25 Apr 2023 at 12:51, Drouvot, Bertrand
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> Thanks for the updated patch.
> Few comments:

Thanks for looking at it!

> 1) subscriber_stdout and subscriber_stderr are not required for this
> test case, we could remove it, I was able to remove those variables
> and run the test successfully:
> +$node_subscriber->start;
> +
> +my %psql_subscriber = (
> + 'subscriber_stdin' => '',
> + 'subscriber_stdout' => '',
> + 'subscriber_stderr' => '');
> +$psql_subscriber{run} = IPC::Run::start(
> + [ 'psql', '-XA', '-f', '-', '-d',
> $node_subscriber->connstr('postgres') ],
> + '<',
> + \$psql_subscriber{subscriber_stdin},
> + '>',
> + \$psql_subscriber{subscriber_stdout},
> + '2>',
> + \$psql_subscriber{subscriber_stderr},
> + $psql_timeout);
>
> I ran it like:
> my %psql_subscriber = (
> 'subscriber_stdin' => '');
> $psql_subscriber{run} = IPC::Run::start(
> [ 'psql', '-XA', '-f', '-', '-d', $node_subscriber->connstr('postgres') ],
> '<',
> \$psql_subscriber{subscriber_stdin},
> $psql_timeout);
>

Not using the 3 std* is also the case for example in 021_row_visibility.pl and 032_relfilenode_reuse.pl
where the "stderr" is set but does not seem to be used.

I don't think that's a problem to keep them all and I think it's better to have
them re-directed to dedicated places.

> 2) Also we have changed the default timeout here, why is this change required:
> my $node_cascading_standby =
> PostgreSQL::Test::Cluster->new('cascading_standby');
> +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
> my $default_timeout = $PostgreSQL::Test::Utils::timeout_default;
> +my $psql_timeout = IPC::Run::timer(2 * $default_timeout);

I think I used 021_row_visibility.pl as an example. But agree there is
others .pl that are using the timeout_default as the psql_timeout and that
the default is enough in our case. So, using the default in V5 attached.

Regards,

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

Attachment Content-Type Size
v5-0001-Add-subscribtion-to-the-standby-test-in-035_stand.patch text/plain 6.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Richard Guo 2023-04-26 08:33:39 Re: [pg_rewind] use the passing callback instead of global function
Previous Message Peter Eisentraut 2023-04-26 07:38:40 Re: run pgindent on a regular basis / scripted manner