Re: pg_upgrade and logical replication

On Thu, 30 Nov 2023 at 13:35, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Nov 29, 2023 at 3:02 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
>
> In general, the test cases are a bit complex to understand, so, it
> will be difficult to enhance these later. The complexity comes from
> the fact that one upgrade test is trying to test multiple things (a)
> Enabled/Disabled subscriptions; (b) relation states 'i' and 'r' are
> preserved after the upgrade. (c) rows from non-refreshed tables are
> not copied, etc. I understand that you may want to cover as many
> things possible in one test to have fewer upgrade tests which could
> save some time but I think it makes the test somewhat difficult to
> understand and enhance. Can we try to split it such that (a) and (b)
> are tested in one test and others could be separated out?

Yes, I had tried to combine a few tests as it was taking more time to
run. I have refactored the tests by removing tab_not_upgraded1 related
test which is more of a logical replication test, adding more
comments, removing intermediate select count checks. So now we have
test1) which checks for upgrade with subscriber having table in
init/ready state, test2) Check that the data inserted to the publisher
when the subscriber is down will be replicated to the new subscriber
once the new subscriber is started (these are done as continuation of
the previous test). test3) Check that pg_upgrade fails when
max_replication_slots configured in the new cluster is less than the
number of subscriptions in the old cluster. test4) Check upgrade fails
with old instance with relation in 'd' datasync(invalid) state and
missing replication origin.
In test4 I have combined both datasync relation state and missing
replication origin as the validation for both is in the same file. I
felt the readability is better now, do let me know if any of the test
is still difficult to understand.

> Few other comments:
> ===================
> 1.
> +$old_sub->safe_psql('postgres',
> + "CREATE SUBSCRIPTION regress_sub CONNECTION '$connstr' PUBLICATION
> regress_pub"
> +);
> +
> +$old_sub->wait_for_subscription_sync($publisher, 'regress_sub');
> +
> +# After the above wait_for_subscription_sync call the table can be either in
> +# 'syncdone' or in 'ready' state. Now wait till the table reaches
> 'ready' state.
> +my $synced_query =
> + "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'r'";
> +$old_sub->poll_query_until('postgres', $synced_query)
> + or die "Timed out while waiting for the table to reach ready state";
>
> Can the table be in 'i' state after above test? If not, then above
> comment is misleading.

This part of test is to get the table in ready state/ modified the
comments appropriately

> 2.
> +# ------------------------------------------------------
> +# Check that pg_upgrade is successful when all tables are in ready or in
> +# init state.
> +# ------------------------------------------------------
> +$publisher->safe_psql('postgres',
> + "INSERT INTO tab_upgraded1 VALUES (generate_series(2,50), 'before
> initial sync')"
> +);
> +$publisher->wait_for_catchup('regress_sub');
>
> The previous comment applies to this one as well.

I have removed this comment and moved it before the upgrade command as
it is more appropriate there.

> 3.
> +$publisher->safe_psql('postgres', "CREATE PUBLICATION regress_pub1");
> +$old_sub->safe_psql('postgres',
> + "CREATE SUBSCRIPTION regress_sub1 CONNECTION '$connstr' PUBLICATION
> regress_pub1"
> +);
> +$old_sub->wait_for_subscription_sync($publisher, 'regress_sub1');
> +
> +# Change configuration to prepare a subscription table in init state
> +$old_sub->append_conf('postgresql.conf',
> + "max_logical_replication_workers = 0");
> +$old_sub->restart;
> +
> +# Add tab_upgraded2 to the publication. Now publication has tab_upgraded1
> +# and tab_upgraded2 tables.
> +$publisher->safe_psql('postgres',
> + "ALTER PUBLICATION regress_pub ADD TABLE tab_upgraded2");
> +
> +$old_sub->safe_psql('postgres',
> + "ALTER SUBSCRIPTION regress_sub REFRESH PUBLICATION");
>
> These two cases for Create and Alter look confusing. I think it would
> be better if Alter's case is moved before the comment: "Check that
> pg_upgrade is successful when all tables are in ready or in init
> state.".

I have added more comments to make it clear now. I have moved the
"check that pg_upgrade is successful when all tables ..." before the
upgrade command to be more clearer. Added comment "Pre-setup for
preparing subscription table in init state. Add tab_upgraded2 to the
publication." and "# The table tab_upgraded2 will be in init state as
the subscriber configuration for max_logical_replication_workers is
set to 0."

> 4.
> +# Insert a row in tab_upgraded1 and tab_not_upgraded1 publisher table while
> +# it's down.
> +insert_line_at_pub('while old_sub is down');
>
> Isn't sub routine insert_line_at_pub() inserts in all three tables? If
> so, then the above comment seems to be wrong and I think it is better
> to explain the intention of this insert.

Modified

> 5.
> +my $result =
> + $new_sub->safe_psql('postgres',
> + "SELECT subenabled FROM pg_subscription WHERE subname = 'regress_sub'");
> +is($result, qq(f),
> + "check that the subscriber that was disable on the old subscriber
> should be disabled in the new subscriber"
> +);
> +$result =
> + $new_sub->safe_psql('postgres',
> + "SELECT subenabled FROM pg_subscription WHERE subname = 'regress_sub1'");
> +is($result, qq(t),
> + "check that the subscriber that was enabled on the old subscriber
> should be enabled in the new subscriber"
> +);
>
> Can't the above be tested with a single query?

Modified

> 6.
> +$new_sub->safe_psql('postgres', "DROP SUBSCRIPTION regress_sub1");
> +
> +# Subscription relations should be preserved. The upgraded subscriber
> won't know
> +# about 'tab_not_upgraded1' because the subscription is not yet refreshed.
> +$result =
> + $new_sub->safe_psql('postgres', "SELECT count(*) FROM pg_subscription_rel");
> +is($result, qq(2),
> + "there should be 2 rows in pg_subscription_rel(representing
> tab_upgraded1 and tab_upgraded2)"
> +);
>
> Here the DROP SUBSCRIPTION looks confusing. Let's try to move it after
> the verification of objects after the upgrade.

I have removed this now, no need to move it to down as we will be
stopping the newsub server at the end of this test and this newsub
will not be used later.

> 7.
> 1.
> +sub insert_line_at_pub
> +{
> + my $payload = shift;
> +
> + foreach ("tab_upgraded1", "tab_upgraded2", "tab_not_upgraded1")
> + {
> + $publisher->safe_psql('postgres',
> + "INSERT INTO " . $_ . " (val) VALUES('$payload')");
> + }
> +}
> +
> +# Initial setup
> +foreach ("tab_upgraded1", "tab_upgraded2", "tab_not_upgraded1")
> +{
> + $publisher->safe_psql('postgres',
> + "CREATE TABLE " . $_ . " (id serial, val text)");
> + $old_sub->safe_psql('postgres',
> + "CREATE TABLE " . $_ . " (id serial, val text)");
> +}
> +insert_line_at_pub('before initial sync');
>
> This makes the test slightly difficult to understand and we don't seem
> to achieve much by writing sub routines.

Removed the subroutines.

The changes for the same is available at:
https://www.postgresql.org/message-id/CALDaNm37E4tmSZd%2Bk1ixtKevX3eucmhdOnw4pGmykZk4C1Nm4Q%40mail.gmail.com

Regards,
Vignesh