Re: Pgoutput not capturing the generated columns

On Fri, 21 Jun 2024 at 12:51, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi, Here are some review comments for patch v9-0003
>
> ======
> Commit Message
>
> /fix/fixes/
Fixed

> ======
> 1.
> General. Is tablesync enough?
>
> I don't understand why is the patch only concerned about tablesync?
> Does it make sense to prevent VIRTUAL column replication during
> tablesync, if you aren't also going to prevent VIRTUAL columns from
> normal logical replication (e.g. when copy_data = false)? Or is this
> already handled somewhere?
I checked the behaviour during incremental changes. I saw during
decoding 'null' values are present for Virtual Generated Columns. I
made the relevant changes to not support replication of Virtual
generated columns.

> ~~~
>
> 2.
> General. Missing test.
>
> Add some test cases to verify behaviour is different for STORED versus
> VIRTUAL generated columns
I have not added the tests as it would give an error in cfbot.
I have added a TODO note for the same. This can be done once the
VIRTUAL generated columns patch is committted.

> ======
> src/sgml/ref/create_subscription.sgml
>
> NITPICK - consider rearranging as shown in my nitpicks diff
> NITPICK - use <literal> sgml markup for STORED
Fixed

> ======
> src/backend/replication/logical/tablesync.c
>
> 3.
> - if ((walrcv_server_version(LogRepWorkerWalRcvConn) >= 120000 &&
> - walrcv_server_version(LogRepWorkerWalRcvConn) <= 160000) ||
> - !MySubscription->includegencols)
> + if (walrcv_server_version(LogRepWorkerWalRcvConn) < 170000)
> + {
> + if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 120000)
> appendStringInfo(&cmd, " AND a.attgenerated = ''");
> + }
> + else if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 170000)
> + {
> + if(MySubscription->includegencols)
> + appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
> + else
> + appendStringInfo(&cmd, " AND a.attgenerated = ''");
> + }
>
> IMO this logic is too tricky to remain uncommented -- please add some comments.
> Also, it seems somewhat complex. I think you can achieve the same more simply:
>
> SUGGESTION
>
> if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 120000)
> {
> bool gencols_allowed = walrcv_server_version(LogRepWorkerWalRcvConn) >= 170000
> && MySubscription->includegencols;
> if (gencols_allowed)
> {
> /* Replication of generated cols is supported, but not VIRTUAL cols. */
> appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
> }
> else
> {
> /* Replication of generated cols is not supported. */
> appendStringInfo(&cmd, " AND a.attgenerated = ''");
> }
> }
Fixed

> ======
>
> 99.
> Please refer also to my attached nitpick diffs and apply those if you agree.
Applied the changes.

I have attached the updated patch v10 here in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEUMCk6cCbw0vVZWo8FRd6ae9CmKG%3DgKP-9Q67jLn8HqtQ%40mail.gmail.com

Thanks and Regards,
Shlok Kyal