Re: speedup COPY TO for partitioned table.

On 2025-06-27 16:14, jian he wrote:
Thanks for updating the patch!

> On Thu, Jun 26, 2025 at 9:43 AM torikoshia <torikoshia@oss.nttdata.com> 
> wrote:
>> 
>> After applying the patch, blank lines exist between these statements 
>> as
>> below. Do we really need these blank lines?
>> 
>> ```
>>                           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).

>> > +/*
>> > + * 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,
>> 
>> This comment only describes the parameters. Wouldn't it better to add 
>> a
>> brief summary of what this function does overall?
>> 
> 
> 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.
    */

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?


Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.