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

From: Shubham Khanna <[email-masked]>
Date: Thu, 14 Nov 2024 11:48:39 +0530

Hi all,

Recently there was an issue reported by Kuroda-san on a different
thread [1]. I have created this thread to discuss the issue
separately.

Currently, the ERROR message for the replication of a regular column
on the publisher node to a generated column on the subscriber node
is:-
ERROR: logical replication target relation \"%s.%s\" is missing
replicated columns: %s
For example:-
test_pub=# CREATE TABLE t1(c1 int, c2 int GENERATED ALWAYS AS (c1 * 2) STORED);
test_pub=# CREATE PUBLICATION pub1 for table t1(c1, c2, c3);
test_pub=# INSERT INTO t1 VALUES (1);

test_sub=# CREATE TABLE t1(c1 int, c2 int GENERATED ALWAYS AS (c1 + 2)
STORED, c3 int GENERATED ALWAYS AS (c1 + 2) STORED);
test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr'
PUBLICATION pub1;
-> ERROR: logical replication target relation "t1" is missing
replicated column: "c2","c3"

The error message was misleading, as it failed to clarify that the
replication of a regular column on the publisher to the corresponding
generated column on the subscriber is not supported.
To avoid and solve the issue, we can update the ERROR message stating
that the replication of the generated column on the subscriber is not
supported. I have attached a patch for the same.

[1]: https://www.postgresql.org/message-id/TYCPR01MB5693AF061D62E55189490D2DF5562%40TYCPR01MB5693.jpnprd01.prod.outlook.com

Thanks and Regards,
Shubham Khanna.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 15 Nov 2024 02:49:16 +0000

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]: https://www.postgresql.org/message-id/CAHut%2BPumbPEqk6v2XVjT7vKWKzQNBjMHXByWJ5%3DFmjEfk1v_pQ%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED