Re: pg_upgrade and logical replication

On Mon, 20 Nov 2023 at 10:44, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are some review comments for patch v15-0001
>
> ======
> src/bin/pg_dump/pg_dump.c
>
> 1. getSubscriptions
>
> + if (fout->remoteVersion >= 170000)
> + appendPQExpBufferStr(query, "o.remote_lsn AS suboriginremotelsn\n");
> + else
> + appendPQExpBufferStr(query, "NULL AS suboriginremotelsn\n");
> +
>
> There should be preceding spaces in those append strings to match the
> other ones.

Modified

> ~~~
>
> 2. dumpSubscriptionTable
>
> +/*
> + * dumpSubscriptionTable
> + * Dump the definition of the given subscription table mapping. This will be
> + * used only in binary-upgrade mode.
> + */
> +static void
> +dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo)
> +{
> + DumpOptions *dopt = fout->dopt;
> + SubscriptionInfo *subinfo = subrinfo->subinfo;
> + PQExpBuffer query;
> + char *tag;
> +
> + /* Do nothing in data-only dump */
> + if (dopt->dataOnly)
> + return;
> +
> + Assert(fout->dopt->binary_upgrade || fout->remoteVersion >= 170000);
>
> The function comment says this is only for binary-upgrade mode, so why
> does the Assert use || (OR)?

Added comments

> ======
> src/bin/pg_upgrade/check.c
>
> 3. check_and_dump_old_cluster
>
> + /*
> + * Subscription dependencies can be migrated since PG17. See comments atop
> + * get_old_cluster_subscription_count().
> + */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> + check_old_cluster_subscription_state(&old_cluster);
> +
>
> Should this be combined with the other adjacent check so there is only
> one "if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)"
> needed?

Modified

> ~~~
>
> 4. check_new_cluster
>
> check_new_cluster_logical_replication_slots();
> +
> + check_new_cluster_subscription_configuration();
>
> When checking the old cluster, the subscription was checked before the
> slots, but here for the new cluster, the slots are checked before the
> subscription. Maybe it makes no difference but it might be tidier to
> do these old/new checks in the same order.

Modified

> ~~~
>
> 5. check_new_cluster_logical_replication_slots
>
> - /* Quick return if there are no logical slots to be migrated. */
> + /* Quick return if there are no logical slots to be migrated */
>
> Change is not relevant for this patch.

Removed it

> ~~~
>
> 6.
>
> + res = executeQueryOrDie(conn, "SELECT setting FROM pg_settings "
> + "WHERE name IN ('max_replication_slots') "
> + "ORDER BY name DESC;");
>
> Using IN and ORDER BY in this SQL seems unnecessary when you are only
> searching for one name.

Modified

> ======
> src/bin/pg_upgrade/info.c
>
> 7. statics
>
> -
> +static void get_old_cluster_subscription_count(DbInfo *dbinfo);
>
> This change also removes an existing blank line -- not sure if that
> was intentional

Modified

> ~~~
>
> 8.
> @@ -365,7 +369,6 @@ get_template0_info(ClusterInfo *cluster)
> PQfinish(conn);
> }
>
> -
> /*
> * get_db_infos()
> *
>
> This blank line change (before get_db_infos) should not be part of this patch.

Modified

> ~~~
>
> 9. get_old_cluster_subscription_count
>
> It seems a slightly misleading function name because this is a PER-DB
> count, not a cluster count.

Modified

> ~~~
>
>
> 10.
> + /* Subscriptions can be migrated since PG17. */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
> + return;
>
> IMO it is better to compare < 1700 instead of <= 1600. It keeps the
> code more aligned with the comment.

Modified

> ~~~
>
> 11. count_old_cluster_subscriptions
>
> +/*
> + * count_old_cluster_subscriptions()
> + *
> + * Returns the number of subscription for all databases.
> + *
> + * Note: this function always returns 0 if the old_cluster is PG16 and prior
> + * because we gather subscriptions only for cluster versions greater than or
> + * equal to PG17. See get_old_cluster_subscription_count().
> + */
> +int
> +count_old_cluster_subscriptions(void)
> +{
> + int nsubs = 0;
> +
> + for (int dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
> + nsubs += old_cluster.dbarr.dbs[dbnum].nsubs;
> +
> + return nsubs;
> +}
>
> 11a.
> /subscription/subscriptions/

Modified

> ~
>
> 11b.
> The code is now consistent with the slots code which looks good. OTOH
> I thought that 'pg_catalog.pg_subscription' is shared across all
> databases of the cluster, so isn't this code inefficient to be
> querying again and again for every database (if there are many of
> them) instead of just querying 1 time only for the whole cluster?

My earlier version was like that, changed it to keep the code
consistent to logical replication slots.

> ======
> src/bin/pg_upgrade/t/004_subscription.pl
>
> 12.
> It is difficult to keep track of all the tables (upgraded and not
> upgraded) at each step of these tests. Maybe the comments can be more
> explicit along the way. e.g
>
> BEFORE
> +# Add tab_not_upgraded1 to the publication
>
> SUGGESTION
> +# Add tab_not_upgraded1 to the publication. Now publication has <blah blah>
>
> and
>
> BEFORE
> +# Subscription relations should be preserved
>
> SUGGESTION
> +# Subscription relations should be preserved. The upgraded won't know
> about 'tab_not_upgraded1' because <blah blah>
>
> etc.

Modified

> ~~~
>
> 13.
> +$result =
> + $new_sub->safe_psql('postgres', "SELECT count(*) FROM tab_not_upgraded1");
> +is($result, qq(0),
> + "no change in table tab_not_upgraded1 afer enable subscription which
> is not part of the publication"
>
> /afer/after/

Modified

> ~~~
>
> 14.
> +# ------------------------------------------------------
> +# Check that pg_upgrade refuses to run a) if there's a subscription with tables
> +# in a state different than 'r' (ready), 'i' (init) and 's' (synchronized)
> +# and/or b) if the subscription does not have a replication origin.
> +# ------------------------------------------------------
>
> 14a,
> /does not have a/has no/

Modified

> ~
>
> 14b.
> Maybe put a) and b) on newlines to be more readable.

Modified

The attached v16 version patch has the changes for the same.

Regards,
Vignesh