Re: in BeginCopyTo make materialized view using COPY TO instead of COPY (query).

On 2025/04/01 12:12, jian he wrote:
> On Mon, Mar 31, 2025 at 11:27 PM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)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