On Thu, Oct 9, 2025 at 12:10 AM jian he <[email protected]> wrote:
>
> On Thu, Oct 9, 2025 at 9:14 AM Masahiko Sawada <[email protected]> wrote:
> >
> >
> > ---
> > + relation_name = get_rel_name(childreloid);
> > + ereport(ERROR,
> > + errcode(ERRCODE_WRONG_OBJECT_TYPE),
> > + errmsg("cannot copy from foreign table
> > \"%s\"", relation_name),
> > + errdetail("Partition \"%s\" is a foreign
> > table in the partitioned table \"%s\"",
> > + relation_name,
> > RelationGetRelationName(rel)),
> > + errhint("Try the COPY (SELECT ...) TO
> > variant."));
> >
> > I think we don't need "the" in the error message.
> >
> > It's conventional to put all err*() macros in parentheses (i.e.,
> > "(errcode(), ...)", it's technically omittable though.
> >
>
> https://www.postgresql.org/docs/current/error-message-reporting.html
> QUOTE:
> <<<<>>>>>
> The extra parentheses were required before PostgreSQL version 12, but
> are now optional.
> Here is a more complex example:
> .....
> <<<<>>>>>
>
> related commit:
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=e3a87b4991cc2d00b7a3082abb54c5f12baedfd1
> Less parenthesis is generally more readable, I think.
Yes, but I think it's more consistent given that we use the
parentheses in all other places in copyto.c.
>
> > How about doing "slot = execute_attr_map_slot(map, slot, root_slot);"
> > instead? (i.e., no need to have 'copyslot')
> >
>
> I tried but it seems not possible.
> table_scan_getnextslot function require certain type of "slot", if we do
> "slot = execute_attr_map_slot(map, slot, root_slot);"
> then pointer "slot" type becomes virtual slot, then
> it will fail on second time call table_scan_getnextslot
Right. Let's keep as it is.
I've attached a patch for cosmetic changes including comment updates,
indent fixes by pgindent, and renaming variable names. Some fixes are
just my taste, so please check the changes.
Also I have a few comments on new tests:
+-- Tests for COPY TO with partitioned tables.
+CREATE TABLE pp (id int,val int) PARTITION BY RANGE (id);
+CREATE TABLE pp_1 (val int, id int) PARTITION BY RANGE (id);
+CREATE TABLE pp_2 (val int, id int) PARTITION BY RANGE (id);
+ALTER TABLE pp ATTACH PARTITION pp_1 FOR VALUES FROM (1) TO (5);
+ALTER TABLE pp ATTACH PARTITION pp_2 FOR VALUES FROM (5) TO (10);
+
+CREATE TABLE pp_15 PARTITION OF pp_1 FOR VALUES FROM (1) TO (5);
+CREATE TABLE pp_510 PARTITION OF pp_2 FOR VALUES FROM (5) TO (10);
+
+INSERT INTO pp SELECT g, 10 + g FROM generate_series(1,6) g;
+
I think it's better to have both cases: partitions' rowtype match the
root's rowtype and partition's rowtype doesn't match the root's
rowtype.
---
+-- Test COPY TO with a foreign table or when the foreign table is a partition
+COPY async_p3 TO stdout; --error
+ERROR: cannot copy from foreign table "async_p3"
+HINT: Try the COPY (SELECT ...) TO variant.
async_p3 is a foreign table so it seems not related to this patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index aba76eb0173..7d5067d90b5 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -119,8 +119,8 @@ static void CopyOneRowTo(CopyToState cstate, TupleTableSlot
*slot);
static void CopyAttributeOutText(CopyToState cstate, const char *string);
static void CopyAttributeOutCSV(CopyToState cstate, const char *string,
bool use_quote);
-static void CopyRelTo(CopyToState cstate, Relation rel, Relation root_rel,
- uint64 *processed);
+static void CopyRelationTo(CopyToState cstate, Relation rel, Relation root_rel,
+ uint64 *processed);
/* built-in format-specific routines */
static void CopyToTextLikeStart(CopyToState cstate, TupleDesc tupDesc);
@@ -681,32 +681,30 @@ BeginCopyTo(ParseState *pstate,
else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
/*
- * Collect a list of partitions containing data, so
that later
+ * Collect OIDs of relation containing data, so that
later
* DoCopyTo can copy the data from them.
- */
- children = find_all_inheritors(RelationGetRelid(rel),
-
AccessShareLock,
-
NULL);
+ */
+ children = find_all_inheritors(RelationGetRelid(rel),
AccessShareLock, NULL);
- foreach_oid(childreloid, children)
+ foreach_oid(child, children)
{
- char relkind =
get_rel_relkind(childreloid);
+ char relkind =
get_rel_relkind(child);
if (relkind == RELKIND_FOREIGN_TABLE)
{
- char *relation_name;
+ char *relation_name =
get_rel_name(child);
- relation_name =
get_rel_name(childreloid);
ereport(ERROR,
-
errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy
from foreign table \"%s\"", relation_name),
- errdetail("Partition
\"%s\" is a foreign table in the partitioned table \"%s\"",
-
relation_name, RelationGetRelationName(rel)),
- errhint("Try the COPY
(SELECT ...) TO variant."));
+
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot copy
from foreign table \"%s\"", relation_name),
+ errdetail("Partition
\"%s\" is a foreign table in partitioned table \"%s\"",
+
relation_name, RelationGetRelationName(rel)),
+ errhint("Try the COPY
(SELECT ...) TO variant.")));
}
+ /* Exclude tables with no data */
if (RELKIND_HAS_PARTITIONS(relkind))
- children =
foreach_delete_current(children, childreloid);
+ children =
foreach_delete_current(children, child);
}
}
else
@@ -1106,21 +1104,21 @@ DoCopyTo(CopyToState cstate)
/*
* If COPY TO source table is a partitioned table, then open
each
* partition and process each individual partition.
- */
+ */
if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
- foreach_oid(scan_oid, cstate->partitions)
+ foreach_oid(child, cstate->partitions)
{
- Relation scan_rel;
+ Relation scan_rel;
- /* We already got the needed lock in
BeginCopyTo */
- scan_rel = table_open(scan_oid, NoLock);
- CopyRelTo(cstate, scan_rel, cstate->rel,
&processed);
+ /* We already got the lock in BeginCopyTo */
+ scan_rel = table_open(child, NoLock);
+ CopyRelationTo(cstate, scan_rel, cstate->rel,
&processed);
table_close(scan_rel, NoLock);
}
}
else
- CopyRelTo(cstate, cstate->rel, NULL, &processed);
+ CopyRelationTo(cstate, cstate->rel, NULL, &processed);
}
else
{
@@ -1140,22 +1138,19 @@ DoCopyTo(CopyToState cstate)
}
/*
- * Scan a single table (which may be a partition) and export its rows to the
- * COPY destination.
+ * Scans a single table and exports its rows to the COPY destination.
*
- * rel: the table from which the actual data will be copied.
- * root_rel: if not NULL, it indicates that COPY TO command copy partitioned
- * table data to the destination, and "rel" is the partition of "root_rel".
- * processed: number of tuples processed.
+ * root_rel can be set to the root table of rel if rel is a partition
+ * table so that we can send tuples in root_rel's rowtype, which might
+ * differ from individual partitions.
*/
static void
-CopyRelTo(CopyToState cstate, Relation rel, Relation root_rel,
- uint64 *processed)
+CopyRelationTo(CopyToState cstate, Relation rel, Relation root_rel, uint64
*processed)
{
TupleTableSlot *slot;
TableScanDesc scandesc;
- AttrMap *map = NULL;
- TupleTableSlot *root_slot = NULL;
+ AttrMap *map = NULL;
+ TupleTableSlot *root_slot = NULL;
scandesc = table_beginscan(rel, GetActiveSnapshot(), 0, NULL);
slot = table_slot_create(rel, NULL);
@@ -1164,7 +1159,7 @@ CopyRelTo(CopyToState cstate, Relation rel, Relation
root_rel,
* A partition's rowtype might differ from the root table's. If we are
* exporting partition data here, we must convert it back to the root
* table's rowtype.
- */
+ */
if (root_rel != NULL)
{
root_slot = table_slot_create(root_rel, NULL);
@@ -1192,8 +1187,7 @@ CopyRelTo(CopyToState cstate, Relation rel, Relation
root_rel,
CopyOneRowTo(cstate, copyslot);
/*
- * Increment the number of processed tuples, and report the
- * progress.
+ * Increment the number of processed tuples, and report the
progress.
*/
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
++(*processed));
