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. + * 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". +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? -- Michael
signature.asc
Description: PGP signature