Re: Problem while updating a foreign table pointing to a partitioned table on foreign server

On Wed, Apr 18, 2018 at 9:43 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> Anyway I think we should warn or error out if one nondirect
> update touches two nor more tuples in the first place.
>
> =# UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 20 END) WHERE a = 1;
> ERROR: updated 2 rows for a tuple identity on the remote end

I liked that idea. But I think your patch wasn't quite right, esp.
when the returning had an SRF in it. Right now n_rows tracks the
number of rows returned if there is a returning list or the number of
rows updated/deleted on the foreign server. If there is an SRF, n_rows
can return multiple rows for a single updated or deleted row. So, I
changed your code to track number of rows updated/deleted and number
of rows returned separately. BTW, your patch didn't handle DELETE
case.

I have attached a set of patches
0001 adds a test case showing the issue.
0002 modified patch based on your idea of throwing an error
0003 WIP patch with a partial fix for the issue as discussed upthread

The expected output in 0001 is set to what it would when the problem
gets fixed. The expected output in 0002 is what it would be when we
commit only 0002 without a complete fix.
>
>
>> There are two ways to fix this
>> 1. Use WHERE CURRENT OF with cursors to update rows. This means that
>> we fetch only one row at a time and update it. This can slow down the
>> execution drastically.
>> 2. Along with ctid use tableoid as a qualifier i.e. WHERE clause of
>> UPDATE/DELETE statement has ctid = $1 AND tableoid = $2 as conditions.
>>
>> PFA patch along the lines of 2nd approach and along with the
>> testcases. The idea is to inject tableoid attribute to be fetched from
>> the foreign server similar to ctid and then add it to the DML
>> statement being constructed.
>>
>> It does fix the problem. But the patch as is interferes with the way
>> we handle tableoid currently. That can be seen from the regression
>> diffs that the patch causes. RIght now, every tableoid reference gets
>> converted into the tableoid of the foreign table (and not the tableoid
>> of the foreign table). Somehow we need to differentiate between the
>> tableoid injected for DML and tableoid references added by the user in
>> the original query and then use tableoid on the foreign server for the
>> first and local foreign table's oid for the second. Right now, I don't
>> see a simple way to do that.
>
> We cannot add no non-system (junk) columns not defined in foreign
> table columns.

Why? That's a probable way of fixing this problem.

> We could pass tableoid via a side channel but we
> get wrong value if the scan is not consists of only one foreign
> relation. I don't think adding remote_tableoid in HeapTupleData
> is acceptable.

I am thinking of adding remote_tableoid in HeapTupleData since not all
FDWs will have the concept of tableoid. But we need to somehow
distinguish the tableoid resjunk added for DMLs and tableoid requested
by the user.

> Explicity defining remote_tableoid column in
> foreign relation might work but it makes things combersome..
>

Not just cumbersome, it's not going to be always right, if the things
change on the foreign server e.g. OID of the table changes because it
got dropped and recreated on the foreign server or OID remained same
but the table got inherited and so on.

I think we should try getting 0001 and 0002 at least committed
independent of 0003.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company