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

From: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Problem while updating a foreign table pointing to a partitioned table on foreign server
Date: 2018-04-27 10:25:25
Views: Raw Message | Whole Thread | Download mbox
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Tests-to-show-problem-when-foreign-table-points-to-a_v2.patch text/x-patch 9.9 KB
0002-Error-out-if-one-iteration-of-non-direct-DML-affects_v2.patch text/x-patch 6.8 KB
0003-DML-on-foreign-table-pointing-to-an-inherited-or-a-p_v2.patch text/x-patch 47.6 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Aleksandr Parfenov 2018-04-27 11:03:07 Range phrase operator in tsquery
Previous Message insaf.k 2018-04-27 09:59:24 Issues while building PG in MS Windows, using MSYS2 and MinGW-w64