On 2025/04/01 12:12, jian he wrote:
On Mon, Mar 31, 2025 at 11:27 PM Fujii Masao
<masao.fu...@oss.nttdata.com> wrote:
Regarding the patch, here are some review comments:
+ errmsg("cannot copy from
materialized view when the materialized view is not populated"),
How about including the object name for consistency with
other error messages in BeginCopyTo(), like this?
errmsg("cannot copy from unpopulated materialized view \"%s\"",
RelationGetRelationName(rel)),
+ errhint("Use the REFRESH
MATERIALIZED VIEW command populate the materialized view first."));
There seems to be a missing "to" just after "command".
Should it be "Use the REFRESH MATERIALIZED VIEW command to
populate the materialized view first."? Or we could simplify
the hint to match what SELECT on an unpopulated materialized
view logs: "Use the REFRESH MATERIALIZED VIEW command.".
based on your suggestion, i changed it to:
Thanks for updating the patch!
if (!RelationIsPopulated(rel))
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot copy from unpopulated
materialized view \"%s\"",
RelationGetRelationName(rel)),
errhint("Use the REFRESH MATERIALIZED VIEW
command to populate the materialized view first."));
I think it's better to use the same hint message as the one output by
"COPY (SELECT * FROM <unpopulated matview>) TO",
specifically: "Use the REFRESH MATERIALIZED VIEW command," for consistency.
The copy.sgml documentation should clarify that COPY TO can
be used with a materialized view only if it is populated.
"COPY TO can be used only with plain tables, not views, and does not
copy rows from child tables or child partitions"
i changed it to
"COPY TO can be used with plain tables and materialized views, not
regular views, and does not copy rows from child tables or child
partitions"
It would be clearer to specify that "COPY TO" applies to *populated*
materialized views rather than just "materialized views"?
Another alternative wording I came up with:
"COPY TO can only be used with plain tables and materialized views,
not regular views. It also does not copy rows from child tables or
child partitions."
If we split the first description into two as you suggested,
I'm tempted to propose the following improvements to enhance
the overall descriptions:
-------------
"COPY TO" can be used with plain tables and populated materialized views. For example, "COPY table
TO" copies the same rows as "SELECT * FROM ONLY table." However, it doesn't directly support other
relation types, such as partitioned tables, inheritance child tables, or views. To copy all rows from these relations,
use "COPY (SELECT * FROM table) TO."
-------------
Wouldn't it be beneficial to add a regression test to check
whether COPY matview TO works as expected?
sure.
The tests seem to have been placed under the category "COPY FROM ... DEFAULT",
which feels a bit misaligned. How about adding them to the end of copy.sql
instead?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
