Re: in BeginCopyTo make materialized view using COPY TO instead of COPY (query). - Mailing list pgsql-hackers

On 2025/04/01 12:12, jian he wrote:
> On Mon, Mar 31, 2025 at 11:27 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>>
>> Regarding the patch, here are some review comments:
>>
>> +                                               errmsg("cannot copy from materialized view when the materialized
viewis 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
materializedview 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
rowsas "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