Re: Add two missing tests in 035_standby_logical_decoding.pl

Hi,

On 4/24/23 8:24 AM, Amit Kapila wrote:
> On Mon, Apr 24, 2023 at 11:24 AM Drouvot, Bertrand
> <bertranddrouvot.pg@gmail.com> wrote:
>>
> 
> Few comments:
> ============

Thanks for looking at it!

> 1.
> +$node_subscriber->init(allows_streaming => 'logical');
> +$node_subscriber->append_conf('postgresql.conf', 'max_replication_slots = 4');
> 
> Why do we need slots on the subscriber?
> 

Good point, it's not needed. I guess it has been missed during my initial patch clean up.

Fixed in V3 attached.

> 2.
> +# Speed up the subscription creation
> +$node_primary->safe_psql('postgres', "SELECT pg_log_standby_snapshot()");
> +
> +# Explicitly shut down psql instance gracefully - to avoid hangs
> +# or worse on windows
> +$psql_subscriber{subscriber_stdin} .= "\\q\n";
> +$psql_subscriber{run}->finish;
> +
> +# Insert some rows on the primary
> +$node_primary->safe_psql('postgres',
> + qq[INSERT INTO tab_rep select generate_series(1,10);]);
> +
> +$node_primary->wait_for_replay_catchup($node_standby);
> +
> +# To speed up the wait_for_subscription_sync
> +$node_primary->safe_psql('postgres', "SELECT pg_log_standby_snapshot()");
> +$node_subscriber->wait_for_subscription_sync($node_standby, 'tap_sub');
> 
> It is not clear to me why you need to do pg_log_standby_snapshot() twice.

That's because there is 2 logical slot creations that have the be done on the standby.

The one for the subscription:

"
CREATE_REPLICATION_SLOT "tap_sub" LOGICAL pgoutput (SNAPSHOT 'nothing')
"

And the one for the data sync:

"
CREATE_REPLICATION_SLOT "pg_16389_sync_16384_7225540800768250444" LOGICAL pgoutput (SNAPSHOT 'use')
"

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().

> 
> 3. Why do you need $psql_subscriber to be used in a different way
> instead of using safe_psql as is used for node_primary?
> 

Because safe_psql() would wait for activity on the primary without being able to launch
pg_log_standby_snapshot() on the primary while waiting. psql_subscriber() allows
to not wait synchronously.

Also adding a comment in V3 to explain why safe_psql() is not being used here.

Regards,

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