Thread: Re: Improve the error message for logical replication of regular column to generated column.
Re: Improve the error message for logical replication of regular column to generated column.
From
Peter Smith
Date:
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
Re: Improve the error message for logical replication of regular column to generated column.
From
Peter Smith
Date:
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
Re: Improve the error message for logical replication of regular column to generated column.
From
Amit Kapila
Date:
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.
Re: Improve the error message for logical replication of regular column to generated column.
From
Peter Smith
Date:
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
Re: Improve the error message for logical replication of regular column to generated column.
From
Amit Kapila
Date:
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.
Re: Improve the error message for logical replication of regular column to generated column.
From
vignesh C
Date:
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
Re: Improve the error message for logical replication of regular column to generated column.
From
Shlok Kyal
Date:
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
Re: Improve the error message for logical replication of regular column to generated column.
From
Shubham Khanna
Date:
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]
has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8RjJ4Qpqia9HccAZ0UWXmgYDebF3su2pw1jFYRYzSkk_QQQ%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
Re: Improve the error message for logical replication of regular column to generated column.
From
vignesh C
Date:
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
Re: Improve the error message for logical replication of regular column to generated column.
From
Amit Kapila
Date:
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.
Re: Improve the error message for logical replication of regular column to generated column.
From
Peter Smith
Date:
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
Re: Improve the error message for logical replication of regular column to generated column.
From
vignesh C
Date:
On Mon, 25 Nov 2024 at 16:06, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> 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.
Few comments:
1) Now that attribute string generation is moved to get_attrs_str and
there are only a couple of error statements in this function, how
about removing the function:
+/*
+ * If !bms_is_empty(missingatts), report the error message as 'Missing
+ * replicated columns.' Otherwise, report the error message as
'Cannot replicate
+ * to generated columns.'
+ */
+static void
+logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+
Bitmapset *missingatts,
+
Bitmapset *genatts)
+{
+ if (!bms_is_empty(missingatts))
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",
- missingattcnt,
- remoterel->nspname,
- remoterel->relname,
-
missingattsbuf.data)));
- }
+
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",
+
bms_num_members(missingatts),
+ remoterel->nspname,
+ remoterel->relname,
+
get_attrs_str(remoterel, missingatts)));
+
+ if (!bms_is_empty(genatts))
+ 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",
+
bms_num_members(genatts),
+ remoterel->nspname,
+ remoterel->relname,
+
get_attrs_str(remoterel, genatts)));
}
2) This comment seems to be wrong, "Cannot replicate to generated
columns" error will be thrown only if genatts bitmap is valid.
+/*
+ * If !bms_is_empty(missingatts), report the error message as 'Missing
+ * replicated columns.' Otherwise, report the error message as
'Cannot replicate
+ * to generated columns.'
+ */
+static void
+logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+
Bitmapset *missingatts,
+
Bitmapset *genatts)
Regards,
Vignesh
Re: Improve the error message for logical replication of regular column to generated column.
From
Peter Smith
Date:
On Tue, Nov 26, 2024 at 1:42 PM vignesh C <vignesh21@gmail.com> wrote:
>.
>
> Few comments:
> 1) Now that attribute string generation is moved to get_attrs_str and
> there are only a couple of error statements in this function, how
> about removing the function:
> +/*
> + * If !bms_is_empty(missingatts), report the error message as 'Missing
> + * replicated columns.' Otherwise, report the error message as
> 'Cannot replicate
> + * to generated columns.'
> + */
> +static void
> +logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
> +
> Bitmapset *missingatts,
> +
> Bitmapset *genatts)
> +{
> + if (!bms_is_empty(missingatts))
> 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",
> - missingattcnt,
> - remoterel->nspname,
> - remoterel->relname,
> -
> missingattsbuf.data)));
> - }
> +
> 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",
> +
> bms_num_members(missingatts),
> + remoterel->nspname,
> + remoterel->relname,
> +
> get_attrs_str(remoterel, missingatts)));
> +
> + if (!bms_is_empty(genatts))
> + 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",
> +
> bms_num_members(genatts),
> + remoterel->nspname,
> + remoterel->relname,
> +
> get_attrs_str(remoterel, genatts)));
> }
>
+1. This idea to just inline those errors instead of calling the
function sounds OK to me too.
Please consider also moving my suggested function comment if you
refactor this way.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Improve the error message for logical replication of regular column to generated column.
From
Amit Kapila
Date:
On Tue, Nov 26, 2024 at 9:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Nov 26, 2024 at 1:42 PM vignesh C <vignesh21@gmail.com> wrote:
> >.
> >
> > Few comments:
> > 1) Now that attribute string generation is moved to get_attrs_str and
> > there are only a couple of error statements in this function, how
> > about removing the function:
> > +/*
> > + * If !bms_is_empty(missingatts), report the error message as 'Missing
> > + * replicated columns.' Otherwise, report the error message as
> > 'Cannot replicate
> > + * to generated columns.'
> > + */
> > +static void
> > +logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
> > +
> > Bitmapset *missingatts,
> > +
> > Bitmapset *genatts)
> > +{
> > + if (!bms_is_empty(missingatts))
> > 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",
> > - missingattcnt,
> > - remoterel->nspname,
> > - remoterel->relname,
> > -
> > missingattsbuf.data)));
> > - }
> > +
> > 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",
> > +
> > bms_num_members(missingatts),
> > + remoterel->nspname,
> > + remoterel->relname,
> > +
> > get_attrs_str(remoterel, missingatts)));
> > +
> > + if (!bms_is_empty(genatts))
> > + 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",
> > +
> > bms_num_members(genatts),
> > + remoterel->nspname,
> > + remoterel->relname,
> > +
> > get_attrs_str(remoterel, genatts)));
> > }
> >
>
> +1. This idea to just inline those errors instead of calling the
> function sounds OK to me too.
>
Keeping them isolated in a function is better as it keeps the caller
function logicalrep_rel_open() easier to follow.
--
With Regards,
Amit Kapila.
Re: Improve the error message for logical replication of regular column to generated column.
From
Peter Smith
Date:
Hi, here are some review comments for patch v7-0001. ====== src/backend/replication/logical/relation.c logicalrep_report_missing_or_gen_attrs: 1. +/* + * If attempting to replicate missing or generated columns, report an error. + * Prioritize 'missing' errors if both occur though the prioritization is + * random. + */ That part "though the prioritization is random" seems wrongly worded because there is nothing random here. I guess the intention was something like "This prioritization design choice was arbitrary.", but TBH it may be better not to give any reason at all. ====== src/test/subscription/t/011_generated.pl 2. +# ============================================================================= +# The following test for expected error when attempting to replicate to a +# generated subscriber column. Test the following combination +# - regular -> generated +# - generated -> generated +# ============================================================================= + Some plurals seemed wrong to me. e.g. "combination" etc. SUGGESTION: The following test verifies the expected error when replicating to a generated subscriber column. Test the following combinations: ====== Kind Regards, Peter Smith. Fujitsu Australia
Re: Improve the error message for logical replication of regular column to generated column.
From
Amit Kapila
Date:
On Wed, Nov 27, 2024 at 3:30 AM Peter Smith <smithpb2250@gmail.com> wrote: > > Hi, here are some review comments for patch v7-0001. > > ====== > src/backend/replication/logical/relation.c > > logicalrep_report_missing_or_gen_attrs: > > 1. > +/* > + * If attempting to replicate missing or generated columns, report an error. > + * Prioritize 'missing' errors if both occur though the prioritization is > + * random. > + */ > > That part "though the prioritization is random" seems wrongly worded > because there is nothing random here. > > I guess the intention was something like "This prioritization design > choice was arbitrary.", but TBH it may be better not to give any > reason at all. > I think we should give a reason so that if we come across any scenario or add another one in the future, it will be easier to make the decision. I'll change the patch to use 'arbitrary' instead of random. -- With Regards, Amit Kapila.
Re: Improve the error message for logical replication of regular column to generated column.
From
vignesh C
Date:
On Wed, 27 Nov 2024 at 12:15, vignesh C <vignesh21@gmail.com> wrote: > > On Wed, 27 Nov 2024 at 08:50, Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Wed, Nov 27, 2024 at 3:30 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > Hi, here are some review comments for patch v7-0001. > > > > > > ====== > > > src/backend/replication/logical/relation.c > > > > > > logicalrep_report_missing_or_gen_attrs: > > > > > > 1. > > > +/* > > > + * If attempting to replicate missing or generated columns, report an error. > > > + * Prioritize 'missing' errors if both occur though the prioritization is > > > + * random. > > > + */ > > > > > > That part "though the prioritization is random" seems wrongly worded > > > because there is nothing random here. > > > > > > I guess the intention was something like "This prioritization design > > > choice was arbitrary.", but TBH it may be better not to give any > > > reason at all. > > > > > > > I think we should give a reason so that if we come across any scenario > > or add another one in the future, it will be easier to make the > > decision. I'll change the patch to use 'arbitrary' instead of random. > > There is a buildfarm failure in [1]. One of the new tests added to > verify the log for the "incompatible generated columns" issue was > incorrect. Specifically, the check qr/ERROR: ( [A-Z0-9]:) should have > been updated to qr/ERROR: ( [A-Z0-9]+:), which is consistent with > similar checks elsewhere in the codebase. The attached patch contains > the necessary changes to address this issue. > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2024-11-27%2004%3A17%3A03 The issue occurs specifically on the prion machine, which is configured with log_error_verbosity = verbose, causing error messages to include the sqlerrcode alongside the error description, as shown below from [1]: 2024-11-27 05:41:13.966 UTC [2990900:3] ERROR: 55000: logical replication target relation "public.t1" has incompatible generated columns: "c2", "c3" In contrast, other buildfarm machines do not include the sqlerrcode in the error messages, as seen here from [2]: 2024-11-27 07:19:45.975 CET [38683:2] ERROR: logical replication target relation "public.t1" has incompatible generated columns: "c2", "c3" The problem arises only when the sqlerrcode is present, as the error code matching was not correct. I have confirmed that the patch referenced in [3] resolves the issue when log_error_verbosity = verbose is enabled. [1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2024-11-27%2004%3A17%3A03 [2]: https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=loach&dt=2024-11-27%2006%3A07%3A55&stg=subscription-check [3]: https://www.postgresql.org/message-id/CALDaNm0C5LPiTxkdqsxiyeaL%3DnuUP8t6ne81sp9jE0%3DMFz%3D-ew%40mail.gmail.com Regards, Vignesh
Re: Improve the error message for logical replication of regular column to generated column.
From
Amit Kapila
Date:
On Wed, Nov 27, 2024 at 12:45 PM vignesh C <vignesh21@gmail.com> wrote: > > > > > There is a buildfarm failure in [1]. One of the new tests added to > > verify the log for the "incompatible generated columns" issue was > > incorrect. Specifically, the check qr/ERROR: ( [A-Z0-9]:) should have > > been updated to qr/ERROR: ( [A-Z0-9]+:), which is consistent with > > similar checks elsewhere in the codebase. The attached patch contains > > the necessary changes to address this issue. > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2024-11-27%2004%3A17%3A03 > > The issue occurs specifically on the prion machine, which is > configured with log_error_verbosity = verbose, causing error messages > to include the sqlerrcode alongside the error description, as shown > below from [1]: > 2024-11-27 05:41:13.966 UTC [2990900:3] ERROR: 55000: logical > replication target relation "public.t1" has incompatible generated > columns: "c2", "c3" > > In contrast, other buildfarm machines do not include the sqlerrcode in > the error messages, as seen here from [2]: > 2024-11-27 07:19:45.975 CET [38683:2] ERROR: logical replication > target relation "public.t1" has incompatible generated columns: "c2", > "c3" > > The problem arises only when the sqlerrcode is present, as the error > code matching was not correct. I have confirmed that the patch > referenced in [3] resolves the issue when log_error_verbosity = > verbose is enabled. > Thanks for the analysis. I have pushed your fix. -- With Regards, Amit Kapila.