Re: Tuple-routing for certain partitioned tables not working as expected

Fujita-san,

On 2017/08/07 12:45, Etsuro Fujita wrote:
> Hi,
>
> I noticed that tuple-routing for partitioned tables that contain
> non-writable foreign partitions doesn't work as expected. Here is an
> example:
>
> postgres=# create extension file_fdw;
> postgres=# create server file_server foreign data wrapper file_fdw;
> postgres=# create user mapping for CURRENT_USER server file_server;
> postgres=# create table p (a int) partition by list (a);
> postgres=# create foreign table t1 partition of p for values in (1) server
> file_server options (format 'csv', filename '/path/to/file', delimiter ',');
> postgres=# create table t2 partition of p for values in (2);
> postgres=# insert into p values (1);
> ERROR: cannot insert into foreign table "t1"
>
> Looks good, but:
>
> postgres=# insert into p values (2);
> ERROR: cannot insert into foreign table "t1"
>
> The insert should work but doesn't. (It also seems odd to me that the
> error message points to t1, not t2.) The reason for that is
> CheckValidResultRel in ExecSetupPartitionTupleRouting aborts any insert
> into a partitioned table in the case where the partitioned table contains
> at least one foreign partition into which the FDW can't insert. I don't
> think that that is intentional behavior, so I'd like to propose to fix
> that by skipping CheckValidResultRel for foreign partitions because we can
> abort an insert into a foreign partition after ExecFindPartition in
> ExecInsert, by checking to see if resultRelInfo->ri_FdwRoutine is not
> NULL. Attached is a proposed patch for that. Since COPY FROM has the
> same issue, I added regression tests for COPY FROM as well as INSERT to
> file_fdw.

Thanks for reporting this. All the issues you mention here seem valid to me.

So, once we add support in general for foreign partition tuple-routing,
we'd still get the same error ("cannot route to foreign partitions") in
the file_fdw's case, but only because
resultRelInfo->ri_FdwRoutine->ExecForeignInsert will be NULL for file_fdw.

The patch looks good too. Although, looking at the following hunk:

+ Assert(partrel->rd_rel->relkind == RELKIND_RELATION ||
+ partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
+
/*
* Verify result relation is a valid target for the current operation.
*/
! if (partrel->rd_rel->relkind == RELKIND_RELATION)
! CheckValidResultRel(partrel, CMD_INSERT);

makes me now wonder if we need the CheckValidResultRel check at all. The
only check currently in place for RELKIND_RELATION is
CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts anyway.

Thanks,
Amit