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