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
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml index 5666931..4ce89e9 100644 --- a/doc/src/sgml/ref/create_subscription.sgml +++ b/doc/src/sgml/ref/create_subscription.sgml @@ -433,16 +433,15 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl <term><literal>include_generated_columns</literal> (<type>boolean</type>)</term> <listitem> <para> - Specifies whether the generated columns present in the tables - associated with the subscription should be replicated. + Specifies whether the <literal>STORED</literal> generated columns present in + the tables associated with the subscription should be replicated. The default is <literal>false</literal>. </para> <para> If the subscriber-side column is also a generated column then this option has no effect; the subscriber column will be filled as normal with the - subscriber-side computed or default data. This option allows replication - of only STORED GENERATED COLUMNS. + subscriber-side computed or default data. </para> </listitem> </varlistentry>