Hi,
On 4/26/23 6:06 AM, vignesh C wrote:
> On Tue, 25 Apr 2023 at 12:51, Drouvot, Bertrand
> <bertranddrouvot.pg@gmail.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
