On Fri, Nov 15, 2024 at 8:19 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> 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.
