Message-ID: <4f9481ea-5dbc-43b2-a8a1-ec4cbecce6e7@oss.nttdata.com>
Date: Thu, 3 Apr 2025 00:19:58 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: in BeginCopyTo make materialized view using COPY TO instead of
 COPY (query).
To: jian he <jian.universality@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
 "David G. Johnston" <david.g.johnston@gmail.com>,
 Kirill Reshke <reshkekirill@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Tom Lane <tgl@sss.pgh.pa.us>, Michael Paquier <michael@paquier.xyz>
References: <CACJufxHVxnyRYy67hiPePNCPwVBMzhTQ6FaL9_Te5On9udG=yg@mail.gmail.com>
 <CACJufxGvy6QNMwZ5xd=L31F+4Rr6aXCASeMFyLZS+TVssK5+vA@mail.gmail.com>
 <CACJufxGZgh4oNV_8TqBwRoL+WFB2n3EqN97aMcuEmjuG+qm0qw@mail.gmail.com>
 <CACJufxHnyH4Zv+RtOABn8-1PjxkKch9R58dGhr+hCnV0LmNxSQ@mail.gmail.com>
 <CALdSSPgsgC_CGDjTyzu=1FAbuavTRsGFDnnm7ufGuWcd29QUHQ@mail.gmail.com>
 <CACJufxE35HxSMqG6su0txZwOLpbnBsDBav5eigxt6L8Nk8bJwQ@mail.gmail.com>
 <CALdSSPi3Gb=fi0ZCoJhAp3HQYKJmiCShcX4xMcUBSL0s5byBXQ@mail.gmail.com>
 <CAKFQuwbs61rxxCUVhECLSq4Xrz9s54dD2TzhX58d7+6WcC8dpA@mail.gmail.com>
 <e38cbf42-0f8e-4c3b-8c77-70c83116fdb2@dunslane.net>
 <CAKFQuwbD775kSHFfyF3shu+XMTX=3SEH+_JYLOWvrPo3y5dK-w@mail.gmail.com>
 <442e62f7-ef64-4df1-8453-d682fe9bdaa3@dunslane.net>
 <CAKFQuwaMFiNGhk-F+TG4uVjiAfFwar6t+7fUc0mKDz55DGODcw@mail.gmail.com>
 <425ebe4b-06c5-4212-af68-fd0e5537c250@dunslane.net>
 <f6abc172-f300-4e90-bcc6-c189d29d2f9f@oss.nttdata.com>
 <CACJufxF3noNs9O6O3fL783h2-Sy1wfJb=0H+WMFmOLuhkfJw8w@mail.gmail.com>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <CACJufxF3noNs9O6O3fL783h2-Sy1wfJb=0H+WMFmOLuhkfJw8w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/4f9481ea-5dbc-43b2-a8a1-ec4cbecce6e7%40oss.nttdata.com>
Precedence: bulk


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 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