On 2025-06-05 09:45, jian he wrote:
hi.
In the V10 patch, there will be some regression if the partition column
ordering is different from the root partitioned table.
because in V10 CopyThisRelTo
+ while (table_scan_getnextslot(scandesc, ForwardScanDirection, slot))
+ {
+ if (map != NULL)
+ {
+ original_slot = slot;
+ root_slot = MakeSingleTupleTableSlot(rootdesc,
&TTSOpsBufferHeapTuple);
+ slot = execute_attr_map_slot(map, slot, root_slot);
+ }
+ else
+ slot_getallattrs(slot);
+
+ if (original_slot != NULL)
+ ExecDropSingleTupleTableSlot(original_slot);
+}
as you can see, for each slot in the partition, i called
MakeSingleTupleTableSlot to get the dumpy root_slot
and ExecDropSingleTupleTableSlot too.
that will cause overhead.
we can call produce root_slot before the main while loop.
like the following:
+ if (root_rel != NULL)
+ {
+ rootdesc = RelationGetDescr(root_rel);
+ root_slot = table_slot_create(root_rel, NULL);
+ map = build_attrmap_by_name_if_req(rootdesc, tupdesc, false);
+ }
....
+ while (table_scan_getnextslot(scandesc, ForwardScanDirection, slot))
+ {
+ TupleTableSlot *copyslot;
+ if (map != NULL)
+ copyslot = execute_attr_map_slot(map, slot, root_slot);
+ else
+ {
+ slot_getallattrs(slot);
+ copyslot = slot;
+ }
+
please check CopyThisRelTo in v11. so, with v11, there is no
regression for case when
partition column ordering differs from partitioned.
Thanks for working on this improvement.
Here are some minor comments on v11 patch:
+ For example, if <replaceable class="parameter">table</replaceable>
is not partitioned table,
<literal>COPY <replaceable class="parameter">table</replaceable>
TO</literal> copies the same rows as
<literal>SELECT * FROM ONLY <replaceable
class="parameter">table</replaceable></literal>
This describes the behavior when the table is not partitioned, but would
it also be helpful to mention the behavior when the table is a
partitioned table?
For example:
If table is a partitioned table, then COPY table TO copies the same
rows as SELECT * FROM table.
+ * if COPY TO source table is a partitioned table, then open
each
if -> If
+ scan_rel = table_open(scan_oid,
AccessShareLock);
- /* Format and send the data */
- CopyOneRowTo(cstate, slot);
+ CopyThisRelTo(cstate, scan_rel, cstate->rel,
&processed);
- /*
- * Increment the number of processed tuples,
and report the
- * progress.
- */
-
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
-
++processed);
+ table_close(scan_rel, AccessShareLock)
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);
``
+/*
+ * 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?
+ * A partition's rowtype might differ from the root table's.
Since we are
+ * export partitioned table data here, we must convert it back
to the root
are export -> are exporting?
--
Regards,
--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.
