Re: Pgoutput not capturing the generated columns

On Thu, May 23, 2024 at 10:56 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch! I checked your patches briefly. Here are my comments.
>
> 01. API
>
> Since the option for test_decoding is enabled by default, I think it should be renamed.
> E.g., "skip-generated-columns" or something.

Let's keep the same name 'include_generated_columns' for both the cases.

> 02. ddl.sql
>
> ```
> +-- check include-generated-columns option with generated column
> +CREATE TABLE gencoltable (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) STORED);
> +INSERT INTO gencoltable (a) VALUES (1), (2), (3);
> +SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts',
'1','include-generated-columns', '1'); 
> +                            data
> +-------------------------------------------------------------
> + BEGIN
> + table public.gencoltable: INSERT: a[integer]:1 b[integer]:2
> + table public.gencoltable: INSERT: a[integer]:2 b[integer]:4
> + table public.gencoltable: INSERT: a[integer]:3 b[integer]:6
> + COMMIT
> +(5 rows)
> ```
>
> We should test non-default case, which the generated columns are not generated.

Added the non-default case, which the generated columns are not generated.

> 03. ddl.sql
>
> Not sure new tests are in the correct place. Do we have to add new file and move tests to it?
> Thought?

Added the new tests in the 'decoding_into_rel.out' file.

> 04. protocol.sgml
>
> Please keep the format of the sgml file.

Fixed.

> 05. protocol.sgml
>
> The option is implemented as the streaming option of pgoutput plugin, so they should be
> located under "Logical Streaming Replication Parameters" section.

Fixed.

> 05. AlterSubscription
>
> ```
> +                               if (IsSet(opts.specified_opts, SUBOPT_GENERATED_COLUMN))
> +                               {
> +                                       ereport(ERROR,
> +                                                       (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                                                        errmsg("toggling generated_column option is not
allowed.")));
> +                               }
> ```
>
> If you don't want to support the option, you can remove SUBOPT_GENERATED_COLUMN
> macro from the function. But can you clarify the reason why you do not want?

Fixed.

> 07. logicalrep_write_tuple
>
> ```
> -               if (!column_in_column_list(att->attnum, columns))
> +               if (!column_in_column_list(att->attnum, columns) && !att->attgenerated)
> +                       continue;
> +
> +               if (att->attgenerated && !publish_generated_column)
>                         continue;
> ```
>
> I think changes in v2 was reverted or wrongly merged.

Fixed.

> 08. test code
>
> Can you add tests that generated columns are replicated by the logical replication?

Added the 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.