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

Dear Julien,

Thank you for updating the patch. I checked yours.
Followings are general or non-minor questions:

1.
Feature freeze for PG16 has already come. So I think there is no reason to rush
making the patch. Based on above, could you allow to upgrade while synchronizing
data? Personally it can be added as 0002 patch which extends the feature. Or
have you already found any problem?

2.
I have a questions about the SQL interface:

ALTER SUBSCRIPTION name ADD TABLE (relid = XYZ, state = 'x' [, lsn = 'X/Y'])

Here the oid of the table is directly specified, but is it really kept between
old and new node? Similar command ALTER PUBLICATION requires the name of table,
not the oid.

3.
Currently getSubscriptionRels() is called from the getSubscriptions(), but I could
not find the reason why we must do like that. Other functions like
getPublicationTables() is directly called from getSchemaData(), so they should
be followed. Additionaly, I found two problems.

* Only tables that to be dumped should be included. See getPublicationTables().
* dropStmt for subscription relations seems not to be needed.
* Maybe security label and comments should be also dumped.

Followings are minor comments.


4. parse_subscription_options

```
+                       opts->state = defGetString(defel)[0];
```

[0] is not needed.

5. AlterSubscription

```
+                               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));
+
```

SUBOPT_LSN accepts "none" string, which means InvalidLSN. Isn't it better to
reject it?

6. dumpSubscription()

```
+       if (dopt->binary_upgrade && dopt->preserve_subscriptions &&
+               subinfo->suboriginremotelsn)
+       {
+               appendPQExpBuffer(query, ", lsn = '%s'", subinfo->suboriginremotelsn);
+       }
```

{} is not needed.

7. pg_dump.h

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

This typedef must be added to typedefs.list.

8. check_for_subscription_state

```
            nb = atooid(PQgetvalue(res, 0, 0));
            if (nb != 0)
            {
                is_error = true;
                pg_log(PG_WARNING,
                       "\nWARNING:  %d subscription have invalid remote_lsn",
                       nb);
            }
```

I think no need to use atooid. Additionaly, isn't it better to show the name of
subscriptions which have invalid remote_lsn?

```
        nb = atooid(PQgetvalue(res, 0, 0));
        if (nb != 0)
        {
            is_error = true;
            pg_log(PG_WARNING,
                   "\nWARNING: database \"%s\" has %d subscription "
                   "relations(s) in non-ready state", active_db->db_name, nb);
        }
```

Same as above.

9. parseCommandLine

```
+       user_opts.preserve_subscriptions = false;
```

I think this initialization is not needed because it is default.

And maybe you missed to run pgindent.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED