Re: pg_upgrade and logical replication

On Tue, Dec 5, 2023 at 10:56 AM Michael Paquier <michael@paquier.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.
>

It is to be consistent with other code paths in the upgrade. We
followed existing coding rules like what we do in
binary_upgrade_set_missing_value->SetAttrMissing(). The probable
theory is that during the upgrade we are not worried about concurrent
operations being blocked till the transaction ends. As in this
particular case, we know that the apply worker won't try to sync any
of those relations or a concurrent DDL won't try to remove it from the
pg_subscrition_rel. This point is not being explicitly commented
because of its similarity with the existing code.

>
> +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 safe because the init state should be marked by the "Alter
Subscription ... Refresh .." command itself. What exactly makes you
think that such a poll would be required?

-- 
With Regards,
Amit Kapila.