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

Hi Shubham,

here are my review comments for patch v4-0001.

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

logicalrep_report_missing_and_gen_attrs:

1.
static void
-logicalrep_report_missing_attrs(LogicalRepRelation *remoterel,
- Bitmapset *missingatts)
+logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+ Bitmapset *atts,
+ bool ismissing)

Maybe the function should be called
'logicalrep_report_missing_or_gen_attrs' (not 'and')

~

2.
- if (!bms_is_empty(missingatts))
+ if (!bms_is_empty(atts))

I felt this should be an Assert because the code becomes easier to
read if you check this before making the call in the first place. See
my NITPICKS patch.

~

3.
+ if (attcnt == 1)
+ appendStringInfo(&attsbuf, _("\"%s\""),
remoterel->attnames[i]);
else
- appendStringInfo(&missingattsbuf, _(", \"%s\""),
+ appendStringInfo(&attsbuf, _(", \"%s\""),
remoterel->attnames[i]);
}

This code can be simplified (e.g. remove the 'else' etc if you just
check > 1 instead). See my NITPICKS patch.

SUGGESTION
if (attcnt > 1)
appendStringInfo(&attsbuf, _(", "));

appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);

~~~

logicalrep_rel_open:

4.
+ /*
+ * Include it in generatedattrs if publishing to a generated
+ * column.
+ */
+ if (attr->attgenerated)
+ generatedattrs = bms_add_member(generatedattrs, attnum);

That comment can be simpler if indeed it is needed at all.

SUGGESTION:
/* Remember which subscriber columns are generated. */

~

5.
As I reported above (#2), I think it is better to check for empty BMS
in the caller because then the code is easier to read. Also, you need
to comment on which of these 2 errors will take precedence because if
there are simultaneous problems you are still only reporting one kind
of error at a time.

SUGGESTION:
/*
* Report any missing or generated columns. Note, if there are both
* kinds then the 'missing' error takes precedence.
*/
if (!bms_is_empty(missingatts))
logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
true);
if (!bms_is_empty(generatedattrs))
logicalrep_report_missing_and_gen_attrs(remoterel, generatedattrs,
false);

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

6.
+# =============================================================================
+# The following test cases exercise logical replication for the combinations
+# where there is a generated column on one or both sides of pub/sub:
+# - regular -> generated and generated -> generated
+# - regular -> missing
+# =============================================================================

6a.
This comment is not quite right. You can't say "where there is a
generated column on one or both sides of pub/sub" because that is not
true for the "regular -> missing" case. See NITPICKS for a suggested
comment.

~

6b.
IMO you should also be testing the "generated -> missing" combination.
You don't need more tests -- just more columns.

~

6c
You also need to include a test where there are BOTH generated and
missing to show the 'missing' error takes precedence. Again, you don't
need more separate test cases to achieve this -- just need more
columns in the tables.

~~~

7.
+# --------------------------------------------------
+# A "regular -> generated" and "generated -> generated" replication fails,
+# reporting an error that the generated column on the subscriber side
+# cannot be replicated.

/and/or/

~~~

8.
+# --------------------------------------------------
+# A "regular -> missing" replication fails, reporting an error
+# that the subscriber side is missing replicated columns.
+#
+# Testcase: regular -> missing
+# Publisher table has regular columns 'c2' and 'c3'.
+# Subscriber table is missing columns 'c2' and 'c3'.
+# --------------------------------------------------

I've also added the "generated -> missing" combination and addressed
the review comment about intercluding a test where there are BOTH
missing and generated columns, so you can see which error takes
precedence. Please see the NITPICKS diff.

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