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