Re: speedup COPY TO for partitioned table.

On Mon, Jun 30, 2025 at 3:57 PM torikoshia <torikoshia(at)oss(dot)nttdata(dot)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