Re: pg_upgrade and logical replication

On Wed, 10 May 2023 at 13:29, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are some review comments for the v5-0001 patch code.
>
> ======
> General
>
> 1. ALTER SUBSCRIPTION name ADD TABLE (relid = XYZ, state = 'x' [, lsn = 'X/Y'])
>
> I was a bit confused by this relation 'state' mentioned in multiple
> places. IIUC the pg_upgrade logic is going to reject anything with a
> non-READY (not 'r') state anyhow, so what is the point of having all
> the extra grammar/parse_subscription_options etc to handle setting the
> state when only possible value must be 'r'?
>

This command has been removed, this code has been removed

>
> 2. state V relstate
>
> I still feel code readbility suffers a bit by calling some fields/vars
> a generic 'state' instead of the more descriptive 'relstate'. Maybe
> it's just me.
>
> Previously commented same (see [1]#3, #4, #5)

Few of the code has been removed, I have modified wherever possible

> ======
> doc/src/sgml/ref/pgupgrade.sgml
>
> 3.
> + <para>
> + Fully preserve the logical subscription state if any. That includes
> + the underlying replication origin with their remote LSN and the list of
> + relations in each subscription so that replication can be simply
> + resumed if the subscriptions are reactivated.
> + </para>
>
> I think the "if any" part is not necessary. If you remove those words,
> then the rest of the sentence can be simplified.
>
> SUGGESTION
> Fully preserve the logical subscription state, which includes the
> underlying replication origin's remote LSN, and the list of relations
> in each subscription. This allows replication to simply resume when
> the subscriptions are reactivated.
>
This has been removed now.

>
> 4.
> + <para>
> + If this option isn't used, it is up to the user to reactivate the
> + subscriptions in a suitable way; see the subscription part in <xref
> + linkend="pg-dump-notes"/> for more information.
> + </para>
>
> The link still renders strangely as previously reported (see [1]#2b).
>
This has been removed now
>
> 5.
> + <para>
> + If this option is used and any of the subscription on the old cluster
> + has an unknown <varname>remote_lsn</varname> (0/0), or has any relation
> + in a state different from <literal>r</literal> (ready), the
> + <application>pg_upgrade</application> run will error.
> + </para>
>
> 5a.
> /subscription/subscriptions/

Modified

> 5b
> "has any relation in a state different from r" --> "has any relation
> with state other than r"

Modified slightly

> ======
> src/backend/commands/subscriptioncmds.c
>
> 6.
> + if (strlen(state_str) != 1)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("invalid relation state: %s", state_str)));
>
> Is this relation state validation overly simplistic, by only checking
> for length 1? Shouldn't this just be asserting the relstate must be
> 'r'?

This code has been removed

> ======
> src/bin/pg_dump/pg_dump.c
>
> 7. getSubscriptionTables
>
> +/*
> + * getSubscriptionTables
> + * get information about the given subscription's relations
> + */
> +void
> +getSubscriptionTables(Archive *fout)
> +{
> + SubscriptionInfo *subinfo;
> + SubRelInfo *rels = NULL;
> + PQExpBuffer query;
> + PGresult *res;
> + int i_srsubid;
> + int i_srrelid;
> + int i_srsubstate;
> + int i_srsublsn;
> + int i_nrels;
> + int i,
> + cur_rel = 0,
> + ntups,
> + last_srsubid = InvalidOid;
>
> Why some above are single int declarations and some are compound int
> declarations? Why not make them all consistent?

Modified

> ~
>
> 8.
> + appendPQExpBuffer(query, "SELECT srsubid, srrelid, srsubstate, srsublsn,"
> + " count(*) OVER (PARTITION BY srsubid) AS nrels"
> + " FROM pg_subscription_rel"
> + " ORDER BY srsubid");
>
> Should this SQL be schema-qualified like pg_catalog.pg_subscription_rel?

Modified

> ~
>
> 9.
> + for (i = 0; i < ntups; i++)
> + {
> + int cur_srsubid = atooid(PQgetvalue(res, i, i_srsubid));
>
> Should 'cur_srsubid' be declared Oid to match the atooid?

Modified

> ~~~
>
> 10. getSubscriptions
>
> + if (PQgetisnull(res, i, i_suboriginremotelsn))
> + subinfo[i].suboriginremotelsn = NULL;
> + else
> + subinfo[i].suboriginremotelsn =
> + pg_strdup(PQgetvalue(res, i, i_suboriginremotelsn));
> +
> + /*
> + * For now assume there's no relation associated with the
> + * subscription. Later code might update this field and allocate
> + * subrels as needed.
> + */
> + subinfo[i].nrels = 0;
>
> The wording "For now assume there's no" kind of gives an ambiguous
> interpretation for this comment. IMO it sounds like this is the
> "current" logic but some future PG version may behave differently - I
> don't think that is the intended meaning at all.
>
> SUGGESTION.
> Here we just initialize nrels to say there are 0 relations associated
> with the subscription. If necessary, subsequent logic will update this
> field and allocate the subrels.

This part of logic has been removed now as it is no more required

> ~~~
>
> 11. dumpSubscription
>
> + for (i = 0; i < subinfo->nrels; i++)
> + {
> + appendPQExpBuffer(query, "\nALTER SUBSCRIPTION %s ADD TABLE "
> + "(relid = %u, state = '%c'",
> + qsubname,
> + subinfo->subrels[i].srrelid,
> + subinfo->subrels[i].srsubstate);
> +
> + if (subinfo->subrels[i].srsublsn[0] != '\0')
> + appendPQExpBuffer(query, ", LSN = '%s'",
> + subinfo->subrels[i].srsublsn);
> +
> + appendPQExpBufferStr(query, ");");
> + }
>
> I previously asked ([1]#11) about how can this ALTER SUBSCRIPTION
> TABLE code happen unless 'preserve_subscriptions' is true, and you
> confirmed "It indirectly is, as in that case subinfo->nrels is
> guaranteed to be 0. I just tried to keep the code simpler and avoid
> too many nested conditions."

I have added the same check used that is used to get the subscription
tables to avoid confusion.

> ~
>
> If you are worried about too many nested conditions then a simple
> Assert(dopt->preserve_subscriptions); might be good to have here.
>
> ======
> src/bin/pg_upgrade/check.c
>
> 12. check_and_dump_old_cluster
>
> + /* PG 10 introduced subscriptions. */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1000 &&
> + user_opts.preserve_subscriptions)
> + {
> + check_for_subscription_state(&old_cluster);
> + }
>
> 12a.
> All the other checks in this function seem to be in decreasing order
> of PG version so maybe this check should be moved to follow that same
> pattern.

Modified

> ~
>
> 12b.
> Also won't it be better to give some error or notice of some kind if
> the option/version are incompatible? I think this was mentioned in a
> previous review.
>
> e.g.
>
> if (user_opts.preserve_subscriptions)
> {
> if (GET_MAJOR_VERSION(old_cluster.major_version) < 1000)
> <pg_log or pg_fatal goes here...>;
> check_for_subscription_state(&old_cluster);
> }

This has been removed now

> ~~~
>
> 13. check_for_subscription_state
>
> + for (int i = 0; i < ntup; i++)
> + {
> + is_error = true;
> + pg_log(PG_WARNING,
> + "\nWARNING: subscription \"%s\" has an invalid remote_lsn",
> + PQgetvalue(res, 0, 0));
> + }
>
> 13a.
> This WARNING does not mention the database, but a similar warning
> later about the non-ready state does mention the database. Probably
> they should be consistent.

Modified

> ~
>
> 13b.
> Something seems amiss. Here the is_error is assigned true; But later
> when you test is_error that is for logging the ready-state problem.
> Isn't there another missing pg_fatal for this invalid remote_lsn case?

Modified

> ======
> src/bin/pg_upgrade/option.c
>
> 14. usage
>
> + printf(_(" --preserve-subscription-state preserve the subscription
> state fully\n"));
>
> Why say "fully"? How is "preserve the subscription state fully"
> different to "preserve the subscription state" from the user's POV?

This has been removed now

These are handled as part of v7 posted at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm1ZrbHaWpJwwNhDTJocRKWd3rEkgJazuDdZ9Z-WdvonFg%40mail.gmail.com

Regards,
Vignesh