On Thu, May 23, 2024 at 5:56 PM vignesh C <vignes...@gmail.com> wrote: > > On Thu, 23 May 2024 at 09:19, Shubham Khanna > <khannashubham1...@gmail.com> wrote: > > > > > 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. > > Few comments: > 1) Since this is removed, tupdesc variable is not required anymore: > +++ b/src/backend/catalog/pg_publication.c > @@ -534,12 +534,6 @@ publication_translate_columns(Relation targetrel, > List *columns, > errmsg("cannot use system > column \"%s\" in publication column list", > colname)); > > - if (TupleDescAttr(tupdesc, attnum - 1)->attgenerated) > - ereport(ERROR, > - > errcode(ERRCODE_INVALID_COLUMN_REFERENCE), > - errmsg("cannot use generated > column \"%s\" in publication column list", > - colname));
Fixed. > 2) In test_decoding include_generated_columns option is used: > + else if (strcmp(elem->defname, > "include_generated_columns") == 0) > + { > + if (elem->arg == NULL) > + continue; > + else if (!parse_bool(strVal(elem->arg), > &data->include_generated_columns)) > + ereport(ERROR, > + > (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("could not > parse value \"%s\" for parameter \"%s\"", > + > strVal(elem->arg), elem->defname))); > + } > > In subscription we have used generated_column, we can try to use the > same option in both places: > + else if (IsSet(supported_opts, SUBOPT_GENERATED_COLUMN) && > + strcmp(defel->defname, > "generated_column") == 0) > + { > + if (IsSet(opts->specified_opts, > SUBOPT_GENERATED_COLUMN)) > + errorConflictingDefElem(defel, pstate); > + > + opts->specified_opts |= SUBOPT_GENERATED_COLUMN; > + opts->generated_column = defGetBoolean(defel); > + } Will update the name to 'include_generated_columns' in the next version of the Patch. > 3) Tab completion can be added for create subscription to include > generated_column option Fixed. > 4) There are few whitespace issues while applying the patch, check for > git diff --check Fixed. > 5) Add few tests for the new option added Added new test cases. Patch v4-0001 contains all the changes required. See [1] for the changes added. [1] https://www.postgresql.org/message-id/CAHv8RjJcOsk%3Dy%2BvJ3y%2BvXhzR9ZUzUEURvS_90hQW3MNfJ5di7A%40mail.gmail.com Thanks and Regards, Shubham Khanna.