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.
