Re: speedup COPY TO for partitioned table.

On Fri, Oct 10, 2025 at 6:03 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > > ---
> > > + 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."));
> > >
> > > I think we don't need "the" in the error message.
> > >
> > > It's conventional to put all err*() macros in parentheses (i.e.,
> > > "(errcode(), ...)", it's technically omittable though.
> > >
> >
> > https://www.postgresql.org/docs/current/error-message-reporting.html
> > QUOTE:
> > <<<<>>>>>
> > The extra parentheses were required before PostgreSQL version 12, but
> > are now optional.
> > Here is a more complex example:
> > .....
> > <<<<>>>>>
> >
> > related commit:
> > https://git.postgresql.org/cgit/postgresql.git/commit/?id=e3a87b4991cc2d00b7a3082abb54c5f12baedfd1
> > Less parenthesis is generally more readable, I think.
>
> Yes, but I think it's more consistent given that we use the
> parentheses in all other places in copyto.c.
>

If you look at tablecmds.c, like ATExecSetNotNull, there are
parentheses and no parentheses cases.
Overall, I think less parentheses improves readability and makes the
code more future-proof.

> >
> > > How about doing "slot = execute_attr_map_slot(map, slot, root_slot);"
> > > instead? (i.e., no need to have 'copyslot')
> > >
> >
> > I tried but it seems not possible.
> > table_scan_getnextslot function require certain type of "slot", if we do
> > "slot = execute_attr_map_slot(map, slot, root_slot);"
> > then pointer "slot" type becomes virtual slot, then
> > it will fail on second time call table_scan_getnextslot
>
> Right. Let's keep as it is.
>

> I've attached a patch for cosmetic changes including comment updates,
> indent fixes by pgindent, and renaming variable names. Some fixes are
> just my taste, so please check the changes.
>
thanks!
I have applied most of it. expect points I mentioned in this email.

> Also I have a few comments on new tests:
>
> +-- Tests for COPY TO with partitioned tables.
> +CREATE TABLE pp (id int,val int) PARTITION BY RANGE (id);
> +CREATE TABLE pp_1 (val int, id int) PARTITION BY RANGE (id);
> +CREATE TABLE pp_2 (val int, id int) PARTITION BY RANGE (id);
> +ALTER TABLE pp ATTACH PARTITION pp_1 FOR VALUES FROM (1) TO (5);
> +ALTER TABLE pp ATTACH PARTITION pp_2 FOR VALUES FROM (5) TO (10);
> +
> +CREATE TABLE pp_15 PARTITION OF pp_1 FOR VALUES FROM (1) TO (5);
> +CREATE TABLE pp_510 PARTITION OF pp_2 FOR VALUES FROM (5) TO (10);
> +
> +INSERT INTO pp SELECT g, 10 + g FROM generate_series(1,6) g;
> +
>
> I think it's better to have both cases: partitions' rowtype match the
> root's rowtype and partition's rowtype doesn't match the root's
> rowtype.
>
sure.

> ---
> +-- Test COPY TO with a foreign table or when the foreign table is a partition
> +COPY async_p3 TO stdout; --error
> +ERROR: cannot copy from foreign table "async_p3"
> +HINT: Try the COPY (SELECT ...) TO variant.
>
> async_p3 is a foreign table so it seems not related to this patch.
>
I replied in
https://postgr.es/m/CACJufxGkkMtRUJEbLczRnWp7x2YWqu4r1gEJEv9Po1UPxS6kGQ%40mail.gmail.com
I kind of doubt anyone would submit a patch just to rewrite a coverage test for
the sake of coverage itself. While we're here, adding nearby coverage tests
should be fine?

i just found out I ignored the case when partitioned tables have RLS.
when exporting a partitioned table,
find_all_inheritors will sort the returned partition by oid.
in DoCopy, we can do the same:
make a SortBy node for SelectStmt->sortClause also mark the
RangeVar->inh as true.
OR
ereport(ERRCODE_FEATURE_NOT_SUPPORTED...) for partitioned tables with RLS.

please see the change I made in DoCopy.