On Tue, 4 Jun 2024 at 10:21, Peter Smith <smithpb2...@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
v7-0002-Support-replication-of-generated-column-during-in.patch
Description: Binary data
v7-0001-Enable-support-for-include_generated_columns-opti.patch
Description: Binary data
