> 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.
I have added the feature in the document.
> 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.
I have made the generated column options as true for test_decoding
plugin so by default we will send generated column data.
> 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?
I verified the backward compatibility test by using the generated
column option and it worked fine. I think there is no need to make any
further changes.
> 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.
Fixed.
The attached v3 Patch has the changes for the same.
Thanks and Regards,
Shubham Khanna.
