Re: bug in update tuple routing with foreign partitions

Amit-san,

Thanks for the comments!

(2019/04/18 14:08), Amit Langote wrote:
> On 2019/04/18 14:06, Amit Langote wrote:
>> On 2019/04/17 21:49, Etsuro Fujita wrote:

>>> I think the reason for that is: the row routed to remp is incorrectly
>>> fetched as a to-be-updated row when updating remp as a subplan targetrel.
>>
>> Yeah.  In the fully-local case, that is, where both the source and the
>> target partition of a row movement operation are local tables, heap AM
>> ensures that tuples that's moved into a given relation in the same command
>> (by way of row movement) are not returned as to-be-updated, because it
>> deems such tuples invisible.  The "same command" part being crucial for
>> that to work.
>>
>> In the case where the target of a row movement operation is a foreign
>> table partition, the INSERT used as part of row movement and subsequent
>> UPDATE of the same foreign table are distinct commands for the remote
>> server.  So, the rows inserted by the 1st command (as part of the row
>> movement) are deemed visible by the 2nd command (UPDATE) even if both are
>> operating within the same transaction.

Yeah, I think so too.

>> I guess there's no easy way for postgres_fdw to make the remote server
>> consider them as a single command.  IOW, no way to make the remote server
>> not touch those "moved-in" rows during the UPDATE part of the local query.

I agree.

>>> The right way to fix this would be to have some way in postgres_fdw in
>>> which we don't fetch such rows when updating such subplan targetrels.  I
>>> tried to figure out a (simple) way to do that, but I couldn't.
>>
>> Yeah, that seems a bit hard to ensure with our current infrastructure.

Yeah, I think we should leave that for future work.

>>> So what I'm thinking is to throw an error for cases like this.  (Though, I
>>> think we should keep to allow rows to be moved from local partitions to a
>>> foreign-table subplan targetrel that has been updated already.)
>>
>> Hmm, how would you distinguish (totally inside postgred_fdw I presume) the
>> two cases?

One thing I came up with to do that is this:

@@ -1917,6 +1937,23 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,

         initStringInfo(&sql);

+       /*
+        * If the foreign table is an UPDATE subplan resultrel that 
hasn't yet
+        * been updated, routing tuples to the table might yield incorrect
+        * results, because if routing tuples, routed tuples will be 
mistakenly
+        * read from the table and updated twice when updating the table 
--- it
+        * would be nice if we could handle this case; but for now, 
throw an error
+        * for safety.
+        */
+       if (plan && plan->operation == CMD_UPDATE &&
+               (resultRelInfo->ri_usesFdwDirectModify ||
+                resultRelInfo->ri_FdwState) &&
+               resultRelInfo > mtstate->resultRelInfo + 
mtstate->mt_whichplan)
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot route tuples into 
foreign table to be updated \"%s\"",
+ 
RelationGetRelationName(rel))));

> Forgot to say that since this is a separate issue from the original bug
> report, maybe we can first finish fixing the latter.  What to do you think?

Yeah, I think we can do that, but my favorite would be to fix the two 
issues in a single shot, because they seem to me rather close to each 
other.  So I updated a new version in a single patch, which I'm attaching.

Notes:

* I kept all the changes in the previous patch, because otherwise 
postgres_fdw would fail to release resources for a foreign-insert 
operation created by postgresBeginForeignInsert() for a tuple-routable 
foreign table (ie, a foreign-table subplan resultrel that has been 
updated already) during postgresEndForeignInsert().

* I revised a comment according to your previous comment, though I 
changed a state name: s/sub_fmstate/aux_fmstate/

* DirectModify also has the latter issue.  Here is an example:

postgres=# create table p (a int, b text) partition by list (a);
postgres=# create table p1 partition of p for values in (1);
postgres=# create table p2base (a int check (a = 2 or a = 3), b text);
postgres=# create foreign table p2 partition of p for values in (2, 3) 
server loopback options (table_name 'p2base');

postgres=# insert into p values (1, 'foo');
INSERT 0 1
postgres=# explain verbose update p set a = a + 1;
                                  QUERY PLAN
-----------------------------------------------------------------------------
  Update on public.p  (cost=0.00..176.21 rows=2511 width=42)
    Update on public.p1
    Foreign Update on public.p2
    ->  Seq Scan on public.p1  (cost=0.00..25.88 rows=1270 width=42)
          Output: (p1.a + 1), p1.b, p1.ctid
    ->  Foreign Update on public.p2  (cost=100.00..150.33 rows=1241 
width=42)
          Remote SQL: UPDATE public.p2base SET a = (a + 1)
(7 rows)

postgres=# update p set a = a + 1;
UPDATE 2
postgres=# select * from p;
  a |  b
---+-----
  3 | foo
(1 row)

As shown in the above bit added to postgresBeginForeignInsert(), I 
modified the patch so that we throw an error for cases like this even 
when using a direct modification plan, and I also added regression test 
cases for that.

Best regards,
Etsuro Fujita