On Thu, Dec 7, 2023 at 8:15 PM vignesh C <vignes...@gmail.com> wrote: > > On Tue, 5 Dec 2023 at 10:56, Michael Paquier <mich...@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. > > 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