Re: Pgoutput not capturing the generated columns

On Tue, 29 Oct 2024 at 11:19, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch! Here are my comments for v44.
>
> 01. fetch_remote_table_info()
>
> `bool *remotegencolpresent` is accessed unconditionally, but it can cause crash
> if NULL is passed to the function. Should we add an Assert to verify it?

I have not made any changes for this as I felt it is not required.
Also Amit felt the same way as in [1].

> 02. fetch_remote_table_info()
>
> ```
> + if (server_version >= 180000)
> + *remotegencolpresent |= DatumGetBool(slot_getattr(slot, 5, &isnull));
> +
> ```
>
> Can we add Assert(!isnull) like other parts?

Included it.

> 03. fetch_remote_table_info()
>
> Also, we do not have to reach here once *remotegencolpresent becomes true.
> Based on 02 and 03, how about below?
>
> ```
> if (server_version >= 180000 && !(*remotegencolpresent))
> {
> *remotegencolpresent |= DatumGetBool(slot_getattr(slot, 5, &isnull));
> Assert(!isnull);
> }
> ```

Modified

> 04. pgoutput_column_list_init()
>
> + if (att->attgenerated)
> + {
> + if (bms_is_member(att->attnum, cols))
> + gencolpresent = true;
> +
> continue;
> + }
>
> I'm not sure it is correct. Why do you skip the generated column even when it is in
> the column list? Also, can you add comments what you want to do?

Modified it now and added comments.

The v45 version attached has the changes for the same.

[1] - https://www.postgresql.org/message-id/CAA4eK1Keq7hewXGe4mUHuCzEA5%3DZR5wQK0%2BL5TU%2BzVFUMrmOFw%40mail.gmail.com

Regards,
Vignesh