Problem while updating a foreign table pointing to a partitioned table on foreign server
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
Attachments:
pg_ft_parttab_dml.patchtext/x-patch; charset=US-ASCII; name=pg_ft_parttab_dml.patchDownload+242-29
Hello.
At Mon, 16 Apr 2018 17:05:28 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in <CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com>
Hi,
Consider this scenariopostgres=# 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
Attachments:
fdw_error_out_multiple_update.patchtext/x-patch; charset=us-asciiDownload+7-0
On Wed, Apr 18, 2018 at 9:43 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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
Attachments:
0001-Tests-to-show-problem-when-foreign-table-points-to-a.patchtext/x-patch; charset=US-ASCII; name=0001-Tests-to-show-problem-when-foreign-table-points-to-a.patchDownload+174-1
0002-Error-out-if-one-iteration-of-non-direct-DML-affects.patchtext/x-patch; charset=US-ASCII; name=0002-Error-out-if-one-iteration-of-non-direct-DML-affects.patchDownload+53-20
0003-An-incomplete-fix-for-problem-in-non-direct-UPDATE-o.patchtext/x-patch; charset=US-ASCII; name=0003-An-incomplete-fix-for-problem-in-non-direct-UPDATE-o.patchDownload+128-30
At Wed, 18 Apr 2018 13:23:06 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in <CAFjFpReWZnJ_raxAroaxb3_uRVpxnrnh8w3BjKs0kgy0Ya2+kA@mail.gmail.com>
On Wed, Apr 18, 2018 at 9:43 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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 endI 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 upthreadThe 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
On Thu, Apr 19, 2018 at 11:38 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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 upthreadThe 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
Attachments:
0001-Tests-to-show-problem-when-foreign-table-points-to-a_v2.patchtext/x-patch; charset=US-ASCII; name=0001-Tests-to-show-problem-when-foreign-table-points-to-a_v2.patchDownload+174-1
0002-Error-out-if-one-iteration-of-non-direct-DML-affects_v2.patchtext/x-patch; charset=US-ASCII; name=0002-Error-out-if-one-iteration-of-non-direct-DML-affects_v2.patchDownload+57-20
0003-DML-on-foreign-table-pointing-to-an-inherited-or-a-p_v2.patchtext/x-patch; charset=US-ASCII; name=0003-DML-on-foreign-table-pointing-to-an-inherited-or-a-p_v2.patchDownload+294-130
On Mon, Apr 16, 2018 at 7:35 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
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.
I think that the place to start would be to change this code to use
something other than TableOidAttributeNumber:
+ var = makeVar(parsetree->resultRelation,
+ TableOidAttributeNumber,
+ OIDOID,
+ -1,
+ InvalidOid,
+ 0);Note that rewriteTargetListUD, which calls AddForeignUpdateTargets,
also contingently adds a "wholerow" attribute which ExecModifyTable()
is able to fish out later. It seems like it should be possible to add
a "remotetableoid" column that works similarly, although I'm not
exactly sure what would be involved.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, May 16, 2018 at 11:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Apr 16, 2018 at 7:35 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote: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.I think that the place to start would be to change this code to use
something other than TableOidAttributeNumber:+ var = makeVar(parsetree->resultRelation, + TableOidAttributeNumber, + OIDOID, + -1, + InvalidOid, + 0);Note that rewriteTargetListUD, which calls AddForeignUpdateTargets,
also contingently adds a "wholerow" attribute which ExecModifyTable()
is able to fish out later. It seems like it should be possible to add
a "remotetableoid" column that works similarly, although I'm not
exactly sure what would be involved.
As of today, all the attributes added by AddForeignUpdateTargets hook
of postgres_fdw are recognised by PostgreSQL. But remotetableoid is
not a recognised attributes. In order to use it, we either have to
define a new system attribute "remotetableoid" or add a user defined
attribute "remotetableoid" in every foreign table. The first one will
be very specific for postgres_fdw and other FDWs won't be able to use
it. The second would mean that SELECT * from foreign table reports
remotetableoid as well, which is awkward. Me and Horiguchi-san
discussed those ideas in this mail thread.
Anyway, my comment to which you have replied is obsolete now. I found
a solution to that problem, which I have implemented in 0003 in the
latest patch-set I have shared.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On Thu, May 17, 2018 at 2:10 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
The second would mean that SELECT * from foreign table reports
remotetableoid as well, which is awkward.
No it wouldn't. You'd just make the additional column resjunk, same
as we do for wholerow.
Anyway, my comment to which you have replied is obsolete now. I found
a solution to that problem, which I have implemented in 0003 in the
latest patch-set I have shared.
Yeah, but I'm not sure I like that solution very much. I don't think
abusing the tableoid to store a remote table OID is very nice.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
Yeah, but I'm not sure I like that solution very much. I don't think
abusing the tableoid to store a remote table OID is very nice.
I'd say it's totally unacceptable. Tableoid *has to* be something
that you can look up in the local pg_class instance, or serious
confusion will ensue.
regards, tom lane
On 2018-May-17, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
Yeah, but I'm not sure I like that solution very much. I don't think
abusing the tableoid to store a remote table OID is very nice.I'd say it's totally unacceptable. Tableoid *has to* be something
that you can look up in the local pg_class instance, or serious
confusion will ensue.
Can we just add a new junk attr, with its own fixed system column
number? I think that's what Robert was proposing.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Can we just add a new junk attr, with its own fixed system column
number? I think that's what Robert was proposing.
Junk attr yes, "fixed system column number" no. That's not how
junk attrs work. What it'd need is a convention for the name of
these resjunk attrs (corresponding to ctidN, wholerowN, etc).
We do already have tableoidN junk attrs, but by the same token
those should always be local OIDs, or we'll be in for deep
confusion. Maybe "remotetableoidN" ?
regards, tom lane
On Thu, May 17, 2018 at 11:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, May 17, 2018 at 2:10 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:The second would mean that SELECT * from foreign table reports
remotetableoid as well, which is awkward.No it wouldn't. You'd just make the additional column resjunk, same
as we do for wholerow.
You suggested
--
I think that the place to start would be to change this code to use
something other than TableOidAttributeNumber:+ var = makeVar(parsetree->resultRelation, + TableOidAttributeNumber, + OIDOID, + -1, + InvalidOid, + 0);
--
Wholerow has its own attribute number 0, ctid has its attribute number
-1. So we can easily create Vars for those and add resjunk entries in
the targetlist. But a "remotetableoid" doesn't have an attribute
number yet! Either it has to be a new system column, which I and
almost everybody here is opposing, or it has to be a user defined
attribute, with an entry in pg_attributes table. In the second case,
how would one make that column resjunk? I don't see any third
possibility.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
At Fri, 18 May 2018 10:19:30 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in <CAFjFpRe5KBBXzio-1iCzmH35kxYy90z6ewLU+VPtM0u=kH-ubw@mail.gmail.com>
ashutosh.bapat> On Thu, May 17, 2018 at 11:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, May 17, 2018 at 2:10 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:The second would mean that SELECT * from foreign table reports
remotetableoid as well, which is awkward.No it wouldn't. You'd just make the additional column resjunk, same
as we do for wholerow.You suggested
--I think that the place to start would be to change this code to use
something other than TableOidAttributeNumber:+ var = makeVar(parsetree->resultRelation, + TableOidAttributeNumber, + OIDOID, + -1, + InvalidOid, + 0);--
Wholerow has its own attribute number 0, ctid has its attribute number
-1. So we can easily create Vars for those and add resjunk entries in
the targetlist. But a "remotetableoid" doesn't have an attribute
number yet! Either it has to be a new system column, which I and
almost everybody here is opposing, or it has to be a user defined
attribute, with an entry in pg_attributes table. In the second case,
how would one make that column resjunk? I don't see any third
possibility.
I have reached to the same thought.
The point here is that it is a base relation, which is not
assumed to have additional columns not in its definition,
including nonsystem junk columns. I'm not sure but it seems not
that simple to give base relations an ability to have junk
columns.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, May 18, 2018 at 4:29 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
I have reached to the same thought.
The point here is that it is a base relation, which is not
assumed to have additional columns not in its definition,
including nonsystem junk columns. I'm not sure but it seems not
that simple to give base relations an ability to have junk
columns.
Do you know where that assumption is embedded specifically?
If you're correct, then the FDW API is and always has been broken by
design for any remote data source that uses a row identifier other
than CTID, unless every foreign table definition always includes the
row identifier as an explicit column. I might be wrong here, but I'm
pretty sure Tom wouldn't have committed this API in the first place
with such a glaring hole in the design.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
At Fri, 18 May 2018 15:31:07 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmoaBuzhhcA21sAm7wH+A-GH2d6GkKhVapkqhnHOW85dDXg@mail.gmail.com>
On Fri, May 18, 2018 at 4:29 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:I have reached to the same thought.
The point here is that it is a base relation, which is not
assumed to have additional columns not in its definition,
including nonsystem junk columns. I'm not sure but it seems not
that simple to give base relations an ability to have junk
columns.Do you know where that assumption is embedded specifically?
Taking the question literally, I see that add_vars_to_targetlist
accepts neither nonsystem (including whole row vars) junk columns
nor nonjunk columns that is not defined in the base relation. The
first line of the following code is that.
Assert(attno >= rel->min_attr && attno <= rel->max_attr);
attno -= rel->min_attr;
if (rel->attr_needed[attno] == NULL)
In the last line attr_needed is of an array of (max_attr -
min_attr) elements, which is allocated in get_relation_info. I
didn't go further so it might be easier than I'm thinking but
anyway core-side modification (seems to me) is required at any
rate.
If you're correct, then the FDW API is and always has been broken by
design for any remote data source that uses a row identifier other
than CTID, unless every foreign table definition always includes the
row identifier as an explicit column.
I actually see that. Oracle-FDW needs to compose row
identification by specifying "key" column option in relation
definition and the key columns are added as resjunk column. This
is the third (or, forth?) option of my comment upthread that was
said as "not only bothersome".
https://github.com/laurenz/oracle_fdw
| Column options (from PostgreSQL 9.2 on)
| key (optional, defaults to "false")
|
| If set to yes/on/true, the corresponding column on the foreign
| Oracle table is considered a primary key column. For UPDATE and
| DELETE to work, you must set this option on all columns that
| belong to the table's primary key.
I might be wrong here, but I'm
pretty sure Tom wouldn't have committed this API in the first place
with such a glaring hole in the design.
I see the API is still not broken in a sense, the ctid of
postgres_fdw is necessarily that of remote table. If we have a
reasonable mapping between remote tableoid:ctid and local ctid,
it works as expected. But such mapping seems to be rather
difficult to create since I don't find a generic way wihtout
needing auxiliary information, and at least there's no guarantee
that ctid has enough space for rows from multiple tables.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hello
I don't think this thread has reached a consensus on a design for a fix,
has it? Does anybody have a clear idea on a path forward? Is anybody
working on a patch?
Thanks
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thanks.
I don't think this thread has reached a consensus on a design for a fix
Right.
If my understanding about non-system junk columns in a base relation
and identifiers of a foreign tuples are correct, what is needed here
is giving base relations the ability to have such junk column.
I'm willing to work on that if I'm not on a wrong way here.
--
Kyotaro Horiguchi
Kyotaro HORIGUCHI <kyota.horiguchi@gmail.com> writes:
If my understanding about non-system junk columns in a base relation
and identifiers of a foreign tuples are correct, what is needed here
is giving base relations the ability to have such junk column.
The core of the problem, I think, is the question of exactly what
postgresAddForeignUpdateTargets should put into the resjunk expressions
it adds to an update/delete query's targetlist. Per discussion yesterday,
up to now it's always emitted Vars referencing the foreign relation,
which is problematic because with that approach the desired info has
to be exposed as either a regular or system column of that relation.
But there's nothing saying that the expression has to be a Var.
My thought about what we might do instead is that
postgresAddForeignUpdateTargets could reserve a PARAM_EXEC slot
and emit a Param node referencing that. Then at runtime, while
reading a potential target row from the remote, we fill that
param slot along with the regular scan tuple slot.
What you want for the first part of that is basically like
generate_new_param() in subselect.c. We don't expose that publicly
at the moment, but we could, or maybe better to invent another wrapper
around it like SS_make_initplan_output_param.
regards, tom lane
On Thu, May 31, 2018 at 7:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kyotaro HORIGUCHI <kyota.horiguchi@gmail.com> writes:
If my understanding about non-system junk columns in a base relation
and identifiers of a foreign tuples are correct, what is needed here
is giving base relations the ability to have such junk column.The core of the problem, I think, is the question of exactly what
postgresAddForeignUpdateTargets should put into the resjunk expressions
it adds to an update/delete query's targetlist. Per discussion yesterday,
up to now it's always emitted Vars referencing the foreign relation,
which is problematic because with that approach the desired info has
to be exposed as either a regular or system column of that relation.
But there's nothing saying that the expression has to be a Var.My thought about what we might do instead is that
postgresAddForeignUpdateTargets could reserve a PARAM_EXEC slot
and emit a Param node referencing that. Then at runtime, while
reading a potential target row from the remote, we fill that
param slot along with the regular scan tuple slot.What you want for the first part of that is basically like
generate_new_param() in subselect.c. We don't expose that publicly
at the moment, but we could, or maybe better to invent another wrapper
around it like SS_make_initplan_output_param.
This looks like a lot of change which might take some time and may not
be back-portable. In the mean time, can we see if 0001 and 0002
patches are good and apply them. Those patches intend to stop the
multiple rows on the foreign server being updated by throwing error
(and aborting the transaction on the foreign server) when that
happens. That will at least avoid silent corruption that happens today
and should be back-portable.
[1]: /messages/by-id/CAFjFpRfK69ptCTNChBBk+LYMXFzJ92SW6NmG4HLn_1y7xFk=kw@mail.gmail.com
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On Thu, May 31, 2018 at 11:34 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
On Thu, May 31, 2018 at 7:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
What you want for the first part of that is basically like
generate_new_param() in subselect.c. We don't expose that publicly
at the moment, but we could, or maybe better to invent another wrapper
around it like SS_make_initplan_output_param.This looks like a lot of change which might take some time and may not
I agree. It needs at least, in a short sight, an additional parameter
(PlannerInfo in a straightforwad way) for
postgresAddForeignUpdateTargets which is a change of FDW-API.
be back-portable. In the mean time, can we see if 0001 and 0002
patches are good and apply them. Those patches intend to stop the
multiple rows on the foreign server being updated by throwing error
(and aborting the transaction on the foreign server) when that
happens. That will at least avoid silent corruption that happens today
and should be back-portable.[1] /messages/by-id/CAFjFpRfK69ptCTNChBBk+LYMXFzJ92SW6NmG4HLn_1y7xFk=kw@mail.gmail.com
Having said that I think that storing oids of the remote table in
local tableoid syscolumn is a breakage of the existing contract about
the field. (I wish this is comprehensible.)
However I haven't found a way to "fix" this without such breakage of
API thus it seems to me inevitable to leave this problem as a
restriction, we still can avoid the problematic behavior by explicitly
declaring remote tableoid column (like the "key" column option of
oracle-fdw).
CREATE FOREIGN TABLE ft1 (rtoid oid, a int, blah, blah) SERVER sv
OPTIONS (remote_tableoid 'rtoid', table_name 'lt1');
However, of-course the proposed fix will work if we allow the
a-kind-of illegal usage of the local tableoid. And it seems to be a
way to cause a series of frequent changes on the same feature.
Thoughts?
