Re: bug in update tuple routing with foreign partitions

(2019/03/06 18:33), Amit Langote wrote:
> I noticed a bug with how UPDATE tuple routing initializes ResultRelInfos
> to use for partition routing targets. Specifically, the bug occurs when
> UPDATE targets include a foreign partition that is locally modified (as
> opposed to being modified directly on the remove server) and its
> ResultRelInfo (called subplan result rel in the source code) is reused for
> tuple routing:
>
> -- setup
> create extension postgres_fdw ;
> create server loopback foreign data wrapper postgres_fdw;
> create user mapping for current_user server loopback;
> create table p (a int) partition by list (a);
> create table p1 partition of p for values in (1);
> create table p2base (a int check (a = 2));
> create foreign table p2 partition of p for values in (2) server loopback
> options (table_name 'p2base');
> insert into p values (1), (2);
>
> -- see in the plan that foreign partition p2 is locally modified
> explain verbose update p set a = 2 from (values (1), (2)) s(x) where a =
> s.x returning *;
> QUERY PLAN
>
> ─────────────────────────────────────────────────────────────────────────────────
> Update on public.p (cost=0.05..236.97 rows=50 width=42)
> Output: p1.a, "*VALUES*".column1
> Update on public.p1
> Foreign Update on public.p2
> Remote SQL: UPDATE public.p2base SET a = $2 WHERE ctid = $1 RETURNING a
> -> Hash Join (cost=0.05..45.37 rows=26 width=42)
> Output: 2, p1.ctid, "*VALUES*".*, "*VALUES*".column1
> Hash Cond: (p1.a = "*VALUES*".column1)
> -> Seq Scan on public.p1 (cost=0.00..35.50 rows=2550 width=10)
> Output: p1.ctid, p1.a
> -> Hash (cost=0.03..0.03 rows=2 width=32)
> Output: "*VALUES*".*, "*VALUES*".column1
> -> Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2
> width=32)
> Output: "*VALUES*".*, "*VALUES*".column1
> -> Hash Join (cost=100.05..191.59 rows=24 width=42)
> Output: 2, p2.ctid, "*VALUES*".*, "*VALUES*".column1
> Hash Cond: (p2.a = "*VALUES*".column1)
> -> Foreign Scan on public.p2 (cost=100.00..182.27 rows=2409
> width=10)
> Output: p2.ctid, p2.a
> Remote SQL: SELECT a, ctid FROM public.p2base FOR UPDATE
> -> Hash (cost=0.03..0.03 rows=2 width=32)
> Output: "*VALUES*".*, "*VALUES*".column1
> -> Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2
> width=32)
> Output: "*VALUES*".*, "*VALUES*".column1
>
>
> -- as opposed to directly on remote side (because there's no local join)
> explain verbose update p set a = 2 returning *;
> QUERY PLAN
> ─────────────────────────────────────────────────────────────────────────────
> Update on public.p (cost=0.00..227.40 rows=5280 width=10)
> Output: p1.a
> Update on public.p1
> Foreign Update on public.p2
> -> Seq Scan on public.p1 (cost=0.00..35.50 rows=2550 width=10)
> Output: 2, p1.ctid
> -> Foreign Update on public.p2 (cost=100.00..191.90 rows=2730 width=10)
> Remote SQL: UPDATE public.p2base SET a = 2 RETURNING a
> (8 rows)
>
> Running the first update query crashes:
>
> update p set a = 2 from (values (1), (2)) s(x) where a = s.x returning
> tableoid::regclass, *;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>
> The problem seems to occur because ExecSetupPartitionTupleRouting thinks
> it can reuse p2's ResultRelInfo for tuple routing. In this case, it can't
> be used, because its ri_FdwState contains information that will be needed
> for postgresExecForeignUpdate to work, but it's still used today. Because
> it's assigned to be used for tuple routing, its ri_FdwState will be
> overwritten by postgresBeginForeignInsert that's invoked by the tuple
> routing code using the aforementioned ResultRelInfo. Crash occurs when
> postgresExecForeignUpdate () can't find the information it's expecting in
> the ri_FdwState.

Agreed, as I said before in another thread.

> The solution is to teach ExecSetupPartitionTupleRouting to avoid using a
> subplan result rel if its ri_FdwState is already set.

I'm not sure that is a good idea, because that requires to create a new
ResultRelInfo, which is not free; I think it requires a lot of work.
Another solution to avoid that would be to store the fmstate created by
postgresBeginForeignInsert() into the ri_FdwState, not overwrite the
ri_FdwState, like the attached. This would not need any changes to the
core, and this would not cause any overheads either, IIUC. What do you
think about that?

> I have attached 2 patches, one for PG 11 where the problem first appeared
> and another for HEAD. The patch for PG 11 is significantly bigger due to
> having to handle the complexities of mapping of subplan result rel indexes
> to leaf partition indexes. Most of that complexity is gone in HEAD due to
> 3f2393edefa5, so the patch for HEAD is much simpler.

Thanks for the patches!

> I've also added a
> test in postgres_fdw.sql to exercise this test case.

Thanks again, but the test case you added works well without any fix:

+-- Check case where the foreign partition is a subplan target rel and
+-- foreign parttion is locally modified (target table being joined
+-- locally prevents a direct/remote modification plan).
+explain (verbose, costs off)
+update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x
returning *;
+ QUERY PLAN

+------------------------------------------------------------------------------
+ Update on public.utrtest
+ Output: remp.a, remp.b, "*VALUES*".column1
+ Foreign Update on public.remp
+ Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1
RETURNING a, b
+ Update on public.locp
+ -> Hash Join
+ Output: 1, remp.b, remp.ctid, "*VALUES*".*, "*VALUES*".column1
+ Hash Cond: (remp.a = "*VALUES*".column1)
+ -> Foreign Scan on public.remp
+ Output: remp.b, remp.ctid, remp.a
+ Remote SQL: SELECT a, b, ctid FROM public.loct FOR UPDATE
+ -> Hash
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Values Scan on "*VALUES*"
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Hash Join
+ Output: 1, locp.b, locp.ctid, "*VALUES*".*, "*VALUES*".column1
+ Hash Cond: (locp.a = "*VALUES*".column1)
+ -> Seq Scan on public.locp
+ Output: locp.b, locp.ctid, locp.a
+ -> Hash
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Values Scan on "*VALUES*"
+ Output: "*VALUES*".*, "*VALUES*".column1
+(24 rows)

In this test case, the foreign partition is updated before ri_FdwState
is overwritten by postgresBeginForeignInsert() that's invoked by the
tuple routing code, so it would work without any fix. So I modified
this test case as such.

Sorry for the long delay, again.

Best regards,
Etsuro Fujita