Re: Improve the error message for logical replication of regular column to generated column.

Hi Shubham,

+1 for the patch idea.

Improving this error message for subscriber-side generated columns
will help to remove some confusion.

Here are my review comments for patch v1-0001.

======
Commit message.

1.
The error message was misleading, as it failed to clarify that the replication
of regular column on the publisher to the corresponding generated column on
the subscriber is not supported.

This patch improves the error handling and reporting mechanism to make it clear
that the replication of regular column on the subscriber is not supported,
resolving the misleading "missing column" error.

~

It makes no difference whether the publishing table column is regular
or generated, so you should not be implying that this has anything to
do with the replication of just regular columns. AFAIK, the *only*
thing that matters is that you cannot replicate into a subscriber-side
generated column or a subscriber-side missing column.

The current master reports replication into either a generated or a
missing column as the same "missing replication column" error. IIUC,
the errors were "correct", although clearly, for the generated column
case the error was quite misleading.

So, this patch is really *only* to improve the error wording when
attempting to replicate into a subscriber-side generated column.
That's what the commit message should be conveying.

======
src/backend/replication/logical/relation.c

logicalrep_rel_open:

2.
Bitmapset *missingatts;
+ StringInfoData gencolsattsbuf;
+ int generatedatts = 0;
+
+ initStringInfo(&gencolsattsbuf);

The existing "missing columns" error is implemented by building a BMS
and then passing it to the function 'logicalrep_report_missing_attrs'
to report the error.

IMO the generated column error is essentially the same, so should be
implemented with almost identical logic -- i.e. you should build a
'generatedattrs' BMS of generated cols with matching names and (if
that BMS is not empty) then pass that to a new function
'logicalrep_report_generated_attrs' (a sibling function to the
existing one).

~~~

3.
+ /*
+ * Check if the subscription table generated column has
+ * same name as a non-generated column in the
+ * corresponding publication table.
+ */

This (misplaced) comment talks about checking if the names are the
same. But I don't see any name-checking logic here (???). Where is it?

~~~

4.
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg_plural("replicating to a target relation's generated column
\"%s\" for \"%s.%s\" is not supported",
+ "replicating to a target relation's generated column \"%s\" for
\"%s.%s\" is not supported",
+ generatedatts, gencolsattsbuf.data, remoterel->nspname,
remoterel->relname)));

There are no plural differences here. This smells like a cut/paste
mistake from logicalrep_report_generated_attrs'.

IMO this error should close match the existing "missing replication
columns" error, and use the errmsg_plural correctly. In other words,
it should look something more like this:

ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg_plural("cannot replicate to target relation \"%s.%s\"
generated column: %s",
"cannot replicate to target relation \"%s.%s\"
generated columns: %s",
...

======
src/test/subscription/t/011_generated.pl

5.
+# =============================================================================
+# Exercise logical replication of a regular column to a subscriber side
+# generated column.
+#
+# A "normal --> generated" replication fails, reporting an error that the
+# replication of a generated column on subscriber side is not supported.
+# =============================================================================
+
+# --------------------------------------------------
+# Test Case: normal --> generated
+# Publisher table has regular columns 'c2' and 'c3'.
+# Subscriber table has generated columns 'c2' and 'c3'.
+# --------------------------------------------------
+

As I have said in previous internal reviews, this test (and the
comments) can be much more sophisticated. AFAICT by cleverly arranging
different publication table column types and different subscriber-side
table column ordering I think you should be able to test multiple
things at once.

Such as
- regular -> generated is detected
- generated -> generated is detected
- that the error only reports the generated column problems where the
column names are matching, not others

~~~~

6.
Also, as previously mentioned in internal reviews, this patch should
include a 2nd test case to do pretty much the same testing but
expecting to get a "missing replication column".

The reasons to include this 2nd test are:
a) The missing column was never tested properly before.
b) This current patch has overlapping logic so you need to be assured
that adding this new error doesn't break the existing one.
c) Only one of these errors wins. Adding both tests will define the
expected order if both error scenarios exist at the same time.

======
Kind Regards,
Peter Smith.
Fujitsu Australia