RE: pg_upgrade and logical replication

On Thursday, December 7, 2023 10:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> 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.

I also look into the patch and didn't find problems about the locking in
AddSubscriptionRelState.

About concurrency stuff, the lock on subscription object and
pg_subscription_rel only conflicts with ALTER/DROP SUBSCRIPTION which holds
AccessExclusiveLock lock, but since there are not concurrent ALTER
SUBSCRIPTION cmds during upgrade, so I think it's OK to release it earlier.

I also thought about the cache invalidation stuff as we modified the catalog
which will generate catcahe invalidateion. But the apply worker which build
cache based on the pg_subscription_rel is not running, and no concurrent
ALTER/DROP SUBSCRIPTION cmds will be executed, so it looks OK as well.

Best Regards,
Hou zj