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.
