On Wed, Sep 16, 2020 at 9:58 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Attaching v6 patch, rebased because of a recent commit
> 3d65b0593c5578014f62e09d4008006f1783f64d. Please consider this for
> further review.
>
Few comments:
==============
1.
+ /* Report error with names of the missing localrel column(s). */
+ if (!bms_is_empty(missingatts))
+ {
+ StringInfoData missingattsbuf;
+ int missingattcnt = 0;
+
+ initStringInfo(&missingattsbuf);
+ while ((i = bms_first_member(missingatts)) >= 0)
+ {
+ missingattcnt++;
+ if (missingattcnt == 1)
+ appendStringInfo(&missingattsbuf, _("\"%s\""),
+ remoterel->attnames[i]);
+ else
+ appendStringInfo(&missingattsbuf, _(", \"%s\""),
+ remoterel->attnames[i]);
+ }
+
+ bms_free(missingatts);
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical replication target relation \"%s.%s\" is missing "
- "some replicated columns",
- remoterel->nspname, remoterel->relname)));
+ errmsg_plural("logical replication target relation \"%s.%s\" is missing "
+ "replicated column: %s",
+ "logical replication target relation \"%s.%s\" is missing "
+ "replicated columns: %s",
+ missingattcnt,
+ remoterel->nspname,
+ remoterel->relname,
+ missingattsbuf.data)));
+ }
I think it is better to move the above code in a separate function
(say logicalrep_report_missing_attrs or something like that).
2. I think we always need to call bms_free(missingatts) because it is
possible that there is no missing attribute and in that case, we won't
free the memory allocated in bms_add_range.
3. The patch doesn't seem to be freeing the memory allocated for missingattsbuf.
4.
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical replication target relation \"%s.%s\" is missing "
- "some replicated columns",
- remoterel->nspname, remoterel->relname)));
+ errmsg_plural("logical replication target relation \"%s.%s\" is missing "
+ "replicated column: %s",
+ "logical replication target relation \"%s.%s\" is missing "
+ "replicated columns: %s",
+ missingattcnt,
+ remoterel->nspname,
+ remoterel->relname,
+ missingattsbuf.data)));
From the second line onwards, the message lines are not aligned in
errmsg_plural.
5. Also, in the above message, keep the error string in a single line.
For ex. see one of the existing messages:
errmsg_plural("WAL segment size must be a power of two between 1 MB
and 1 GB, but the control file specifies %d byte", .. I think it will
be easy to read that way. I know this is not exactly related to your
patch but improving it while changing this message seems fine.
6. I think we should add one test case for this in the existing
regression test (src/test/subscription/t/008_diff_schema).
--
With Regards,
Amit Kapila.
