> 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.
v3-0001-Support-generated-column-capturing-generated-colu.patch
Description: Binary data