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 Kind Regards, Peter Smith. Fujitsu Australia
