Re: pg_upgrade and logical replication

On Thu, Dec 7, 2023 at 7:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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).
>

True, but we have kept it that way from the consistency point of view
as well. We can change it if you think otherwise.

--
With Regards,
Amit Kapila.