Hi, here are some review comments about patch v17-0003
======
1.
Missing a docs change?
Previously, (v16-0002) the patch included a change to
doc/src/sgml/protocol.sgml like below to say STORED generated instead
of just generated.
<para>
- Boolean option to enable generated columns. This option controls
- whether generated columns should be included in the string
- representation of tuples during logical decoding in PostgreSQL.
+ Boolean option to enable <literal>STORED</literal> generated columns.
+ This option controls whether <literal>STORED</literal>
generated columns
+ should be included in the string representation of tuples
during logical
+ decoding in PostgreSQL.
</para>
Why is that v16 change no longer present in patch v17-0003?
======
src/backend/catalog/pg_publication.c
2.
Previously, (v16-0003) this patch included a change to clarify the
kind of generated cols that are allowed in a column list.
* Translate a list of column names to an array of attribute numbers
* and a Bitmapset with them; verify that each attribute is appropriate
- * to have in a publication column list (no system or generated attributes,
- * no duplicates). Additional checks with replica identity are done later;
- * see pub_collist_contains_invalid_column.
+ * to have in a publication column list (no system or virtual generated
+ * attributes, no duplicates). Additional checks with replica identity
+ * are done later; see pub_collist_contains_invalid_column.
Why is that v16 change no longer present in patch v17-0003?
======
src/backend/replication/logical/tablesync.c
3. make_copy_attnamelist
- if (!attr->attgenerated)
+ if (attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
continue;
IIUC this logic is checking to make sure the subscriber-side table
column was not a generated column (because we don't replicate on top
of generated columns). So, does the distinction of STORED/VIRTUAL
really matter here?
~~~
fetch_remote_table_info:
nitpick - Should not change any spaces unrelated to the patch
======
send_relation_and_attrs:
- if (att->attgenerated && !include_generated_columns)
+ if (att->attgenerated && (att->attgenerated !=
ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
continue;
nitpick - It seems over-complicated. Conditions can be split so the
code fragment looks the same as in other places in this patch.
======
99.
Please see the attached diffs patch that implements any nitpicks
mentioned above.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index c2a7d18..5288769 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1008,7 +1008,7 @@ fetch_remote_table_info(char *nspname, char *relname,
bool **remotegenlist_res,
" LEFT JOIN pg_catalog.pg_index i"
" ON (i.indexrelid =
pg_get_replica_identity_index(%u))"
" WHERE a.attnum > 0::pg_catalog.int2"
- " AND NOT a.attisdropped",
lrel->remoteid);
+ " AND NOT a.attisdropped",
lrel->remoteid);
if(server_version >= 120000)
{
diff --git a/src/backend/replication/pgoutput/pgoutput.c
b/src/backend/replication/pgoutput/pgoutput.c
index 944554d..a256ab7 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -786,8 +786,14 @@ send_relation_and_attrs(Relation relation, TransactionId
xid,
if (att->attisdropped)
continue;
- if (att->attgenerated && (att->attgenerated !=
ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
- continue;
+ if (att->attgenerated)
+ {
+ if (!include_generated_columns)
+ continue;
+
+ if (att->attgenerated != ATTRIBUTE_GENERATED_STORED)
+ continue;
+ }
if (att->atttypid < FirstGenbkiObjectId)
continue;
