Re: Improve the error message for logical replication of regular column to generated column. - Mailing list pgsql-hackers

Hi Shubham.

======
Commit message.

1.
FYI, to clarify my previous review comment [1] #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] https://www.postgresql.org/message-id/CAHut%2BPt_vyFDGMbLXa94o4ffn4jNmFc8s6jkhmW-%3DBRTZM-HtQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia