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@kurilemu.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.