Re: pg_upgrade and logical replication

On Tue, Dec 5, 2023 at 6:37 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.

It seems no problem to me with releasing locks early, I'm not sure how
much it helps in better concurrency as it acquires lower level locks
such as AccessShareLock and RowExclusiveLock though (SetAttrMissing()
acquires AccessExclusiveLock on the table on the other hand).

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com