On Thu, May 23, 2024 at 10:56 AM Hayato Kuroda (Fujitsu)
<[email protected]> 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.
