On Thu, 23 May 2024 at 09:19, Shubham Khanna
<[email protected]> 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
