Re: Add two missing tests in 035_standby_logical_decoding.pl

On Tue, 25 Apr 2023 at 12:51, Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On 4/25/23 6:23 AM, Amit Kapila wrote:
> > On Mon, Apr 24, 2023 at 3:36 PM Drouvot, Bertrand
> > <bertranddrouvot.pg@gmail.com> wrote:
> >>
> >> Without the second "pg_log_standby_snapshot()" then wait_for_subscription_sync() would be waiting
> >> some time on the poll for "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');"
> >>
> >> Adding a comment in V3 to explain the need for the second pg_log_standby_snapshot().
> >>
> >
> > Won't this still be unpredictable because it is possible that the
> > tablesync worker may take more time to get launched or create a
> > replication slot? If that happens after your second
> > pg_log_standby_snapshot() then wait_for_subscription_sync() will be
> > hanging.
>
> Oh right, that looks like a possible scenario.
>
> > Wouldn't it be better to create a subscription with
> > (copy_data = false) to make it predictable and then we won't need
> > pg_log_standby_snapshot() to be performed twice?
> >
> > If you agree with the above suggestion then you probably need to move
> > wait_for_subscription_sync() before Insert.
> >
>
> I like that idea, thanks! Done in V4 attached.
>
> Not related to the above corner case, but while re-reading the patch I also added:
>
> "
> $node_primary->wait_for_replay_catchup($node_standby);
> "
>
> between the publication creation on the primary and the subscription to the standby
> (to ensure the publication gets replicated before we request for the subscription creation).

Thanks for the updated patch.
Few comments:
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);

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);

Regards,
Vignesh