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

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!

I had a high level look at the patch, few comments:
1) New ereport style can be used by removing the brackets around errcode:
1.a)
+                               ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                                errmsg("invalid
relation identifier used: %s", rel_str)));
+                       }

1.b)
+                       if (strlen(state_str) != 1)
+                               ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                                errmsg("invalid
relation state: %s", state_str)));

1.c)
+               case ALTER_SUBSCRIPTION_ADD_TABLE:
+                       {
+                               if (!IsBinaryUpgrade)
+                                       ereport(ERROR,
+
(errcode(ERRCODE_SYNTAX_ERROR)),
+                                                       errmsg("ALTER
SUBSCRIPTION ... ADD TABLE is not supported"));


2) Since this is a single statement, the braces are not required in this case:
2.a)
+       if (!fout->dopt->binary_upgrade ||
!fout->dopt->preserve_subscriptions ||
+               fout->remoteVersion < 100000)
+       {
+               return;
+       }

2.b) Similarly here too
+       if (dopt->binary_upgrade && dopt->preserve_subscriptions &&
+               subinfo->suboriginremotelsn)
+       {
+               appendPQExpBuffer(query, ", lsn = '%s'",
subinfo->suboriginremotelsn);
+       }

3) Since this comment is a very short comment, this can be changed
into a single line comment:
+       /*
+        * Get subscription relation fields.
+        */

4) Since cur_rel will be initialized in "if (cur_srsubid !=
last_srsubid)", it need not be initialized here:
+       int                     i,
+                               cur_rel = 0,
+                               ntups,

5) SubRelInfo should be placed above SubRemoveRels:
+++ b/src/tools/pgindent/typedefs.list
@@ -2647,6 +2647,7 @@ SubqueryScan
 SubqueryScanPath
 SubqueryScanState
 SubqueryScanStatus
+SubRelInfo
 SubscriptExecSetup

Regards,
Vignesh