Re: pg_upgrade and logical replication

On Thu, 23 Nov 2023 at 05:56, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are some review comments for patch v17-0001
>
> ======
> src/bin/pg_dump/pg_dump.c
>
> 1. getSubscriptionTables
>
> +/*
> + * getSubscriptionTables
> + * Get information about subscription membership for dumpable tables. This
> + * will be used only in binary-upgrade mode and for PG17 or later versions.
> + */
> +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;
>
> I still felt that the function comment ("used only in binary-upgrade
> mode and for PG17 or later") was misleading. IMO that sounds like it
> would be OK for PG17 regardless of the binary mode, but the code says
> otherwise.
>
> Assuming the code is correct, perhaps the comment should say:
> "... used only in binary-upgrade mode for PG17 or later versions."

Modified

> ~~~
>
> 2. dumpSubscriptionTable
>
> +/*
> + * dumpSubscriptionTable
> + * Dump the definition of the given subscription table mapping. This will be
> + * used only in binary-upgrade mode and for PG17 or later versions.
> + */
> +static void
> +dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo)
>
> (this is the same as the previous review comment #1)
>
> Assuming the code is correct, perhaps the comment should say:
> "... used only in binary-upgrade mode for PG17 or later versions."

Modified

> ======
> src/bin/pg_upgrade/check.c
>
> 3.
> +static void
> +check_old_cluster_subscription_state()
> +{
> + FILE *script = NULL;
> + char output_path[MAXPGPATH];
> + int ntup;
> + ClusterInfo *cluster = &old_cluster;
> +
> + prep_status("Checking for subscription state");
> +
> + snprintf(output_path, sizeof(output_path), "%s/%s",
> + log_opts.basedir,
> + "subs_invalid.txt");
> + for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
> + {
> + PGresult *res;
> + DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
> + PGconn *conn = connectToServer(cluster, active_db->db_name);
>
> There seems no need for an extra variable ('cluster') here when you
> can just reference 'old_cluster' directly in the code, the same as
> other functions in this file do all the time.

Modified

The v18 version patch attached at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm3wyYY5ywFpCwUVW1_Di1af3WxeZggGEDQEu8qa58a7FQ%40mail.gmail.com