Re: pgsql: Allow insert and update tuple routing and COPY for foreign table

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-d8f88442f2b9@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