Re: speedup COPY TO for partitioned table.

On 2025-07-15 12:31, jian he wrote:
> On Mon, Jul 14, 2025 at 10:02 PM Álvaro Herrera <alvherre(at)kurilemu(dot)de>
> wrote:
>>
>> On 2025-Jul-02, jian he wrote:
>>
>> > @@ -673,11 +680,34 @@ BeginCopyTo(ParseState *pstate,
>> > errmsg("cannot copy from sequence \"%s\"",
>> > RelationGetRelationName(rel))));
>> > else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>> > - ereport(ERROR,
>> > - (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>> > - errmsg("cannot copy from partitioned table \"%s\"",
>> > - RelationGetRelationName(rel)),
>> > - errhint("Try the COPY (SELECT ...) TO variant.")));
>> > + {
>> > + children = find_all_inheritors(RelationGetRelid(rel),
>> > + AccessShareLock,
>> > + NULL);
>> > +
>> > + foreach_oid(childreloid, children)
>> > + {
>> > + char relkind = get_rel_relkind(childreloid);
>> > +
>> > + if (relkind == RELKIND_FOREIGN_TABLE)
>> > + {
>> > + char *relation_name;
>> > +
>> > + relation_name = get_rel_name(childreloid);
>> > + ereport(ERROR,
>> > + errcode(ERRCODE_WRONG_OBJECT_TYPE),
>> > + errmsg("cannot copy from foreign table \"%s\"", relation_name),
>> > + errdetail("Partition \"%s\" is a foreign table in the partitioned table \"%s\"",
>> > + relation_name, RelationGetRelationName(rel)),
>> > + errhint("Try the COPY (SELECT ...) TO variant."));
>> > + }
>>
>> This code looks like it's duplicating what you could obtain by using
>> RelationGetPartitionDesc and then observe the ->isleaf bits. Maybe
>> have
>> a look at the function RelationHasForeignPartition() in the patch at
>> https://postgr.es/m/CANhcyEW_s2LD6RiDSMHtWQnpYB67EWXqf7N8mn7dOrnaKMfROg@mail.gmail.com
>> which looks very similar to what you need here. I think that would
>> also
>> have the (maybe dubious) advantage that the rows will be output in
>> partition bound order rather than breadth-first (partition hierarchy)
>> OID order.
>>
> hi.
>
> else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> {
> PartitionDesc pd = RelationGetPartitionDesc(rel, true);
> for (int i = 0; i < pd->nparts; i++)
> {
> Relation partRel;
> if (!pd->is_leaf[i])
> continue;
> partRel = table_open(pd->oids[i], AccessShareLock);
> if (partRel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
> ereport(ERROR,
> errcode(ERRCODE_WRONG_OBJECT_TYPE),
> errmsg("cannot copy from foreign table
> \"%s\"", RelationGetRelationName(partRel)),
> errdetail("Partition \"%s\" is a foreign
> table in the partitioned table \"%s\"",
>
> RelationGetRelationName(partRel), RelationGetRelationName(rel)),
> errhint("Try the COPY (SELECT ...) TO
> variant."));
> table_close(partRel, NoLock);
> scan_oids = lappend_oid(scan_oids,
> RelationGetRelid(partRel));
> }
> }
>
> I tried the above code, but it doesn't work because
> RelationGetPartitionDesc
> only retrieves the immediate partition descriptor of a partitioned
> relation, it
> doesn't recurse to the lowest level.
>
> Actually Melih Mutlu raised this question at
> https://postgr.es/m/CAGPVpCQou3hWQYUqXNTLKdcuO6envsWJYSJqbZZQnRCjZA6nkQ%40mail.gmail.com
> I kind of ignored it...
> I guess we have to stick with find_all_inheritors here?

That might be the case.
I thought we could consider using RelationHasForeignPartition() instead,
if [1] gets committed.
However, since that function only tells us whether any foreign
partitions exist, whereas the current patch outputs the specific
problematic partitions or foreign tables in the log, I think the current
approach is more user-friendly.

> <command>COPY TO</command> can be used with plain
> - tables and populated materialized views.
> - For example,
> + tables, populated materialized views and partitioned tables.
> + For example, if <replaceable
class="parameter">table</replaceable> is not partitioned table,
> <literal>COPY <replaceable class="parameter">table</replaceable>
> TO</literal> copies the same rows as
> <literal>SELECT * FROM ONLY <replaceable
class="parameter">table</replaceable></literal>.

I believe "is not a partitioned table" here is intended to refer to both
plain tables and materialized views.
However, as far as I understand, using ONLY with a materialized view has
no effect.
So, wouldn’t it be better and clearer to say "if the table is a plain
table" instead?
I think the behavior for materialized views can be described along with
that for partitioned tables. For example:

<command>COPY TO</command> can be used with plain
tables, populated materialized views and partitioned tables.
For example, if <replaceable class="parameter">table</replaceable>
is a plain table,
<literal>COPY <replaceable class="parameter">table</replaceable>
TO</literal> copies the same rows as
<literal>SELECT * FROM ONLY <replaceable
class="parameter">table</replaceable></literal>.

If <replaceable class="parameter">table</replaceable> is a
partitioned table or a materialized view,
<literal>COPY <replaceable class="parameter">table</replaceable>
TO</literal>
copies the same rows as <literal>SELECT * FROM <replaceable
class="parameter">table</replaceable></literal>.

+ List *children = NIL;
...

@@ -673,11 +680,34 @@ BeginCopyTo(ParseState *pstate,
errmsg("cannot copy from sequence \"%s\"",
RelationGetRelationName(rel))));
else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy from partitioned table
\"%s\"",
- RelationGetRelationName(rel)),
- errhint("Try the COPY (SELECT ...) TO
variant.")));
+ {
+ children = find_all_inheritors(RelationGetRelid(rel),

Since 'children' is only used inside the else if block, I think we don't
need the separate "List *children = NIL;" declaration.
Instead, it could just be "List *children = find_all_inheritors(...)".

[1]
https://www.postgresql.org/message-id/flat/CANhcyEW_s2LD6RiDSMHtWQnpYB67EWXqf7N8mn7dOrnaKMfROg%40mail.gmail.com#7bfbe70576e7a3f7162ca268f08bd395

--
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.