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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
Hi,
Consider this scenario

postgres=# CREATE TABLE plt (a int, b int) PARTITION BY LIST(a);
postgres=# CREATE TABLE plt_p1 PARTITION OF plt FOR VALUES IN (1);
postgres=# CREATE TABLE plt_p2 PARTITION OF plt FOR VALUES IN (2);
postgres=# INSERT INTO plt VALUES (1, 1), (2, 2);
postgres=# CREATE FOREIGN TABLE fplt (a int, b int) SERVER loopback
OPTIONS (table_name 'plt');
postgres=# SELECT tableoid::regclass, ctid, * FROM fplt;
 tableoid | ctid  | a | b
----------+-------+---+---
 fplt     | (0,1) | 1 | 1
 fplt     | (0,1) | 2 | 2
(2 rows)

-- Need to use random() so that following update doesn't turn into a
direct UPDATE.
postgres=# EXPLAIN (VERBOSE, COSTS OFF)
postgres-# UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE
20 END) WHERE a = 1;
                                         QUERY PLAN
--------------------------------------------------------------------------------------------
 Update on public.fplt
   Remote SQL: UPDATE public.plt SET b = $2 WHERE ctid = $1
   ->  Foreign Scan on public.fplt
         Output: a, CASE WHEN (random() <= '1'::double precision) THEN
10 ELSE 20 END, ctid
         Remote SQL: SELECT a, ctid FROM public.plt WHERE ((a = 1)) FOR UPDATE
(5 rows)

postgres=# UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE
20 END) WHERE a = 1;
postgres=# SELECT tableoid::regclass, ctid, * FROM fplt;
 tableoid | ctid  | a | b
----------+-------+---+----
 fplt     | (0,2) | 1 | 10
 fplt     | (0,2) | 2 | 10
(2 rows)

We expect only 1 row with a = 1 to be updated, but both the rows get
updated. This happens because both the rows has ctid = (0, 1) and
that's the only qualification used for UPDATE and DELETE. Thus when a
non-direct UPDATE is run on a foreign table which points to a
partitioned table or inheritance hierarchy on the foreign server, it
will update rows from all child table which have ctids same as the
qualifying rows. Same is the case with DELETE.

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.

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

pg_ft_parttab_dml.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Kyotaro HORIGUCHI-2
Hello.

At Mon, 16 Apr 2018 17:05:28 +0530, Ashutosh Bapat <[hidden email]> wrote in <CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=[hidden email]>

> Hi,
> Consider this scenario
>
> postgres=# CREATE TABLE plt (a int, b int) PARTITION BY LIST(a);
> postgres=# CREATE TABLE plt_p1 PARTITION OF plt FOR VALUES IN (1);
> postgres=# CREATE TABLE plt_p2 PARTITION OF plt FOR VALUES IN (2);
> postgres=# INSERT INTO plt VALUES (1, 1), (2, 2);
> postgres=# CREATE FOREIGN TABLE fplt (a int, b int) SERVER loopback
> OPTIONS (table_name 'plt');
> postgres=# SELECT tableoid::regclass, ctid, * FROM fplt;
>  tableoid | ctid  | a | b
> ----------+-------+---+---
>  fplt     | (0,1) | 1 | 1
>  fplt     | (0,1) | 2 | 2
> (2 rows)
>
> -- Need to use random() so that following update doesn't turn into a
> direct UPDATE.
> postgres=# EXPLAIN (VERBOSE, COSTS OFF)
> postgres-# UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE
> 20 END) WHERE a = 1;
>                                          QUERY PLAN
> --------------------------------------------------------------------------------------------
>  Update on public.fplt
>    Remote SQL: UPDATE public.plt SET b = $2 WHERE ctid = $1
>    ->  Foreign Scan on public.fplt
>          Output: a, CASE WHEN (random() <= '1'::double precision) THEN
> 10 ELSE 20 END, ctid
>          Remote SQL: SELECT a, ctid FROM public.plt WHERE ((a = 1)) FOR UPDATE
> (5 rows)
>
> postgres=# UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE
> 20 END) WHERE a = 1;
> postgres=# SELECT tableoid::regclass, ctid, * FROM fplt;
>  tableoid | ctid  | a | b
> ----------+-------+---+----
>  fplt     | (0,2) | 1 | 10
>  fplt     | (0,2) | 2 | 10
> (2 rows)
>
> We expect only 1 row with a = 1 to be updated, but both the rows get
> updated. This happens because both the rows has ctid = (0, 1) and
> that's the only qualification used for UPDATE and DELETE. Thus when a
> non-direct UPDATE is run on a foreign table which points to a
> partitioned table or inheritance hierarchy on the foreign server, it
> will update rows from all child table which have ctids same as the
> qualifying rows. Same is the case with DELETE.
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


> 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. 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. Explicity defining remote_tableoid column in
foreign relation might work but it makes things combersome..

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index e1c2639fde..7cd31cb6ab 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1895,6 +1895,13 @@ postgresExecForeignUpdate(EState *estate,
 
  MemoryContextReset(fmstate->temp_cxt);
 
+ /* ERROR if more than one row was updated on the remote end */
+ if (n_rows > 1)
+ ereport(ERROR,
+ (errcode (ERRCODE_FDW_ERROR), /* XXX */
+ errmsg ("updated %d rows for a tuple identity on the remote end",
+ n_rows)));
+
  /* Return NULL if nothing was updated on the remote end */
  return (n_rows > 0) ? slot : NULL;
 }
Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Wed, Apr 18, 2018 at 9:43 AM, Kyotaro HORIGUCHI
<[hidden email]> 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

0001-Tests-to-show-problem-when-foreign-table-points-to-a.patch (13K) Download Attachment
0002-Error-out-if-one-iteration-of-non-direct-DML-affects.patch (9K) Download Attachment
0003-An-incomplete-fix-for-problem-in-non-direct-UPDATE-o.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Kyotaro HORIGUCHI-2
At Wed, 18 Apr 2018 13:23:06 +0530, Ashutosh Bapat <[hidden email]> wrote in <[hidden email]>

> On Wed, Apr 18, 2018 at 9:43 AM, Kyotaro HORIGUCHI
> <[hidden email]> 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.

Yeah, sorry. It was to just show how the error looks
like. Attached 0002 works and looks fine except the following.

> /* 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.

> 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.

> > 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.

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.

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.

> > 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.

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.)

- ForeignScan for TableModify adds "remote_tableoid" instead of
  tableoid to receive remote tableoid and returns it as a part of
  a ordinary return tuple.

- ForeignUpdate/Delete sees the remote_tableoid instead of
  tuple's tableOid field.

Yes, it is dreadfully bad interface, especially it is not
guaranteed to be passed to modify side if users don't write a
query to do so. So, yes, the far bad than cumbersome.


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

Agreed on 0002. 0001 should be committed with 0003?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Previous Thread Next Thread