Re: speedup COPY TO for partitioned table.

On Tue, Apr 1, 2025 at 1:38 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 1 Apr 2025 at 06:31, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> >
> > On Mon, Mar 31, 2025 at 4:05 PM Kirill Reshke <reshkekirill(at)gmail(dot)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.