Re: Pgoutput not capturing the generated columns

On Thu, May 23, 2024 at 5:56 PM vignesh C <vignesh21@gmail.com> wrote:
>
> 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));

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.