Re: pgsql: Allow insert and update tuple routing and COPY forforeign table - Mailing list pgsql-committers

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