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)
<kuroda(dot)hayato(at)fujitsu(dot)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.