Re: pg_upgrade and logical replication

On Mon, 24 Apr 2023 at 12:52, Julien Rouhaud <rjuju123(at)gmail(dot)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