On Wed, 10 May 2023 at 13:29, Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for the v5-0001 patch code. > > ====== > General > > 1. ALTER SUBSCRIPTION name ADD TABLE (relid = XYZ, state = 'x' [, lsn = > 'X/Y']) > > I was a bit confused by this relation 'state' mentioned in multiple > places. IIUC the pg_upgrade logic is going to reject anything with a > non-READY (not 'r') state anyhow, so what is the point of having all > the extra grammar/parse_subscription_options etc to handle setting the > state when only possible value must be 'r'? >
This command has been removed, this code has been removed > > 2. state V relstate > > I still feel code readbility suffers a bit by calling some fields/vars > a generic 'state' instead of the more descriptive 'relstate'. Maybe > it's just me. > > Previously commented same (see [1]#3, #4, #5) Few of the code has been removed, I have modified wherever possible > ====== > doc/src/sgml/ref/pgupgrade.sgml > > 3. > + <para> > + Fully preserve the logical subscription state if any. That includes > + the underlying replication origin with their remote LSN and the list > of > + relations in each subscription so that replication can be simply > + resumed if the subscriptions are reactivated. > + </para> > > I think the "if any" part is not necessary. If you remove those words, > then the rest of the sentence can be simplified. > > SUGGESTION > Fully preserve the logical subscription state, which includes the > underlying replication origin's remote LSN, and the list of relations > in each subscription. This allows replication to simply resume when > the subscriptions are reactivated. > This has been removed now. > > 4. > + <para> > + If this option isn't used, it is up to the user to reactivate the > + subscriptions in a suitable way; see the subscription part in <xref > + linkend="pg-dump-notes"/> for more information. > + </para> > > The link still renders strangely as previously reported (see [1]#2b). > This has been removed now > > 5. > + <para> > + If this option is used and any of the subscription on the old cluster > + has an unknown <varname>remote_lsn</varname> (0/0), or has any > relation > + in a state different from <literal>r</literal> (ready), the > + <application>pg_upgrade</application> run will error. > + </para> > > 5a. > /subscription/subscriptions/ Modified > 5b > "has any relation in a state different from r" --> "has any relation > with state other than r" Modified slightly > ====== > src/backend/commands/subscriptioncmds.c > > 6. > + if (strlen(state_str) != 1) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("invalid relation state: %s", state_str))); > > Is this relation state validation overly simplistic, by only checking > for length 1? Shouldn't this just be asserting the relstate must be > 'r'? This code has been removed > ====== > src/bin/pg_dump/pg_dump.c > > 7. getSubscriptionTables > > +/* > + * getSubscriptionTables > + * get information about the given subscription's relations > + */ > +void > +getSubscriptionTables(Archive *fout) > +{ > + SubscriptionInfo *subinfo; > + SubRelInfo *rels = NULL; > + PQExpBuffer query; > + PGresult *res; > + int i_srsubid; > + int i_srrelid; > + int i_srsubstate; > + int i_srsublsn; > + int i_nrels; > + int i, > + cur_rel = 0, > + ntups, > + last_srsubid = InvalidOid; > > Why some above are single int declarations and some are compound int > declarations? Why not make them all consistent? Modified > ~ > > 8. > + appendPQExpBuffer(query, "SELECT srsubid, srrelid, srsubstate, srsublsn," > + " count(*) OVER (PARTITION BY srsubid) AS nrels" > + " FROM pg_subscription_rel" > + " ORDER BY srsubid"); > > Should this SQL be schema-qualified like pg_catalog.pg_subscription_rel? Modified > ~ > > 9. > + for (i = 0; i < ntups; i++) > + { > + int cur_srsubid = atooid(PQgetvalue(res, i, i_srsubid)); > > Should 'cur_srsubid' be declared Oid to match the atooid? Modified > ~~~ > > 10. getSubscriptions > > + if (PQgetisnull(res, i, i_suboriginremotelsn)) > + subinfo[i].suboriginremotelsn = NULL; > + else > + subinfo[i].suboriginremotelsn = > + pg_strdup(PQgetvalue(res, i, i_suboriginremotelsn)); > + > + /* > + * For now assume there's no relation associated with the > + * subscription. Later code might update this field and allocate > + * subrels as needed. > + */ > + subinfo[i].nrels = 0; > > The wording "For now assume there's no" kind of gives an ambiguous > interpretation for this comment. IMO it sounds like this is the > "current" logic but some future PG version may behave differently - I > don't think that is the intended meaning at all. > > SUGGESTION. > Here we just initialize nrels to say there are 0 relations associated > with the subscription. If necessary, subsequent logic will update this > field and allocate the subrels. This part of logic has been removed now as it is no more required > ~~~ > > 11. dumpSubscription > > + for (i = 0; i < subinfo->nrels; i++) > + { > + appendPQExpBuffer(query, "\nALTER SUBSCRIPTION %s ADD TABLE " > + "(relid = %u, state = '%c'", > + qsubname, > + subinfo->subrels[i].srrelid, > + subinfo->subrels[i].srsubstate); > + > + if (subinfo->subrels[i].srsublsn[0] != '\0') > + appendPQExpBuffer(query, ", LSN = '%s'", > + subinfo->subrels[i].srsublsn); > + > + appendPQExpBufferStr(query, ");"); > + } > > I previously asked ([1]#11) about how can this ALTER SUBSCRIPTION > TABLE code happen unless 'preserve_subscriptions' is true, and you > confirmed "It indirectly is, as in that case subinfo->nrels is > guaranteed to be 0. I just tried to keep the code simpler and avoid > too many nested conditions." I have added the same check used that is used to get the subscription tables to avoid confusion. > ~ > > If you are worried about too many nested conditions then a simple > Assert(dopt->preserve_subscriptions); might be good to have here. > > ====== > src/bin/pg_upgrade/check.c > > 12. check_and_dump_old_cluster > > + /* PG 10 introduced subscriptions. */ > + if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1000 && > + user_opts.preserve_subscriptions) > + { > + check_for_subscription_state(&old_cluster); > + } > > 12a. > All the other checks in this function seem to be in decreasing order > of PG version so maybe this check should be moved to follow that same > pattern. Modified > ~ > > 12b. > Also won't it be better to give some error or notice of some kind if > the option/version are incompatible? I think this was mentioned in a > previous review. > > e.g. > > if (user_opts.preserve_subscriptions) > { > if (GET_MAJOR_VERSION(old_cluster.major_version) < 1000) > <pg_log or pg_fatal goes here...>; > check_for_subscription_state(&old_cluster); > } This has been removed now > ~~~ > > 13. check_for_subscription_state > > + for (int i = 0; i < ntup; i++) > + { > + is_error = true; > + pg_log(PG_WARNING, > + "\nWARNING: subscription \"%s\" has an invalid remote_lsn", > + PQgetvalue(res, 0, 0)); > + } > > 13a. > This WARNING does not mention the database, but a similar warning > later about the non-ready state does mention the database. Probably > they should be consistent. Modified > ~ > > 13b. > Something seems amiss. Here the is_error is assigned true; But later > when you test is_error that is for logging the ready-state problem. > Isn't there another missing pg_fatal for this invalid remote_lsn case? Modified > ====== > src/bin/pg_upgrade/option.c > > 14. usage > > + printf(_(" --preserve-subscription-state preserve the subscription > state fully\n")); > > Why say "fully"? How is "preserve the subscription state fully" > different to "preserve the subscription state" from the user's POV? This has been removed now These are handled as part of v7 posted at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm1ZrbHaWpJwwNhDTJocRKWd3rEkgJazuDdZ9Z-WdvonFg%40mail.gmail.com Regards, Vignesh