From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | vignesh C <vignesh21(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Subject: | Re: pg_upgrade and logical replication |
Date: | 2023-11-22 01:17:30 |
Message-ID: | CAHut+PtrEjVPDGHC_+fghJYWiwrnNR0Zz0QziMLPKDZcmBb8ag@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for addressing my past review comments.
Here are some more review comments for patch v16-0001
======
doc/src/sgml/ref/pgupgrade.sgml
1.
+ <para>
+ Create all the new tables that were created in the publication during
+ upgrade and refresh the publication by executing
+ <link linkend="sql-altersubscription"><command>ALTER
SUBSCRIPTION ... REFRESH PUBLICATION</command></link>.
+ </para>
"Create all ... that were created" sounds a bit strange.
SUGGESTION (maybe like this or similar?)
Create equivalent subscriber tables for anything that became newly
part of the publication during the upgrade and....
======
src/bin/pg_dump/pg_dump.c
2. getSubscriptionTables
+/*
+ * getSubscriptionTables
+ * Get information about subscription membership for dumpable tables. This
+ * will be used only in binary-upgrade mode.
+ */
+void
+getSubscriptionTables(Archive *fout)
+{
+ DumpOptions *dopt = fout->dopt;
+ SubscriptionInfo *subinfo = NULL;
+ SubRelInfo *subrinfo;
+ PQExpBuffer query;
+ PGresult *res;
+ int i_srsubid;
+ int i_srrelid;
+ int i_srsubstate;
+ int i_srsublsn;
+ int ntups;
+ Oid last_srsubid = InvalidOid;
+
+ if (dopt->no_subscriptions || !dopt->binary_upgrade ||
+ fout->remoteVersion < 170000)
+ return;
This function comment says "used only in binary-upgrade mode." and the
Assert says the same. But, is this compatible with the other function
dumpSubscriptionTable() where it says "used only in binary-upgrade
mode and for PG17 or later versions"?
======
src/bin/pg_upgrade/check.c
3. check_new_cluster_subscription_configuration
+static void
+check_new_cluster_subscription_configuration(void)
+{
+ PGresult *res;
+ PGconn *conn;
+ int nsubs_on_old;
+ int max_replication_slots;
+
+ /* Logical slots can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
+ return;
IMO it is better to say < 1700 in this check, instead of <= 1600.
~~~
4.
+ /* Quick return if there are no subscriptions to be migrated */
+ if (nsubs_on_old == 0)
+ return;
Missing period in comment.
~~~
5.
+/*
+ * check_old_cluster_subscription_state()
+ *
+ * Verify that each of the subscriptions has all their corresponding tables in
+ * i (initialize), r (ready) or s (synchronized) state.
+ */
+static void
+check_old_cluster_subscription_state(ClusterInfo *cluster)
This function is only for the old cluster (hint: the function name) so
there is no need to pass the 'cluster' parameter here. Just directly
use old_cluster in the function body.
======
src/bin/pg_upgrade/t/004_subscription.pl
6.
+# Add tab_not_upgraded1 to the publication. Now publication has tab_upgraded1
+# and tab_upgraded2 tables.
+$publisher->safe_psql('postgres',
+ "ALTER PUBLICATION regress_pub ADD TABLE tab_upgraded2");
Typo in comment. You added tab_not_upgraded2, not tab_not_upgraded1
~~
7.
+# Subscription relations should be preserved. The upgraded won't know
+# about 'tab_not_upgraded1' because the subscription is not yet refreshed.
Typo or missing word in comment?
"The upgraded" ??
======
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Noah Misch | 2023-11-22 01:29:45 | dblink query interruptibility |
Previous Message | Michael Paquier | 2023-11-22 00:34:24 | Re: Typo with amtype = 's' in opr_sanity.sql |