Re: pg_upgrade and logical replication
От | vignesh C |
---|---|
Тема | Re: pg_upgrade and logical replication |
Дата | |
Msg-id | CALDaNm0xbac8Xzu7f-Eq6MkyGyX_Bvnmw-os8zta9d_SzYd7aA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: pg_upgrade and logical replication (Peter Smith <smithpb2250@gmail.com>) |
Ответы |
Re: pg_upgrade and logical replication
(Peter Smith <smithpb2250@gmail.com>)
|
Список | pgsql-hackers |
On Mon, 20 Nov 2023 at 10:44, Peter Smith <smithpb2250@gmail.com> wrote: > > Here are some review comments for patch v15-0001 > > ====== > src/bin/pg_dump/pg_dump.c > > 1. getSubscriptions > > + if (fout->remoteVersion >= 170000) > + appendPQExpBufferStr(query, "o.remote_lsn AS suboriginremotelsn\n"); > + else > + appendPQExpBufferStr(query, "NULL AS suboriginremotelsn\n"); > + > > There should be preceding spaces in those append strings to match the > other ones. Modified > ~~~ > > 2. dumpSubscriptionTable > > +/* > + * dumpSubscriptionTable > + * Dump the definition of the given subscription table mapping. This will be > + * used only in binary-upgrade mode. > + */ > +static void > +dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo) > +{ > + DumpOptions *dopt = fout->dopt; > + SubscriptionInfo *subinfo = subrinfo->subinfo; > + PQExpBuffer query; > + char *tag; > + > + /* Do nothing in data-only dump */ > + if (dopt->dataOnly) > + return; > + > + Assert(fout->dopt->binary_upgrade || fout->remoteVersion >= 170000); > > The function comment says this is only for binary-upgrade mode, so why > does the Assert use || (OR)? Added comments > ====== > src/bin/pg_upgrade/check.c > > 3. check_and_dump_old_cluster > > + /* > + * Subscription dependencies can be migrated since PG17. See comments atop > + * get_old_cluster_subscription_count(). > + */ > + if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700) > + check_old_cluster_subscription_state(&old_cluster); > + > > Should this be combined with the other adjacent check so there is only > one "if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)" > needed? Modified > ~~~ > > 4. check_new_cluster > > check_new_cluster_logical_replication_slots(); > + > + check_new_cluster_subscription_configuration(); > > When checking the old cluster, the subscription was checked before the > slots, but here for the new cluster, the slots are checked before the > subscription. Maybe it makes no difference but it might be tidier to > do these old/new checks in the same order. Modified > ~~~ > > 5. check_new_cluster_logical_replication_slots > > - /* Quick return if there are no logical slots to be migrated. */ > + /* Quick return if there are no logical slots to be migrated */ > > Change is not relevant for this patch. Removed it > ~~~ > > 6. > > + res = executeQueryOrDie(conn, "SELECT setting FROM pg_settings " > + "WHERE name IN ('max_replication_slots') " > + "ORDER BY name DESC;"); > > Using IN and ORDER BY in this SQL seems unnecessary when you are only > searching for one name. Modified > ====== > src/bin/pg_upgrade/info.c > > 7. statics > > - > +static void get_old_cluster_subscription_count(DbInfo *dbinfo); > > This change also removes an existing blank line -- not sure if that > was intentional Modified > ~~~ > > 8. > @@ -365,7 +369,6 @@ get_template0_info(ClusterInfo *cluster) > PQfinish(conn); > } > > - > /* > * get_db_infos() > * > > This blank line change (before get_db_infos) should not be part of this patch. Modified > ~~~ > > 9. get_old_cluster_subscription_count > > It seems a slightly misleading function name because this is a PER-DB > count, not a cluster count. Modified > ~~~ > > > 10. > + /* Subscriptions can be migrated since PG17. */ > + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600) > + return; > > IMO it is better to compare < 1700 instead of <= 1600. It keeps the > code more aligned with the comment. Modified > ~~~ > > 11. count_old_cluster_subscriptions > > +/* > + * count_old_cluster_subscriptions() > + * > + * Returns the number of subscription for all databases. > + * > + * Note: this function always returns 0 if the old_cluster is PG16 and prior > + * because we gather subscriptions only for cluster versions greater than or > + * equal to PG17. See get_old_cluster_subscription_count(). > + */ > +int > +count_old_cluster_subscriptions(void) > +{ > + int nsubs = 0; > + > + for (int dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++) > + nsubs += old_cluster.dbarr.dbs[dbnum].nsubs; > + > + return nsubs; > +} > > 11a. > /subscription/subscriptions/ Modified > ~ > > 11b. > The code is now consistent with the slots code which looks good. OTOH > I thought that 'pg_catalog.pg_subscription' is shared across all > databases of the cluster, so isn't this code inefficient to be > querying again and again for every database (if there are many of > them) instead of just querying 1 time only for the whole cluster? My earlier version was like that, changed it to keep the code consistent to logical replication slots. > ====== > src/bin/pg_upgrade/t/004_subscription.pl > > 12. > It is difficult to keep track of all the tables (upgraded and not > upgraded) at each step of these tests. Maybe the comments can be more > explicit along the way. e.g > > BEFORE > +# Add tab_not_upgraded1 to the publication > > SUGGESTION > +# Add tab_not_upgraded1 to the publication. Now publication has <blah blah> > > and > > BEFORE > +# Subscription relations should be preserved > > SUGGESTION > +# Subscription relations should be preserved. The upgraded won't know > about 'tab_not_upgraded1' because <blah blah> > > etc. Modified > ~~~ > > 13. > +$result = > + $new_sub->safe_psql('postgres', "SELECT count(*) FROM tab_not_upgraded1"); > +is($result, qq(0), > + "no change in table tab_not_upgraded1 afer enable subscription which > is not part of the publication" > > /afer/after/ Modified > ~~~ > > 14. > +# ------------------------------------------------------ > +# Check that pg_upgrade refuses to run a) if there's a subscription with tables > +# in a state different than 'r' (ready), 'i' (init) and 's' (synchronized) > +# and/or b) if the subscription does not have a replication origin. > +# ------------------------------------------------------ > > 14a, > /does not have a/has no/ Modified > ~ > > 14b. > Maybe put a) and b) on newlines to be more readable. Modified The attached v16 version patch has the changes for the same. Regards, Vignesh
Вложения
В списке pgsql-hackers по дате отправления: