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