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>
