Re: pg_upgrade and logical replication

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

In response to

Responses

Browse pgsql-hackers by date

  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