public inbox for [email protected]
help / color / mirror / Atom feedFrom: Fujii Masao <[email protected]>
To: jian he <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: in BeginCopyTo make materialized view using COPY TO instead of COPY (query).
Date: Thu, 3 Apr 2025 00:19:58 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACJufxF3noNs9O6O3fL783h2-Sy1wfJb=0H+WMFmOLuhkfJw8w@mail.gmail.com>
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>
<[email protected]>
<CAKFQuwbD775kSHFfyF3shu+XMTX=3SEH+_JYLOWvrPo3y5dK-w@mail.gmail.com>
<[email protected]>
<CAKFQuwaMFiNGhk-F+TG4uVjiAfFwar6t+7fUc0mKDz55DGODcw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CACJufxF3noNs9O6O3fL783h2-Sy1wfJb=0H+WMFmOLuhkfJw8w@mail.gmail.com>
On 2025/04/01 12:12, jian he wrote:
> On Mon, Mar 31, 2025 at 11:27 PM Fujii Masao
> <[email protected]> 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
view thread (28+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: in BeginCopyTo make materialized view using COPY TO instead of COPY (query).
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox