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

On Thu, Jun 26, 2025 at 9:43 AM torikoshia <torikos...@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.


Reply via email to