On Tue, Nov 26, 2024 at 5:45 AM Peter Smith <smithpb2...@gmail.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.
v6-0001-Error-message-improvement.patch
Description: Binary data
