Re: pg_upgrade and logical replication - Mailing list pgsql-hackers

On Wed, 22 Nov 2023 at 06:48, Peter Smith <smithpb2250@gmail.com> wrote:
> ======
> 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....

Modified

> ======
> 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"?
>
Modified

> ======
> 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.
>
Modified

> ~~~
>
> 4.
> + /* Quick return if there are no subscriptions to be migrated */
> + if (nsubs_on_old == 0)
> + return;
>
> Missing period in comment.
>
Modified

> ~~~
>
> 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.
>
Modified

> ======
> 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
>
Modified

> ~~
>
> 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" ??
>
Modified

Attached the v17 patch which have the same changes

Thanks,
Shlok Kumar Kyal