Re: pg_upgrade and logical replication

Here are some review comments for patch v4-0001 (not the test code)

(There are some overlaps here with what Kuroda-san already posted
yesterday because we were looking at the same patch code. Also, a few
of my comments might become moot points if refactoring will be done
according to Kuroda-san's "general" questions).

======
Commit message

1.
To fix this problem, this patch teaches pg_dump in binary upgrade mode to emit
additional commands to be able to restore the content of pg_subscription_rel,
and addition LSN parameter in the subscription creation to restore the
underlying replication origin remote LSN. The LSN parameter is only accepted
in CREATE SUBSCRIPTION in binary upgrade mode.

~

SUGGESTION
To fix this problem, this patch teaches pg_dump in binary upgrade mode
to emit additional ALTER SUBSCRIPTION commands to facilitate restoring
the content of pg_subscription_rel, and provides an additional LSN
parameter for CREATE SUBSCRIPTION to restore the underlying
replication origin remote LSN. The new ALTER SUBSCRIPTION syntax and
new LSN parameter are not exposed to the user -- they are only
accepted in binary upgrade mode.

======
src/sgml/ref/pgupgrade.sgml

2.
+ <varlistentry>
+ <term><option>--preserve-subscription-state</option></term>
+ <listitem>
+ <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 reactived.
+ If that 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.
+ 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>
+ </listitem>
+ </varlistentry>

~

2a.
"If that option isn't used" --> "If this option isn't used"

~

2b.
The link renders strangely. It just says:

See the subscription part in the [section called "Notes"] for more information.

Maybe the link part can be rewritten so that it renders more nicely,
and also makes mention of pg_dump.

~

2c.
Maybe it is more readable to have the "isn't used" and "is used" parts
as separate paragraphs?

~

2d.
Typo /reactived/reactivated/ ??

======
src/backend/commands/subscriptioncmds.c

3.
+#define SUBOPT_RELID 0x00008000
+#define SUBOPT_STATE 0x00010000

Maybe 'SUBOPT_RELSTATE' is a better name for this per-relation state option?

~~~

4. SubOpts

+ Oid relid;
+ char state;
} SubOpts;

(similar to #3)

Maybe 'relstate' is a better name for this per-relation state?

~~~

5. parse_subscription_options

+ else if (IsSet(supported_opts, SUBOPT_STATE) &&
+ strcmp(defel->defname, "state") == 0)
+ {

(similar to #3)

Maybe called this option "relstate".

~

6.
+ if (strlen(state_str) != 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid relation state used")));

IIUC this is syntax not supposed to be reachable by user input. Maybe
there is some merit in making the errors similar looking to the normal
options, but OTOH it could also be misleading.

This might as well just be: Assert(strlen(state_str) == 1 &&
*state_str == SUBREL_STATE_READY);
or even simply: Assert(IsBinaryUpgrade);

~~~

7. CreateSubscription

+ if(IsBinaryUpgrade)
+ supported_opts |= SUBOPT_LSN;
parse_subscription_options(pstate, stmt->options, supported_opts, &opts);

7a.
Missing whitespace after the "if".

~

7b.
I wonder if this was deserving of a comment something like "The LSN
option is for internal use only"...

~~~

8. CreateSubscription

+ originid = replorigin_create(originname);
+
+ if (IsBinaryUpgrade && IsSet(opts.lsn, SUBOPT_LSN))
+ replorigin_advance(originid, opts.lsn, InvalidXLogRecPtr,
+ false /* backward */ ,
+ false /* WAL log */ );

I think the 'IsBinaryUpgrade' check is redundant here because
SUBOPT_LSN is not possible to be set unless that is true anyhow.

~~~

9. AlterSubscription

+ AddSubscriptionRelState(subid, opts.relid, opts.state,
+ opts.lsn);

This line wrapping of AddSubscriptionRelState seems unnecessary.

======
src/bin/pg_dump/pg_backup.h

10.
+
+ bool preserve_subscriptions;
} DumpOptions;

Maybe name this field "preserve_subscription_state" for consistency
with the option name.

======
src/bin/pg_dump/pg_dump.c

11. dumpSubscription

if (subinfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
+ {
+ 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, ");");
+ }
+

Maybe I misunderstood something -- Shouldn't this new ALTER
SUBSCRIPTION TABLE cmd only be happening when the option
dopt->preserve_subscriptions is true?

======
src/bin/pg_dump/pg_dump.h

12. SubRelInfo

+/*
+ * The SubRelInfo struct is used to represent subscription relation.
+ */
+typedef struct _SubRelInfo
+{
+ Oid srrelid;
+ char srsubstate;
+ char *srsublsn;
+} SubRelInfo;
+

12a.
"represent subscription relation" --> "represent a subscription relation"

~

12b.
Should include the indent file typdefs.list in the patch, and add this
new typedef to it.

======
src/bin/pg_upgrade/check.c

13. check_for_subscription_state

+/*
+ * check_for_subscription_state()
+ *
+ * Verify that all subscriptions have a valid remote_lsn and doesn't contain
+ * any table in a state different than ready.
+ */
+static void
+check_for_subscription_state(ClusterInfo *cluster)

SUGGESTION
Verify that all subscriptions have a valid remote_lsn and do not
contain any tables with srsubstate other than READY ('r').

~~~

14.
+ /* No subscription before pg10. */
+ if (GET_MAJOR_VERSION(cluster->major_version < 1000))
+ return;

14a.
The existing checking code seems slightly different to this because
the other check_XXX calls are guarded by the GET_MAJOR_VERSION before
being called.

~

14b.
Furthermore, I was confused about the combination when the < PG10 and
user_opts.preserve_subscriptions is true. Since this is just a return
(not an error) won't the subsequent pg_dump still attempt to use that
option (--preserve-subscriptions) even though we already know it
cannot work?

Would it be better to give an ERROR saying -preserve-subscriptions is
incompatible with the old PG version?

~~~

15.

+ pg_log(PG_WARNING,
+ "\nWARNING: %d subscription have invalid remote_lsn",
+ nb);

15a.
"have invalid" --> "has invalid"

~

15b.
I guess it would be more useful if the message can include the names
of the failing subscription and/or the relation that was in the wrong
state. Maybe that means moving all this checking logic into the
pg_dump code?

======
src/bin/pg_upgrade/option.c

16. parseCommandLine

user_opts.transfer_mode = TRANSFER_MODE_COPY;
+ user_opts.preserve_subscriptions = false;

This initial assignment is not needed because user_opts is static.

======
src/bin/pg_upgrade/pg_upgrade.h

17.
char *socketdir; /* directory to use for Unix sockets */
+ bool preserve_subscriptions; /* fully transfer subscription state */
} UserOpts;

Maybe name this field 'preserve_subscription_state' to match the option.

------
Kind Regards,
Peter Smith.
Fujitsu Australia