Re: speedup COPY TO for partitioned table. - Mailing list pgsql-hackers

On Mon, Jun 30, 2025 at 3:57 PM torikoshia <torikoshia@oss.nttdata.com> wrote:
>
> >> ```
> >>                           scan_rel = table_open(scan_oid,
> >> AccessShareLock);
> >>
> >>                           CopyThisRelTo(cstate, scan_rel, cstate->rel,
> >> &processed);
> >>
> >>                           table_close(scan_rel, AccessShareLock);
> >> ``
> >>
> > we can remove these empty new lines.
> > actually, I realized we don't need to use AccessShareLock here—we can
> > use NoLock
> > instead, since BeginCopyTo has already acquired AccessShareLock via
> > find_all_inheritors.
>
> That makes sense.
> I think it would be helpful to add a comment explaining why NoLock is
> safe here — for example:
>
>    /* We already got the needed lock */
>
> In fact, in other places where table_open(..., NoLock) is used, similar
> explanatory comments are often included(Above comment is one of them).
>

hi.

I changed it to:
        foreach_oid(scan_oid, cstate->partitions)
        {
            Relation        scan_rel;
            /* We already got the needed lock in BeginCopyTo */
            scan_rel = table_open(scan_oid, NoLock);
            CopyRelTo(cstate, scan_rel, cstate->rel, &processed);
            table_close(scan_rel, NoLock);
        }

> > what do you think the following
> >
> > /*
> >  * CopyThisRelTo:
> >  * This will scanning a single table (which may be a partition) and
> > exporting
> >  * its rows to a COPY destination.
> >  *
> >  * rel: the relation from which the actual data will be copied.
> >  * root_rel: if not NULL, it indicates that we are copying partitioned
> > relation
> >  * data to the destination, and "rel" is the partition of "root_rel".
> >  * processed: number of tuples processed.
> > */
> > static void
> > CopyThisRelTo(CopyToState cstate, Relation rel, Relation root_rel,
> >               uint64 *processed)
>
> I think it would be better to follow the style of nearby functions in
> the same file. For example:
>
>    /*
>     * Scan a single table (which may be a partition) and export
>     * its rows to the COPY destination.
>     */
>

now it is:
/*
 * Scan a single table (which may be a partition) and export its rows to the
 * COPY destination.
 *
 * rel: the relation from which the actual data will be copied.
 * root_rel: if not NULL, it indicates that we are copying partitioned relation
 * data to the destination, and "rel" is the partition of "root_rel".
 * processed: number of tuples processed.
*/
static void
CopyRelTo(CopyToState cstate, Relation rel, Relation root_rel,
          uint64 *processed)


> Also, regarding the function name CopyThisRelTo() — I wonder if the
> "This" is really necessary?
> Maybe something simpler like CopyRelTo() is enough.
>
> What do you think?
>
sure. CopyRelTo looks good to me.

while at it.
I found that in BeginCopyTo:
            ereport(ERROR,
                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                     errmsg("cannot copy from foreign table \"%s\"",
                            RelationGetRelationName(rel)),
                     errhint("Try the COPY (SELECT ...) TO variant.")));

                    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."));

don't have any regress tests on it.
see https://coverage.postgresql.org/src/backend/commands/copyto.c.gcov.html
So I added some tests on contrib/postgres_fdw/sql/postgres_fdw.sql