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 <smithpb2250(at)gmail(dot)com> 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.