> On Oct 9, 2025, at 22:50, jian he <[email protected]> wrote:
>> 3
>> ```
>> + if (RELKIND_HAS_PARTITIONS(relkind))
>> + children = foreach_delete_current(children, childreloid);
>> + }
>> ```
>>
>> I wonder if there is any specially consideration of using
>> RELKIND_HAS_PARTITIONS() here? Because according to the function comment of
>> find_all_inheritors(), it will only return OIDs of relations; while
>> RELKIND_HAS_PARTITIONS checks for both relations and views. Logically using
>> this macro works, but it may lead to some confusion to code readers.
>>
>
> find_all_inheritors comments says:
> * Returns a list of relation OIDs including the given rel plus
> * all relations that inherit from it, directly or indirectly.
>
> CREATE TABLE pp (id int,val int) PARTITION BY RANGE (id);
> CREATE TABLE pp_1 (val int, id int) PARTITION BY RANGE (id);
> ALTER TABLE pp ATTACH PARTITION pp_1 FOR VALUES FROM (1) TO (5);
>
> If we copy partitioned table "pp" data out, but partitioned table "pp_1"
> don't have storage, so we have to skip it, using RELKIND_HAS_PARTITIONS
> to skip it should be fine.
My point is that RELKIND_HAS_PARTITIONS is defined as:
#define RELKIND_HAS_PARTITIONS(relkind) \
((relkind) == RELKIND_PARTITIONED_TABLE || \
(relkind) == RELKIND_PARTITIONED_INDEX)
It just checks relkind to be table or index. The example in your explanation
seems to not address my concern. Why do we need to check against index?
>
>> 4
>> ```
>> @@ -722,6 +754,7 @@ BeginCopyTo(ParseState *pstate,
>> DestReceiver *dest;
>>
>> cstate->rel = NULL;
>> + cstate->partitions = NIL;
>> ```
>>
>> Both NULL assignment are not needed as cstate is allocated by palloc0().
>>
> I guess this is just a code convention. Such not necessary is quite common
> within the codebase.
I don’t agree. cstate has a lot of more fields with pointer types, why don’t
set NULL to them?
>
>> 5
>> ```
>> +static void
>> +CopyRelTo(CopyToState cstate, Relation rel, Relation root_rel,
>> + uint64 *processed)
>> ```
>>
>> Instead of using a pointer to pass out processed count, I think it’s better
>> to return the process count. I understand the current implementation allows
>> continuous increment while calling this function in a loop. However, it’s a
>> bit error-prone, a caller must make sure “processed” is well initialized.
>> With returning a unit64, the caller’s code is still simple:
>>
>> ```
>> processed += CopyRelTo(cstate, …);
>> ```
>>
> pgstat_progress_update_param was within CopyRelTo.
> so we have to pass (uint64 *processed) to CopyRelTo.
> Am I missing something?
>
Make sense. I didn’t notice postage_progress_update_param. So, “processed” is
both input and output. In that case, I think the comment for parameter
“processed” should be enhanced, for example:
```
* processed: on entry, contains the current count of processed count;
* this function increments it by the number of rows copied
* from this relation and writes back the updated total.
```
Or a short version:
```
* processed: input/output; cumulative count of tuples processed, incremented
here.
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
