Re: pg_upgrade and logical replication

On Tue, 5 Dec 2023 at 10:56, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> > I have made minor changes in the comments and code at various places.
> > See and let me know if you are not happy with the changes. I think
> > unless there are more suggestions or comments, we can proceed with
> > committing it.
>
> Yeah. I am planning to look more closely at what you have here, and
> it is going to take me a bit more time though (some more stuff planned
> for next CF, an upcoming conference and end/beginning-of-year
> vacations), but I think that targetting the beginning of next CF in
> January would be OK.
>
> Overall, I have the impression that the patch looks pretty solid, with
> a restriction in place for "init" and "ready" relations, while there
> are tests to check all the states that we expect. Seeing coverage
> about all that makes me a happy hacker.
>
> + * If retain_lock is true, then don't release the locks taken in this function.
> + * We normally release the locks at the end of transaction but in binary-upgrade
> + * mode, we expect to release those immediately.
>
> I think that this should be documented in pg_upgrade_support.c where
> the caller expects the locks to be released, and why these should be
> released. There is a risk that this comment becomes obsolete if
> AddSubscriptionRelState() with locks released is called in a different
> code path. Anyway, I am not sure to get why this is OK, or even
> necessary. It seems like a good practice to keep the locks on the
> subscription until the transaction that updates its state. If there's
> a specific reason explaining why that's better, the patch should tell
> why.

Added comments for this.

> + * However, this shouldn't be a problem as the upgrade ensures
> + * that all the transactions were replicated before upgrading the
> + * publisher.
> This wording looks a bit confusing to me, as "the upgrade" could refer
> to the upgrade of a subscriber, but what we want to tell is that the
> replay of the transactions is enforced when doing a publisher upgrade.
> I'd suggest something like "the upgrade of the publisher ensures that
> all the transactions were replicated before upgrading it".

Modified

> +my $result = $old_sub->safe_psql('postgres',
> + "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'i'");
> +is($result, qq(t), "Check that the table is in init state");
>
> Hmm. Not sure that this is safe. Shouldn't this be a
> poll_query_until(), polling that the state of the relation is what we
> want it to be after requesting a fresh of the publication on the
> subscriber?

This is not required as the table will be added in init state after
"Alter Subscription ... Refresh .." command itself.

Thanks for the comments, the attached v24 version patch has the
changes for the same.

Regards,
Vignesh