Hi, Here are some review comments for patch v9-0003
======
Commit Message
/fix/fixes/
======
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?
~~~
2.
General. Missing test.
Add some test cases to verify behaviour is different for STORED versus
VIRTUAL generated columns
======
src/sgml/ref/create_subscription.sgml
NITPICK - consider rearranging as shown in my nitpicks diff
NITPICK - use <literal> sgml markup for STORED
======
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 = ''");
}
}
======
99.
Please refer also to my attached nitpick diffs and apply those if you agree.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
