Re: pg_upgrade and logical replication

On Mon, 24 Apr 2023 at 12:52, Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> Hi,
>
> On Tue, Apr 18, 2023 at 01:40:51AM +0000, Hayato Kuroda (Fujitsu) wrote:
> >
> > I found a cfbot failure on macOS [1]. According to the log,
> > "SELECT count(*) FROM t2" was executed before synchronization was done.
> >
> > ```
> > [09:24:21.018](0.132s) not ok 18 - Table t2 should now have 3 rows on the new subscriber
> > ```
> >
> > With the patch present, wait_for_catchup() is executed after REFRESH, but
> > it may not be sufficient because it does not check pg_subscription_rel.
> > wait_for_subscription_sync() seems better for the purpose.
>
> Fixed, thanks!
>
> v5 attached with all previously mentioned fixes.

Few comments:
1) Should we document this command:
+               case ALTER_SUBSCRIPTION_ADD_TABLE:
+                       {
+                               if (!IsBinaryUpgrade)
+                                       ereport(ERROR,
+
(errcode(ERRCODE_SYNTAX_ERROR)),
+                                                       errmsg("ALTER
SUBSCRIPTION ... ADD TABLE is not supported"));
+
+                               supported_opts = SUBOPT_RELID |
SUBOPT_STATE | SUBOPT_LSN;
+                               parse_subscription_options(pstate,
stmt->options,
+
            supported_opts, &opts);
+
+                               /* relid and state should always be provided. */
+                               Assert(IsSet(opts.specified_opts,
SUBOPT_RELID));
+                               Assert(IsSet(opts.specified_opts,
SUBOPT_STATE));
+
+                               AddSubscriptionRelState(subid,
opts.relid, opts.state,
+
         opts.lsn);
+

Should we document something like:
This command is for use by in-place upgrade utilities. Its use for
other purposes is not recommended or supported. The behavior of the
option may change in future releases without notice.

2) Similarly in pg_dump too:
@@ -431,6 +431,7 @@ main(int argc, char **argv)
                {"table-and-children", required_argument, NULL, 12},
                {"exclude-table-and-children", required_argument, NULL, 13},
                {"exclude-table-data-and-children", required_argument,
NULL, 14},
+               {"preserve-subscription-state", no_argument,
&dopt.preserve_subscriptions, 1},


Should we document something like:
This command is for use by in-place upgrade utilities. Its use for
other purposes is not recommended or supported. The behavior of the
option may change in future releases without notice.

3) This same error is possible for ready state table but with invalid
remote_lsn, should we include this too in the error message:
+       if (is_error)
+               pg_fatal("--preserve-subscription-state is incompatible with "
+                                "subscription relations in non-ready state");
+
+       check_ok();
+}

Regards,
Vignesh