On Mon, 2019-04-22 at 21:45 +0900, Etsuro Fujita wrote:
Thanks for looking into this!
> (2019/04/20 20:53), Laurenz Albe wrote:
> > On Fri, 2018-04-06 at 23:24 +0000, Robert Haas wrote:
> > > Allow insert and update tuple routing and COPY for foreign tables.
> > >
> > > Also enable this for postgres_fdw.
> > >
> > > Etsuro Fujita, based on an earlier patch by Amit Langote. The larger
> > > patch series of which this is a part has been reviewed by Amit
> > > Langote, David Fetter, Maksim Milyutin, Álvaro Herrera, Stephen Frost,
> > > and me. Minor documentation changes to the final version by me.
> > >
> > > Discussion:
> > > http://postgr.es/m/29906a26-da12-8c86-4fb9-d8f88442f...@lab.ntt.co.jp
> >
> > This commit makes life hard for foreign data wrappers that support
> > data modifications.
> >
> > If a FDW implements ExecForeignInsert, this commit automatically assumes
> > that it also supports COPY FROM. It will call ExecForeignInsert without
> > calling PlanForeignModify and BeginForeignModify, and a FDW that does not
> > expect that will probably fail.
>
> This is not 100% correct; the FDW documentation says:
>
> <para>
> Tuples inserted into a partitioned table by
> <command>INSERT</command> or
> <command>COPY FROM</command> are routed to partitions. If an FDW
> supports routable foreign-table partitions, it should also provide the
> following callback functions. These functions are also called when
> <command>COPY FROM</command> is executed on a foreign table.
> </para>
I don't see the difference between the documentation and what I wrote above.
Before v11, a FDW could expect that ExecForeignInsert is only called if
BeginForeignModify was called earlier.
That has silently changed with v11.
> > maybe there are FDWs that support INSERT but don't want to support COPY
> > for some reason.
>
> I agree on that point.
>
> > I propose that PostgreSQL only allows COPY FROM on a foreign table if the
> > FDW
> > implements BeginForeignInsert. The attached patch implements that.
>
> I don't think that is a good idea, because there might be some FDWs that
> want to support COPY FROM on foreign tables without providing
> BeginForeignInsert. (As for INSERT into foreign tables, we actually
> allow FDWs to support it without providing PlanForeignModify,
> BeginForeignModify, or EndForeignModify.)
>
> It's permissible to throw an error in BeginForeignInsert, so what I was
> thinking for FDWs that don't want to support COPY FROM and
> INSERT/UPDATE/COPY FROM tuple routing was to provide BeginForeignInsert
> implementing something like this:
>
> static void
> fooBeginForeignInsert(ModifyTableState *mtstate,
> ResultRelInfo *resultRelInfo)
> {
> Relation rel = resultRelInfo->ri_RelationDesc;
>
> if (mtstate->ps.plan == NULL)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot copy to foreign table \"%s\"",
> RelationGetRelationName(rel))));
> else
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot route tuples into foreign table \"%s\"",
> RelationGetRelationName(rel))));
> }
Sure, it is not hard to modify a FDW to continue working with v11.
My point is that this should not be necessary.
If a FDW worked well with v10, it should continue to work with v11
unless there is a necessity for a compatibility-breaking change.
On the other hand, if a FDW wants to support COPY in v11 and has no
need for BeginForeignInsert to support that, it is a simple exercise
for it to provide an empty BeginForeignInsert just to signal that it
wants to support COPY.
I realized that my previous patch forgot to check for tuple routing,
updated patch attached.
Yours,
Laurenz Albe
From 545ac9d567ea6ca610ce08355451923cc787e13d Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.a...@cybertec.at>
Date: Mon, 22 Apr 2019 21:35:06 +0200
Subject: [PATCH] Foreign table COPY FROM and tuple routing requires
BeginForeignInsert
Commit 3d956d956a introduced support for foreign tables as partitions
and COPY FROM on foreign tables.
If a foreign data wrapper supports data modifications, but either has
not adapted to this change yet or doesn't want to support it
for other reasons, it probably got broken by the above commit,
because COPY will just call ExecForeignInsert anyway, which might not
work because neither PlanForeignModify nor BeginForeignModify have
been called.
To avoid breaking third-party foreign data wrappers in that way, allow
COPY FROM and tuple routing for foreign tables only if the foreign data
wrapper implements BeginForeignInsert.
---
doc/src/sgml/fdwhandler.sgml | 2 ++
src/backend/commands/copy.c | 5 +++++
src/backend/executor/execPartition.c | 17 ++++++++++++-----
3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 2c07a86637..84f06d7f81 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -724,6 +724,8 @@ BeginForeignInsert(ModifyTableState *mtstate,
perform any initialization needed prior to the actual insertion.
Subsequently, <function>ExecForeignInsert</function> will be called for
each tuple to be inserted into the foreign table.
+ All foreign data wrappers that support <command>COPY FROM</command> or
+ partitions that are foreign tables have to provide this callback function.
</para>
<para>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c39218f8db..2e09af0070 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2857,6 +2857,11 @@ CopyFrom(CopyState cstate)
resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate,
resultRelInfo);
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot copy to foreign table \"%s\"",
+ RelationGetRelationName(cstate->rel))));
/* Prepare to catch AFTER triggers. */
AfterTriggerBeginQuery();
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 50800aa5ca..0316d4cbd4 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -939,12 +939,19 @@ ExecInitRoutingInfo(ModifyTableState *mtstate,
partrouteinfo->pi_PartitionToRootMap = NULL;
/*
- * If the partition is a foreign table, let the FDW init itself for
- * routing tuples to the partition.
+ * If the partition is a foreign table and the FDW supports it,
+ * let it init itself for routing tuples to the partition.
*/
- if (partRelInfo->ri_FdwRoutine != NULL &&
- partRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
- partRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, partRelInfo);
+ if (partRelInfo->ri_FdwRoutine != NULL)
+ {
+ if (partRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
+ partRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, partRelInfo);
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot route inserted tuples to foreign table \"%s\"",
+ RelationGetRelationName(partRelInfo->ri_RelationDesc))));
+ }
partRelInfo->ri_PartitionInfo = partrouteinfo;
partRelInfo->ri_CopyMultiInsertBuffer = NULL;
--
2.20.1
