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-13 08:21:39 |
Message-ID: | CAHut+PuxAwTfQC7GfFATMV6YPimMPSEtZ0L_+-VhiYcc2hVVnQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Here are some review comments for patch v13-0001
======
src/bin/pg_dump/pg_dump.c
1. getSubscriptionTables
+ int i_srsublsn;
+ int i;
+ int cur_rel = 0;
+ int ntups;
What is the difference between 'i' and 'cur_rel'?
AFAICT these represent the same tuple index, in which case you might
as well throw away 'cur_rel' and only keep 'i'.
~~~
2. getSubscriptionTables
+ for (i = 0; i < ntups; i++)
+ {
+ Oid cur_srsubid = atooid(PQgetvalue(res, i, i_srsubid));
+ Oid relid = atooid(PQgetvalue(res, i, i_srrelid));
+ TableInfo *tblinfo;
Since this is all new code, using C99 style for loop variable
declaration of 'i' will be better.
======
src/bin/pg_upgrade/check.c
3. check_for_subscription_state
+check_for_subscription_state(ClusterInfo *cluster)
+{
+ int dbnum;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+ int ntup;
+
+ /* Subscription relations state can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) < 1700)
+ return;
+
+ prep_status("Checking for subscription state");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "subscription_state.txt");
I felt this filename ought to be more like
'subscriptions_with_bad_state.txt' because the current name looks like
a normal logfile with nothing to indicate that it is only for the
states of the "bad" subscriptions.
~~~
4.
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
Since this is all new code, using C99 style for loop variable
declaration of 'dbnum' will be better.
~~~
5.
+ * a) SUBREL_STATE_DATASYNC:A relation upgraded while in this state
+ * would retain a replication slot, which could not be dropped by the
+ * sync worker spawned after the upgrade because the subscription ID
+ * tracked by the publisher does not match anymore.
missing whitespace
/SUBREL_STATE_DATASYNC:A relation/SUBREL_STATE_DATASYNC: A relation/
======
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2023-11-13 08:22:06 | Re: pg_upgrade and logical replication |
Previous Message | Anthonin Bonnefoy | 2023-11-13 08:19:52 | Re: Add PQsendSyncMessage() to libpq |