On Thu, Dec 7, 2023 at 8:15 PM vignesh C <[email protected]> wrote:
>
> On Tue, 5 Dec 2023 at 10:56, Michael Paquier <[email protected]> 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.
Thank you for updating the patch.
Here are some minor comments:
+ if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relid)))
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("relation %u does not exist", relid));
+
I think the error code should be ERRCODE_UNDEFINED_TABLE, and the
error message should be something like "relation with OID %u does not
exist". Or we might not need such checks since an undefined-object
error is caught by relation_open()?
---
+ /* Fetch the existing tuple. */
+ tup = SearchSysCache2(SUBSCRIPTIONNAME, MyDatabaseId,
+ CStringGetDatum(subname));
+ if (!HeapTupleIsValid(tup))
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("subscription \"%s\" does not
exist", subname));
+
+ form = (Form_pg_subscription) GETSTRUCT(tup);
+ subid = form->oid;
The above code can be replaced with "get_subscription_oid(subname,
false)". binary_upgrade_replorigin_advance() has the same code.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
