Re: [HACKERS] Add support for tuple routing to foreign partitions

(2018/02/22 11:52), Amit Langote wrote:
> On 2018/02/21 20:54, Etsuro Fujita wrote:
>> void
>> BeginForeignRouting(ModifyTableState *mtstate,
>>                      ResultRelInfo *resultRelInfo,
>>                      int partition_index);
>>
>> Prepare for a tuple-routing operation on a foreign table.  This is called
>> from ExecSetupPartitionTupleRouting and ExecInitPartitionInfo.
>
> I wonder why partition_index needs to be made part of this API?

The reason for that is because I think the FDW might want to look at the 
partition info stored in mtstate->mt_partition_tuple_routing for some 
reason or other, such as parent_child_tupconv_maps and 
child_parent_tupconv_maps, which are accessed with the given partition 
index.

>> Patch foreign-routing-fdwapi-WIP.patch: main patch to add new FDW APIs,
>> which is created on top of patch postgres-fdw-refactoring-WIP.patch and
>> the lazy-initialization-of-partition-info patch [1].
>
> Noticed a typo in the patch (s/parition/partition/g):
>
> +                 * Also let the FDW init itself if this parition is foreign.
>
> +     * Also let the FDW init itself if this parition is foreign.

Good catch!  Will fix.

> Perhaps an independent concern, but one thing I noticed is that it does
> not seem to play well with the direct modification (update push-down)
> feature.  Now because updates (at least local, let's say) support
> re-routing, I thought we'd be able move rows across servers via the local
> server, but with direct modification we'd never get the chance.  However,
> since update tuple routing is triggered by partition constraint failure,
> which we don't enforce for foreign tables/partitions anyway, I'm not sure
> if we need to do anything about that, and even if we did, whether it
> concerns this proposal.

Good point!  Actually, update tuple routing we have in HEAD doesn't 
allow re-routing tuples from foreign partitions even without direct 
modification.  Here is an example using postgres_fdw:

postgres=# create table pt (a int, b text) partition by list (a);
CREATE TABLE
postgres=# create table loct (a int check (a in (1)), b text);
CREATE TABLE
postgres=# create foreign table remp partition of pt for values in (1) 
server loopback options (table_name 'loct');
CREATE FOREIGN TABLE
postgres=# create table locp partition of pt for values in (2);
CREATE TABLE
postgres=# insert into remp values (1, 'foo');
INSERT 0 1
postgres=# insert into locp values (2, 'bar');
INSERT 0 1
postgres=# select tableoid::regclass, * from pt;
  tableoid | a |  b
----------+---+-----
  remp     | 1 | foo
  locp     | 2 | bar
(2 rows)

postgres=# create function postgres_fdw_abs(int) returns int as $$begin 
return abs($1); end$$ language plpgsql immutable;
CREATE FUNCTION
postgres=# explain verbose update pt set a = postgres_fdw_abs(a) + 1 
where b = 'foo';
                                          QUERY PLAN

--------------------------------------------------------------------------------
-------------
  Update on public.pt  (cost=100.00..154.54 rows=12 width=42)
    Foreign Update on public.remp
      Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1
    Update on public.locp
    ->  Foreign Scan on public.remp  (cost=100.00..127.15 rows=6 width=42)
          Output: (postgres_fdw_abs(remp.a) + 1), remp.b, remp.ctid
          Remote SQL: SELECT a, b, ctid FROM public.loct WHERE ((b = 
'foo'::text)) FOR UPDATE
    ->  Seq Scan on public.locp  (cost=0.00..27.39 rows=6 width=42)
          Output: (postgres_fdw_abs(locp.a) + 1), locp.b, locp.ctid
          Filter: (locp.b = 'foo'::text)
(10 rows)

postgres=# update pt set a = postgres_fdw_abs(a) + 1 where b = 'foo';
ERROR:  new row for relation "loct" violates check constraint "loct_a_check"
DETAIL:  Failing row contains (2, foo).
CONTEXT:  Remote SQL command: UPDATE public.loct SET a = $2 WHERE ctid = $1

To be able to move tuples across foreign servers, I think we would first 
have to do something to allow re-routing tuples from foreign partitions. 
  The patches I proposed hasn't done anything about that, so the patched 
version would behave the same way as HEAD with/without direct modification.

> That said, I saw in the changes to ExecSetupPartitionTupleRouting() that
> BeginForeignRouting() is called for a foreign partition even if direct
> modification might already have been set up.  If direct modification is
> set up, then ExecForeignRouting() will never get called, because we'd
> never call ExecUpdate() or ExecInsert().

Yeah, but I am thinking to leave the support for re-routing tuples 
across foreign servers for another patch.

One difference between HEAD and the patched version would be: we can 
re-route tuples from a plain partition to a foreign partition if the 
foreign partition supports this tuple-routing API.  Here is an example 
using the above data:

postgres=# select tableoid::regclass, * from pt;
  tableoid | a |  b
----------+---+-----
  remp     | 1 | foo
  locp     | 2 | bar
(2 rows)

postgres=# update pt set a = 1 where b = 'bar';
UPDATE 1
postgres=# select tableoid::regclass, * from pt;
  tableoid | a |  b
----------+---+-----
  remp     | 1 | foo
  remp     | 1 | bar
(2 rows)

This would introduce an asymmetry (we can move tuples from plain 
partitions to foreign partitions, but the reverse is not true), but I am 
thinking that it would be probably okay to document about that.

Thank you for the review!

Best regards,
Etsuro Fujita