On Fri, 21 Jun 2024 at 12:51, Peter Smith <smithpb2...@gmail.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