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

Hi Shubham.

======
Commit message.

1.
FYI, to clarify my previous review comment [1]/messages/by-id/CAHut+Pt_vyFDGMbLXa94o4ffn4jNmFc8s6jkhmW-=BRTZM-HtQ@mail.gmail.com #1, I think a more
correct commit message might be:

SUGGESTION
Currently, if logical replication attempts to target a subscriber-side
table column that is either missing or generated, it produces the
following identical error message:
ERROR: logical replication target relation \"%s.%s\" is missing
replicated columns: %s

While the error itself is valid, the message wording can be misleading
for generated columns. This patch introduces a distinct error message
specifically for the generated column scenario.

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

2.
I noticed another problem when testing the new error message. There
are too many quotes for the column names. e.g.
2024-11-15 09:59:54.966 AEDT [32701] ERROR: replicating to a target
relation's generated column ""b"" for "public.t1" is not supported

This is because the patch code is quoting the individual faulty
columns and then you are re-quoting the whole list of faulty column
again in the err message. Please see the existing code in
'logicalrep_report_missing_attrs' for how this should look -- e.g. the
column list %s substitution marker in the message is NOT quoted.

"... is missing replicated column: %s"

======

BUT...

3. A different approach?

TBH, is introducing a whole new error message even a good idea?

Now there are going to be two separate error messages where previously
there was only one. So if the table has multiple problems at the same
time then still only one of them can "win". i.e. you have to either
report the "generated columns" problem 1st or the "missing columns"
problem 1st -- either way that might not be a good user experience
because they might be unaware of multiple problems until they try the
CREATE SUBSCRIPTION a 2nd time and then it fails a 2nd time with the
other kind of error! That could be annoying.

A better solution may be just to *combine* everything, so the user
only has to deal with one error. IIUC that's what is already happening
in master code, so this patch doesn't need to do anything except make
a quite trivial change to the wording of the existing error message.

For example:
BEFORE
errmsg_plural("logical replication target relation \"%s.%s\" is
missing replicated column: %s",
"logical replication target relation \"%s.%s\" is
missing replicated columns: %s",
SUGGESTION
errmsg_plural("logical replication target relation \"%s.%s\" has
missing or generated replicated column: %s",
"logical replication target relation \"%s.%s\" has
missing or generated replicated columns: %s",

Thoughts?

======
[1]: /messages/by-id/CAHut+Pt_vyFDGMbLXa94o4ffn4jNmFc8s6jkhmW-=BRTZM-HtQ@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Shubham,

Thanks for creating a patch! I checked yours and I have comments.

01.
```
+		StringInfoData gencolsattsbuf;
+		int			generatedatts = 0;
+
+		initStringInfo(&gencolsattsbuf);
```

gencolsattsbuf is initialized at the beginning but won't be free'd.

But I prefer the Peter's suggestion - you can combine the reporting stuff to
logicalrep_report_missing_attrs and rename the function. This is clearer than
directly adding declarations and ereport() in logicalrep_rel_open().

02.

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

I don't think this comment is correct. The error can be reported even when
both publisher and subscriber has the generated column, right?
Also, I feel comments can be located atop "if".

03.
I feel if you combine the reporting stuff with logicalrep_report_missing_attrs, some
of changes are not needed anymore. You can just add comment in logicalrep_rel_open
and modify the message in logicalrep_report_missing_attrs.

[1]: /messages/by-id/CAHut+PumbPEqk6v2XVjT7vKWKzQNBjMHXByWJ5=FmjEfk1v_pQ@mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Nov 15, 2024 at 6:10 AM Peter Smith <smithpb2250@gmail.com> wrote:

3. A different approach?

TBH, is introducing a whole new error message even a good idea?

Now there are going to be two separate error messages where previously
there was only one. So if the table has multiple problems at the same
time then still only one of them can "win". i.e. you have to either
report the "generated columns" problem 1st or the "missing columns"
problem 1st -- either way that might not be a good user experience
because they might be unaware of multiple problems until they try the
CREATE SUBSCRIPTION a 2nd time and then it fails a 2nd time with the
other kind of error! That could be annoying.

I don't know why the user needs to perform CREATE SUBSCRIPTION
multiple times to see this. IIUC, this error will happen in the apply
worker and after fixing the first, the user should see the second. I
think this can happen in other ways in apply worker as well.

A better solution may be just to *combine* everything, so the user
only has to deal with one error. IIUC that's what is already happening
in master code, so this patch doesn't need to do anything except make
a quite trivial change to the wording of the existing error message.

For example:
BEFORE
errmsg_plural("logical replication target relation \"%s.%s\" is
missing replicated column: %s",
"logical replication target relation \"%s.%s\" is
missing replicated columns: %s",
SUGGESTION
errmsg_plural("logical replication target relation \"%s.%s\" has
missing or generated replicated column: %s",
"logical replication target relation \"%s.%s\" has
missing or generated replicated columns: %s",

With this, we can combine two different ERRORs into one but it won't
be evident if the column name referred in the message is generated or
missing. I see your point but combining two different errors into one
is also confusing. We can try to add more checks to make this
distinction clear but it doesn't seem worth the effort and complexity.
Also, it is not clear whether combining different ERRORs is a good
idea in the first place.

--
With Regards,
Amit Kapila.

On Fri, Nov 15, 2024 at 2:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Nov 15, 2024 at 6:10 AM Peter Smith <smithpb2250@gmail.com> wrote:

3. A different approach?

TBH, is introducing a whole new error message even a good idea?

Now there are going to be two separate error messages where previously
there was only one. So if the table has multiple problems at the same
time then still only one of them can "win". i.e. you have to either
report the "generated columns" problem 1st or the "missing columns"
problem 1st -- either way that might not be a good user experience
because they might be unaware of multiple problems until they try the
CREATE SUBSCRIPTION a 2nd time and then it fails a 2nd time with the
other kind of error! That could be annoying.

I don't know why the user needs to perform CREATE SUBSCRIPTION
multiple times to see this. IIUC, this error will happen in the apply
worker and after fixing the first, the user should see the second. I
think this can happen in other ways in apply worker as well.

Yeah, I was thinking more of the scenario where the CREATE
SUBSCRIPTION gave the immediate error, so the user panics and does
DROP SUBSCRIPTION to give them all the time they need while they fix
the problem. Then they won't see the second problem until they
recreate the subscription.

But if they just are happy to leave the original CREATE SUBSCRIPTION
failing continuously while they fix the first problem then I think you
are correct --- the error should just fall through further to show the
next problem.

A better solution may be just to *combine* everything, so the user
only has to deal with one error. IIUC that's what is already happening
in master code, so this patch doesn't need to do anything except make
a quite trivial change to the wording of the existing error message.

For example:
BEFORE
errmsg_plural("logical replication target relation \"%s.%s\" is
missing replicated column: %s",
"logical replication target relation \"%s.%s\" is
missing replicated columns: %s",
SUGGESTION
errmsg_plural("logical replication target relation \"%s.%s\" has
missing or generated replicated column: %s",
"logical replication target relation \"%s.%s\" has
missing or generated replicated columns: %s",

With this, we can combine two different ERRORs into one but it won't
be evident if the column name referred in the message is generated or
missing. I see your point but combining two different errors into one
is also confusing. We can try to add more checks to make this
distinction clear but it doesn't seem worth the effort and complexity.
Also, it is not clear whether combining different ERRORs is a good
idea in the first place.

I don't know if it needs to be spelled out explicitly in the message
which is which because the user will surely know their own subscriber
table definition, so it will be quite obvious to them if a named
column is missing or generated.

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

On Fri, Nov 15, 2024 at 9:06 AM Peter Smith <smithpb2250@gmail.com> wrote:

On Fri, Nov 15, 2024 at 2:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

A better solution may be just to *combine* everything, so the user
only has to deal with one error. IIUC that's what is already happening
in master code, so this patch doesn't need to do anything except make
a quite trivial change to the wording of the existing error message.

For example:
BEFORE
errmsg_plural("logical replication target relation \"%s.%s\" is
missing replicated column: %s",
"logical replication target relation \"%s.%s\" is
missing replicated columns: %s",
SUGGESTION
errmsg_plural("logical replication target relation \"%s.%s\" has
missing or generated replicated column: %s",
"logical replication target relation \"%s.%s\" has
missing or generated replicated columns: %s",

With this, we can combine two different ERRORs into one but it won't
be evident if the column name referred in the message is generated or
missing. I see your point but combining two different errors into one
is also confusing. We can try to add more checks to make this
distinction clear but it doesn't seem worth the effort and complexity.
Also, it is not clear whether combining different ERRORs is a good
idea in the first place.

I don't know if it needs to be spelled out explicitly in the message
which is which because the user will surely know their own subscriber
table definition, so it will be quite obvious to them if a named
column is missing or generated.

The separate messages in this case would be clearer and better.

--
With Regards,
Amit Kapila.

On Thu, Nov 14, 2024 at 2:09 PM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I have fixed the given comments. The attached Patch contains the
required changes.

Thanks and regards,
Shubham Khanna.

On Fri, Nov 15, 2024 at 8:19 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Shubham,

Thanks for creating a patch! I checked yours and I have comments.

01.
```
+               StringInfoData gencolsattsbuf;
+               int                     generatedatts = 0;
+
+               initStringInfo(&gencolsattsbuf);
```

gencolsattsbuf is initialized at the beginning but won't be free'd.

But I prefer the Peter's suggestion - you can combine the reporting stuff to
logicalrep_report_missing_attrs and rename the function. This is clearer than
directly adding declarations and ereport() in logicalrep_rel_open().

02.

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

I don't think this comment is correct. The error can be reported even when
both publisher and subscriber has the generated column, right?
Also, I feel comments can be located atop "if".

03.
I feel if you combine the reporting stuff with logicalrep_report_missing_attrs, some
of changes are not needed anymore. You can just add comment in logicalrep_rel_open
and modify the message in logicalrep_report_missing_attrs.

[1]: /messages/by-id/CAHut+PumbPEqk6v2XVjT7vKWKzQNBjMHXByWJ5=FmjEfk1v_pQ@mail.gmail.com

I have fixed the given comments. The v2 version patch attached at [1]/messages/by-id/CAHv8RjJfuLO7HK1P=haY2stdGxYRAqrOwe6Ov4rzsprU63NQkg@mail.gmail.com
has the changes for the same.
[1]: /messages/by-id/CAHv8RjJfuLO7HK1P=haY2stdGxYRAqrOwe6Ov4rzsprU63NQkg@mail.gmail.com

Thanks and Regards,
Shubham Khanna.

On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

I have fixed the given comments. The attached Patch contains the
required changes.

Few comments:
1)
a)You can mention that "If ismissing is true, report the error message
as 'Missing replicated columns.' Otherwise, report the error message
as 'Cannot replicate to generated column."
 /*
- * Report error with names of the missing local relation column(s), if any.
+ * Report error with names of the missing and generated local
relation column(s), if any.
  */

b) You can keep the line within 80 chars in this case.

2) Spurious blank line:
+                       ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                        errmsg_plural("logical
replication target relation \"%s.%s\" is missing replicated column:
%s",
+
"logical replication target relation \"%s.%s\" is missing replicated
columns: %s",
+                                                                  attcnt,
+
remoterel->nspname,
+
remoterel->relname,
+
attsbuf.data)));
+
+               else
+                       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",
+                                                                  attcnt,
+
remoterel->nspname,
+
remoterel->relname,
+
attsbuf.data)));

3) This comment is not correct as the definition of
generated(publisher) to generated(subscriber) can be same:
+                               /*
+                                * Add to generatedattrs if names
match but definitions
+                                * differ.
+                                */
+                               if (attr->attgenerated)
+                                       generatedattrs =
bms_add_member(generatedattrs, i);

4)
a) You can use "regular" instead of "normal":
+# A "normal -> generated" and "generated -> generated" replication fails,
+# reporting an error that the generated column on the subscriber side
+# cannot be replicated.
+#
+# Test Case: normal -> generated and generated -> generated
+# Publisher table has regular column 'c2' and generated column 'c3'.
+# Subscriber table has generated columns 'c2' and 'c3'.

b) similarly here too:
+# --------------------------------------------------
+# A "normal -> missing" replication fails, reporting an error
+# that the subscriber side is missing replicated columns.
+#
+# Testcase: normal -> missing
+# Publisher table has normal columns 'c2' and 'c3'.
+# Subscriber table is missing columns 'c2' and 'c3'.
+# --------------------------------------------------

Regards,
Vignesh

On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Thu, Nov 14, 2024 at 2:09 PM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I have fixed the given comments. The attached Patch contains the
required changes.

Thanks for providing the patch.
I have few comments:

1. Getting segmentation fault for following test case:

Publisher:
CREATE TABLE t1 (a INT, b INT);
create publication pub1 for table t1(b)

Subscriber:
CREATE TABLE t1 (a INT, b int GENERATED ALWAYS AS (a + 1) STORED NOT NULL)
create subscription test1 connection 'dbname=postgres host=localhost
port=5432' publication pub1

Subscriber logs:
2024-11-16 17:23:16.919 IST [3842385] LOG: logical replication apply
worker for subscription "test1" has started
2024-11-16 17:23:16.931 IST [3842389] LOG: logical replication table
synchronization worker for subscription "test1", table "t1" has
started
2024-11-16 17:29:47.855 IST [3842359] LOG: background worker "logical
replication tablesync worker" (PID 3842389) was terminated by signal
11: Segmentation fault
2024-11-16 17:29:47.856 IST [3842359] LOG: terminating any other
active server processes

2.
+ initStringInfo(&attsbuf);

'attsbuf' not free'd. I think we should pfree it.

Thanks and Regards,
Shlok Kyal

On Fri, Nov 15, 2024 at 7:07 PM vignesh C <vignesh21@gmail.com> wrote:

On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

I have fixed the given comments. The attached Patch contains the
required changes.

Few comments:
1)
a)You can mention that "If ismissing is true, report the error message
as 'Missing replicated columns.' Otherwise, report the error message
as 'Cannot replicate to generated column."
/*
- * Report error with names of the missing local relation column(s), if any.
+ * Report error with names of the missing and generated local
relation column(s), if any.
*/

b) You can keep the line within 80 chars in this case.

2) Spurious blank line:
+                       ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                        errmsg_plural("logical
replication target relation \"%s.%s\" is missing replicated column:
%s",
+
"logical replication target relation \"%s.%s\" is missing replicated
columns: %s",
+                                                                  attcnt,
+
remoterel->nspname,
+
remoterel->relname,
+
attsbuf.data)));
+
+               else
+                       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",
+                                                                  attcnt,
+
remoterel->nspname,
+
remoterel->relname,
+
attsbuf.data)));

3) This comment is not correct as the definition of
generated(publisher) to generated(subscriber) can be same:
+                               /*
+                                * Add to generatedattrs if names
match but definitions
+                                * differ.
+                                */
+                               if (attr->attgenerated)
+                                       generatedattrs =
bms_add_member(generatedattrs, i);

4)
a) You can use "regular" instead of "normal":
+# A "normal -> generated" and "generated -> generated" replication fails,
+# reporting an error that the generated column on the subscriber side
+# cannot be replicated.
+#
+# Test Case: normal -> generated and generated -> generated
+# Publisher table has regular column 'c2' and generated column 'c3'.
+# Subscriber table has generated columns 'c2' and 'c3'.

b) similarly here too:
+# --------------------------------------------------
+# A "normal -> missing" replication fails, reporting an error
+# that the subscriber side is missing replicated columns.
+#
+# Testcase: normal -> missing
+# Publisher table has normal columns 'c2' and 'c3'.
+# Subscriber table is missing columns 'c2' and 'c3'.
+# --------------------------------------------------

I have fixed the given comments. The attached Patch contains the
required changes.

Thanks and regards,
Shubham Khanna.

On Sat, Nov 16, 2024 at 5:43 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Thu, Nov 14, 2024 at 2:09 PM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I have fixed the given comments. The attached Patch contains the
required changes.

Thanks for providing the patch.
I have few comments:

1. Getting segmentation fault for following test case:

Publisher:
CREATE TABLE t1 (a INT, b INT);
create publication pub1 for table t1(b)

Subscriber:
CREATE TABLE t1 (a INT, b int GENERATED ALWAYS AS (a + 1) STORED NOT NULL)
create subscription test1 connection 'dbname=postgres host=localhost
port=5432' publication pub1

Subscriber logs:
2024-11-16 17:23:16.919 IST [3842385] LOG: logical replication apply
worker for subscription "test1" has started
2024-11-16 17:23:16.931 IST [3842389] LOG: logical replication table
synchronization worker for subscription "test1", table "t1" has
started
2024-11-16 17:29:47.855 IST [3842359] LOG: background worker "logical
replication tablesync worker" (PID 3842389) was terminated by signal
11: Segmentation fault
2024-11-16 17:29:47.856 IST [3842359] LOG: terminating any other
active server processes

2.
+ initStringInfo(&attsbuf);

'attsbuf' not free'd. I think we should pfree it.

I have fixed the given comments. The v3 version patch attached at [1]/messages/by-id/CAHv8RjJ4Qpqia9HccAZ0UWXmgYDebF3su2pw1jFYRYzSkk_QQQ@mail.gmail.com
has the changes for the same.
[1]: /messages/by-id/CAHv8RjJ4Qpqia9HccAZ0UWXmgYDebF3su2pw1jFYRYzSkk_QQQ@mail.gmail.com

Thanks and Regards,
Shubham Khanna.

On Mon, 18 Nov 2024 at 15:47, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Fri, Nov 15, 2024 at 7:07 PM vignesh C <vignesh21@gmail.com> wrote:

I have fixed the given comments. The attached Patch contains the
required changes.

Couple of minor comments:
1) Since the previous error is going to exit, this pfree is not required:
+               else
+                       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",
+                                                                  attcnt,
+
remoterel->nspname,
+
remoterel->relname,
+
attsbuf.data)));
+
+               pfree(attsbuf.data);

2) "You can add single-line comments such as 'Report missing columns'
and 'Report replicating to generated columns.'"
+               logicalrep_report_missing_and_gen_attrs(remoterel,
generatedattrs,
+
                         false);
+               logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
+
                         true);

Regards,
Vignesh

On Mon, Nov 18, 2024 at 4:11 PM vignesh C <vignesh21@gmail.com> wrote:

On Mon, 18 Nov 2024 at 15:47, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Fri, Nov 15, 2024 at 7:07 PM vignesh C <vignesh21@gmail.com> wrote:

I have fixed the given comments. The attached Patch contains the
required changes.

Couple of minor comments:
1) Since the previous error is going to exit, this pfree is not required:
+               else
+                       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",
+                                                                  attcnt,
+
remoterel->nspname,
+
remoterel->relname,
+
attsbuf.data)));
+
+               pfree(attsbuf.data);

2) "You can add single-line comments such as 'Report missing columns'
and 'Report replicating to generated columns.'"
+               logicalrep_report_missing_and_gen_attrs(remoterel,
generatedattrs,
+
false);
+               logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
+
true);

I have fixed the given comments. The attached Patch contains the
required changes.

Thanks and regards,
Shubham Khanna.

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

On Mon, Nov 25, 2024 at 8:50 AM Peter Smith <smithpb2250@gmail.com> wrote:

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);

This and the proposed coding pattern by patch look odd to me. We
should have a single call to logicalrep_report_missing_and_gen_attrs()
and pass both missing and generated maps to the function. Then, let
the function display the appropriate ERROR message.

--
With Regards,
Amit Kapila.

On Mon, Nov 25, 2024 at 8:50 AM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I have fixed the given comments. The attached Patch contains the
required changes.

Thanks and regards,
Shubham Khanna.

On Mon, Nov 25, 2024 at 5:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 25, 2024 at 8:50 AM Peter Smith <smithpb2250@gmail.com> wrote:

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);

This and the proposed coding pattern by patch look odd to me. We
should have a single call to logicalrep_report_missing_and_gen_attrs()
and pass both missing and generated maps to the function. Then, let
the function display the appropriate ERROR message.

Yes, that would be better.

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

Hi Shubham,

Here are my review comments for patch v5-0001.

Please don't reply with a blanket "I have fixed the given comments"
because it was not true. E.g., some of my previous comments are
rejected in favour of Amit's better code suggestion, but then other
comments seem not addressed for reasons unknown.

======
Commit message.

1.
Now that the errors for the 'missing' and 'generated' columns are
separated, it means that if some subscriber table suffers both
problems at the same time then only one of those errors can be
reported. I think you should mention here that if that happens the
missing column error takes precedence.

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

get_attrs_str:

2.
+ * Generates a comma-separated string of attribute names based on the provided
+ * relation information and a bitmap indicating which attributes are included.
+ *
+ * The result is a palloc'd string.

"Generate"?

I think you can simplify the function comment a bit (also mentioning
the palloc'd string seemed overkill to me).

SUGGESTION:
Returns a comma-separated string of attribute names based on the
provided relation and bitmap indicating which attributes to include.

~

3.
+static char *
+get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)

All other static functions in this file have a common prefix
'logicalrep_', so it will be better for this to follow the same
pattern.

~~~~

logicalrep_report_missing_and_gen_attrs:

4.
+/*
+ * If !bms_is_empty(missingatts), report the error message as 'Missing
+ * replicated columns.' Otherwise, report the error message as
'Cannot replicate
+ * to generated columns.'
+ */

The function comment does not need to include code fragments or spell
out the actual errorS because the code is self-explanatory. Anyway,
the "Otherwise" here was not quite correct because the generated BMS
is also checked for emptiness. Finally, I think here it is better to
be explicit about the case when there are BOTH errors -- e.g. say that
the 'missing' error wins.

So the whole function comment can be simplified.

SUGGESTION:
/*
* If attempting to replicate to subscriber side missing columns or generated
* columns then report an error.
*
* (If there are both kinds of errors the 'missing' error takes precedence).
*/

~

5.
+static void
+logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+ Bitmapset *missingatts,
+ Bitmapset *genatts)

5a.
As I wrote in the previous review [1 - #1], because only one error can
happen at a time, IMO this function name should be
'logicalrep_report_missing_or_gen_attrs' (e.g. 'or' not 'and').

~

5b.
/genatts/generatedatts/ (that is what you called the BMS in the
caller, so better to be consistent)

~

logicalrep_rel_open:

6.
+ Bitmapset  *missingatts; /* Bitmapset for missing attributes. */
+ Bitmapset  *generatedattrs = NULL; /* Bitmapset for generated
+ * attributes. */

Those comments don't achieve anything because they are just saying the
same as the code. You might as well remove them.

~

7.
+ /*
+ * Report any missing and generated columns. Note, if there are both
+ * kinds then the 'missing' error takes precedence.
+ */
+ logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
+ generatedattrs);

This comment can also be removed. The function name is already
self-explanatory, and the information of the "Note" part belongs in
the function comment.

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

The tests LGTM.

======

Please refer to the attached diffs patch which includes most (but not
all) of the suggestions mentioned above.

======
[1]: /messages/by-id/CAHut+PuoDsPUO1YDBOEWAsKT8dXA0PDoK6S_Yc6kO_s8yPKHfA@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia.