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

On Tue, Apr 1, 2025 at 1:38 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 1 Apr 2025 at 06:31, jian he <jian.universality@gmail.com> wrote:
> >
> > On Mon, Mar 31, 2025 at 4:05 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> >
> > Thanks for doing the benchmark.
>
> Few comments to improve the comments, error message and remove
> redundant assignment:
> 1) How about we change below:
> /*
>  * partition's rowtype might differ from the root table's.  We must
>  * convert it back to the root table's rowtype as we are export
>  * partitioned table data here.
> */
> To:
> /*
>  * A partition's row type might differ from the root table's.
>  * Since we're exporting partitioned table data, we must
>  * convert it back to the root table's row type.
>  */
>
i changed it to
    /*
     * A partition's rowtype might differ from the root table's.
     * Since we are export partitioned table data here,
     * we must convert it back to the root table's rowtype.
    */
Since many places use "rowtype",
using "rowtype" instead of "row type" should be fine.


> 2) How about we change below:
> +                               if (relkind == RELKIND_FOREIGN_TABLE)
> +                                       ereport(ERROR,
> +
> errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                                                       errmsg("cannot
> copy from foreign table \"%s\"",
> +
>  RelationGetRelationName(rel)),
> +
> errdetail("partition \"%s\" is a foreign table",
> RelationGetRelationName(rel)),
> +                                                       errhint("Try
> the COPY (SELECT ...) TO variant."));
>
> To:
> +                               if (relkind == RELKIND_FOREIGN_TABLE)
> +                                       ereport(ERROR,
> +
> errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                                                       errmsg("cannot
> copy from a partitioned table having foreign table partition \"%s\"",
> +
>  RelationGetRelationName(rel)),
> +
> errdetail("partition \"%s\" is a foreign table",
> RelationGetRelationName(rel)),
> +                                                       errhint("Try
> the COPY (SELECT ...) TO variant."));
>
i am not so sure.
since the surrounding code we have

else if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
            ereport(ERROR,
                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                     errmsg("cannot copy from foreign table \"%s\"",
                            RelationGetRelationName(rel)),
                     errhint("Try the COPY (SELECT ...) TO variant.")));

let's see what others think.
> 3) How about we change below:
> /*
>  * rel: the relation to be copied to.
>  * root_rel: if not NULL, then the COPY partitioned relation to destination.
>  * processed: number of tuples processed.
> */
> To:
> /*
>  * rel: the relation from which data will be copied.
>  * root_rel: If not NULL, indicates that rel's row type must be
>  *           converted to root_rel's row type.
>  * processed: number of tuples processed.
>  */
>
i changed it to

/*
 * 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.
*/

>  4)  You can initialize processed to 0 along with declaration in
> DoCopyTo function and remove the below:
>  +       if (cstate->rel && cstate->rel->rd_rel->relkind ==
> RELKIND_PARTITIONED_TABLE)
>         {
> ...
> ...
>                 processed = 0;
> -               while (table_scan_getnextslot(scandesc,
> ForwardScanDirection, slot))
> ...
> ...
> -
> -               ExecDropSingleTupleTableSlot(slot);
> -               table_endscan(scandesc);
> +       }
> +       else if (cstate->rel)
> +       {
> +               processed = 0;
> +               CopyThisRelTo(cstate, cstate->rel, NULL, &processed);
>         }
ok.