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

On Thu, Apr 19, 2018 at 11:38 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
>> /* No rows should be returned if no rows were updated. */
>> Assert(n_rows_returned == 0 || n_rows_updated > 0);
>
> The assertion is correct but I think that we shouldn't crash
> server by any kind of protocol error. I think ERROR is suitable.
>

That's a good idea. Done.

>> 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.
>
> In other words, tuples returned from ForeignNext
> (postgresIterateForeignScan) on a foreign (base) relation cannot
> contain a non-system column which is not a part of the relation,
> since its tuple descriptor doesn't know of and does error out it.
> The current 0003 stores remote tableoid in tuples' existing
> tableOid field (not a column data), which is not proper since
> remote tableoid is bogus for the local server. I might missing
> something here, though. If we can somehow attach an blob at the
> end of t_data and it is finally passed to
> ExecForeignUpdate/Delete, the problem would be resolved.

Attached 0003 uses HeapTupleData::t_tableoid to store remote tableoid
and local tableoid. Remote tableoid is stored there for a scan
underlying DELETE/UPDATE. Local tableoid is stored otherwise. We use a
flag fetch_foreign_tableoid, stand alone and in deparse_expr_cxt to
differentiate between these two usages.

>
> I don't think it is acceptable but (hopefully) almost solves this
> problem if we allow that. User always sees the conventional
> tableOid and all ExecForeignUpdate/Delete have to do is to use
> remote_tableoid as a part of remote tuple identifier. Required to
> consider how to propagate the remote_tableoid through joins or
> other intermediate executor nodes, though. It is partly similar
> to the way deciding join push down.

0003 does that. Fortunately we already have testing UPDATE/DELETE with joins.

>
> Another point is that, even though HeapTupleData is the only
> expected coveyer of the tuple identification, assuming tableoid +
> ctid is not adequite since FDW interface is not exlusive for
> postgres_fdw. The existig ctid field is not added for the purpose
> and just happened to (seem to) work as tuple identifier for
> postgres_fdw but I think tableoid is not.

I am not able to understand. postgresAddForeignUpdateTargets does that
specifically for postgres_fdw. I am using the same function to add
junk column for tableoid similar to ctid.

>
> The same can be said on ctid. Maybe my description was
> unclear. Specifically, I intended to say something like:
>
> - If we want to update/delete remote partitioned/inhtance tables
> without direct modify, the foreign relation must have a columns
> defined as "tableoid as remote_tableoid" or something. (We
> could change the column name by a fdw option.)

Ok. I think, I misunderstood your proposal. IIUC, this way, SELECT *
FROM foreign_table is going to report remote_tableoid, which won't be
welcome by users.

Let me know what you think of the attached patches.

>
>
>> I think we should try getting 0001 and 0002 at least committed
>> independent of 0003.
>
> Agreed on 0002. 0001 should be committed with 0003?

0001 adds testcases which show the problem, so we have to commit it
with 0003 or 0002.

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