Re: Pgoutput not capturing the generated columns - Mailing list pgsql-hackers

On Thu, 23 May 2024 at 09:19, Shubham Khanna
<khannashubham1197@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