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)); 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); + } 3) Tab completion can be added for create subscription to include generated_column option 4) There are few whitespace issues while applying the patch, check for git diff --check 5) Add few tests for the new option added Regards, Vignesh