Re: Improve the error message for logical replication of regular column to generated column.
On Wed, Nov 27, 2024 at 12:45 PM vignesh C 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.
Re: Improve the error message for logical replication of regular column to generated column.
On Wed, 27 Nov 2024 at 12:15, vignesh C wrote: > > On Wed, 27 Nov 2024 at 08:50, Amit Kapila wrote: > > > > On Wed, Nov 27, 2024 at 3:30 AM Peter Smith 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.
On Wed, 27 Nov 2024 at 08:50, Amit Kapila wrote: > > On Wed, Nov 27, 2024 at 3:30 AM Peter Smith 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 Regards, Vignesh diff --git a/src/test/subscription/t/011_generated.pl b/src/test/subscription/t/011_generated.pl index 66e6d8da5a..b1b87cf85e 100644 --- a/src/test/subscription/t/011_generated.pl +++ b/src/test/subscription/t/011_generated.pl @@ -361,7 +361,7 @@ $node_subscriber->safe_psql( # Verify that an error occurs. my $offset = -s $node_subscriber->logfile; $node_subscriber->wait_for_log( - qr/ERROR: ( [A-Z0-9]:)? logical replication target relation "public.t1" has incompatible generated columns: "c2", "c3"/, + qr/ERROR: ( [A-Z0-9]+:)? logical replication target relation "public.t1" has incompatible generated columns: "c2", "c3"/, $offset); # cleanup
Re: Improve the error message for logical replication of regular column to generated column.
On Wed, Nov 27, 2024 at 3:30 AM Peter Smith 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.
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.
On Tue, Nov 26, 2024 at 1:37 PM Shubham Khanna wrote: > > I have fixed the given comments. The attached Patch contains the > required changes. > The patch looks mostly good to me. I have made slight adjustments in the comments and error message. The following proposed error message appears to have a missing connector between relation and columns: "cannot replicate to target relation \"%s.%s\" generated column: %s"; so, I propose to change it to: "logical replication target relation \"%s.%s\" has incompatible generated column: %s". The proposed message is similar to the existing message for missing columns. Additionally, I kept only one test case as there is no need to keep tests for every possible combination. -- With Regards, Amit Kapila. v7-0001-Improve-error-message-for-replication-of-generate.patch Description: Binary data
Re: Improve the error message for logical replication of regular column to generated column.
On Tue, Nov 26, 2024 at 5:45 AM Peter Smith wrote: > > 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] > https://www.postgresql.org/message-id/CAHut%2BPuoDsPUO1YDBOEWAsKT8dXA0PDoK6S_Yc6kO_s8yPKHfA%40mail.gmail.com > I have fixed the given comments. The attached Patch contains the required changes. Thanks and regards, Shubham Khanna. v6-0001-Error-message-improvement.patch Description: Binary data
Re: Improve the error message for logical replication of regular column to generated column.
On Tue, Nov 26, 2024 at 9:47 AM Peter Smith wrote:
>
> On Tue, Nov 26, 2024 at 1:42 PM vignesh C 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.
On Tue, Nov 26, 2024 at 1:42 PM vignesh C 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.
On Mon, 25 Nov 2024 at 16:06, Shubham Khanna
wrote:
>
> On Mon, Nov 25, 2024 at 8:50 AM Peter Smith 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
Re: Improve the error message for logical replication of regular column to generated column.
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]
https://www.postgresql.org/message-id/CAHut%2BPuoDsPUO1YDBOEWAsKT8dXA0PDoK6S_Yc6kO_s8yPKHfA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index cdce752..7fb1604 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -220,13 +220,11 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel,
const char *attname)
}
/*
- * 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.
+ * Returns a comma-separated string of attribute names based on the provided
+ * relation and bitmap indicating which attributes to include.
*/
static char *
-get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)
+logicalrep_get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)
{
StringInfoData attsbuf;
int attcnt = 0;
@@ -249,12 +247,13 @@ get_attrs_str(LogicalRepRelation *remoterel, Bitmapset
*atts)
}
/*
- * If !bms_is_empty(missingatts), report the error message as 'Missing
- * replicated columns.' Otherwise, report the error message as 'Cannot
replicate
- * to generated columns.'
+ * If attempting to replicate to subscriber side
Re: Improve the error message for logical replication of regular column to generated column.
On Mon, Nov 25, 2024 at 5:27 PM Amit Kapila wrote: > > On Mon, Nov 25, 2024 at 8:50 AM Peter Smith 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.
On Mon, Nov 25, 2024 at 8:50 AM Peter Smith 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.
v5-0001-Error-message-improvement.patch
Description: Binary data
Re: Improve the error message for logical replication of regular column to generated column.
On Mon, Nov 25, 2024 at 8:50 AM Peter Smith 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.
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
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index 2c7be7d..21f4f12 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -224,49 +224,46 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel,
const char *attname)
* generated columns.'
*/
static void
-logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+logicalrep_report_missing_or_gen_attrs(LogicalRepRelation *remoterel,
Bitmapset *atts,
bool ismissing)
{
- if (!bms_is_empty(atts))
- {
- StringInfoData attsbuf;
- int
Re: Improve the error message for logical replication of regular column to generated column.
On Sat, Nov 16, 2024 at 5:43 PM Shlok Kyal wrote:
>
> On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
> wrote:
> >
> > On Thu, Nov 14, 2024 at 2:09 PM Peter Smith 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 m
Re: Improve the error message for logical replication of regular column to generated column.
On Mon, 18 Nov 2024 at 15:47, Shubham Khanna
wrote:
>
> On Fri, Nov 15, 2024 at 7:07 PM vignesh C 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.
On Mon, Nov 18, 2024 at 4:11 PM vignesh C wrote:
>
> On Mon, 18 Nov 2024 at 15:47, Shubham Khanna
> wrote:
> >
> > On Fri, Nov 15, 2024 at 7:07 PM vignesh C 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.
v4-0001-Error-message-improvement.patch
Description: Binary data
Re: Improve the error message for logical replication of regular column to generated column.
On Fri, Nov 15, 2024 at 7:07 PM vignesh C wrote:
>
> On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
> 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.
v3-0001-Error-message-improvement.patch
Description: Binary data
Re: Improve the error message for logical replication of regular column to generated column.
On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
wrote:
>
> On Thu, Nov 14, 2024 at 2:09 PM Peter Smith 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
> > -
Re: Improve the error message for logical replication of regular column to generated column.
On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
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.
On Thu, Nov 14, 2024 at 2:09 PM Peter Smith 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 t
Re: Improve the error message for logical replication of regular column to generated column.
On Fri, Nov 15, 2024 at 8:19 AM Hayato Kuroda (Fujitsu) 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]: > https://www.postgresql.org/message-id/CAHut%2BPumbPEqk6v2XVjT7vKWKzQNBjMHXByWJ5%3DFmjEfk1v_pQ%40mail.gmail.com > I have fixed the given comments. The v2 version patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CAHv8RjJfuLO7HK1P%3DhaY2stdGxYRAqrOwe6Ov4rzsprU63NQkg%40mail.gmail.com Thanks and Regards, Shubham Khanna.
Re: Improve the error message for logical replication of regular column to generated column.
On Fri, Nov 15, 2024 at 9:06 AM Peter Smith wrote:
>
> On Fri, Nov 15, 2024 at 2:07 PM Amit Kapila 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.
On Fri, Nov 15, 2024 at 2:07 PM Amit Kapila wrote:
>
> On Fri, Nov 15, 2024 at 6:10 AM Peter Smith 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.
On Fri, Nov 15, 2024 at 6:10 AM Peter Smith 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.
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
Re: Improve the error message for logical replication of regular column to generated column.
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.
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
