On Wed, 26 Apr 2023 at 13:45, Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>
> 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.
ok, that way it will be consistent across others too.
> > 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.
>
Thanks for fixing this.
There was one typo in the commit message, subscribtion should be
subscription, the rest of the changes looks good to me:
Subject: [PATCH v5] Add subscribtion to the standby test in
035_standby_logical_decoding.pl
Adding one test, to verify that subscribtion to the standby is possible.
Regards,
Vignesh