On Tue, 4 Jun 2024 at 10:21, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi,
>
> Here are some review comments for patch v5-0002.
>
> ======
> GENERAL
>
> G1.
> IIUC now you are unconditionally allowing all generated columns to be copied.
>
> I think this is assuming that the table sync code (in the next patch
> 0003?) is going to explicitly name all the columns it wants to copy
> (so if it wants to get generated cols then it will name the generated
> cols, and if is doesn't want generated cols then it won't name them).
>
> Maybe that is OK for the logical replication tablesync case, but I am
> not sure if it will be desirable to *always* copy generated columns in
> other user scenarios.
>
> e.g. I was wondering if there should be a new COPY command option
> introduced here -- INCLUDE_GENERATED_COLUMNS (with default false) so
> then the current HEAD behaviour is unaffected unless that option is
> enabled.
>
> ~~~
>
> G2.
> The current COPY command documentation [1] says "If no column list is
> specified, all columns of the table except generated columns will be
> copied."
>
> But this 0002 patch has changed that documented behaviour, and so the
> documentation needs to be changed as well, right?
>
> ======
> Commit Message
>
> 1.
> Currently COPY command do not copy generated column. With this commit
> added support for COPY for generated column.
>
> ~
>
> The grammar/cardinality is not good here. Try some tool (Grammarly or
> chatGPT, etc) to help correct it.
>
> ======
> src/backend/commands/copy.c
>
> ======
> src/test/regress/expected/generated.out
>
> ======
> src/test/regress/sql/generated.sql
>
> 2.
> I think these COPY test cases require some explicit comments to
> describe what they are doing, and what are the expected results.
>
> Currently, I have doubts about some of this test input/output
>
> e.g.1. Why is the 'b' column sometimes specified as 1? It needs some
> explanation. Are you expecting this generated col value to be
> ignored/overwritten or what?
>
> COPY gtest1 (a, b) FROM stdin DELIMITER ' ';
> 5 1
> 6 1
> \.
>
> e.g.2. what is the reason for this new 'missing data for column "b"'
> error? Or is it some introduced quirk because "b" now cannot be
> generated since there is no value for "a"? I don't know if the
> expected *.out here is OK or not, so some test comments may help to
> clarify it.
>
> ======
> [1] https://www.postgresql.org/docs/devel/sql-copy.html
>
Hi Peter,
I have removed the changes in the COPY command. I came up with an
approach which requires changes only in tablesync code. We can COPY
generated columns during tablesync using syntax 'COPY (SELECT
column_name from table) TO STDOUT.'
I have attached the patch for the same.
v7-0001 : Not Modified
v7-0002: Support replication of generated columns during initial sync.
Thanks and Regards,
Shlok Kyal
