From: | Shubham Khanna <khannashubham1197(at)gmail(dot)com> |
---|---|
To: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
Cc: | Rajendra Kumar Dangwal <dangwalrajendra888(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "euler(at)eulerto(dot)com" <euler(at)eulerto(dot)com> |
Subject: | Re: Pgoutput not capturing the generated columns |
Date: | 2024-06-03 09:55:38 |
Message-ID: | CAHv8Rj+nfCOv4=+ynJM=NA5ST8Ck6ReRLfEgR_J8Aps3eatLzA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, May 23, 2024 at 10:56 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)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.
Thanks and Regards,
Shubham Khanna.
From | Date | Subject | |
---|---|---|---|
Next Message | Shubham Khanna | 2024-06-03 10:01:46 | Re: Pgoutput not capturing the generated columns |
Previous Message | Shubham Khanna | 2024-06-03 09:30:38 | Re: Pgoutput not capturing the generated columns |