Dear Shubham,
Thanks for creating a patch! Here are high-level comments.
1.
Please document the feature. If it is hard to describe, we should change the API.
2.
Currently, the option is implemented as streaming option. Are there any reasons
to choose the way? Another approach is to implement as slot option, like failover
and temporary.
3.
You said that subscription option is not supported for now. Not sure, is it mean
that logical replication feature cannot be used for generated columns? If so,
the restriction won't be acceptable. If the combination between this and initial
sync is problematic, can't we exclude them in CreateSubscrition and AlterSubscription?
E.g., create_slot option cannot be set if slot_name is NONE.
4.
Regarding the test_decoding plugin, it has already been able to decode the
generated columns. So... as the first place, is the proposed option really needed
for the plugin? Why do you include it?
If you anyway want to add the option, the default value should be on - which keeps
current behavior.
5.
Assuming that the feature become usable used for logical replicaiton. Not sure,
should we change the protocol version at that time? Nodes prior than PG17 may
not want receive values for generated columns. Can we control only by the option?
6. logicalrep_write_tuple()
```
- if (!column_in_column_list(att->attnum, columns))
+ if (!column_in_column_list(att->attnum, columns) && !att->attgenerated)
+ continue;
```
Hmm, does above mean that generated columns are decoded even if they are not in
the column list? If so, why? I think such columns should not be sent.
7.
Some functions refer data->publish_generated_column many times. Can we store
the value to a variable?
Below comments are for test_decoding part, but they may be not needed.
=====
a. pg_decode_startup()
```
+ else if (strcmp(elem->defname, "include_generated_columns") == 0)
```
Other options for test_decoding do not have underscore. It should be
"include-generated-columns".
b. pg_decode_change()
data->include_generated_columns is referred four times in the function.
Can you store the value to a varibable?
c. pg_decode_change()
```
- true);
+ true, data->include_generated_columns );
```
Please remove the blank.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
