join pushdown and issue with foreign update
Hi.
There's issue with join pushdown after
commit 86dc90056dfdbd9d1b891718d2e5614e3e432f35
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed Mar 31 11:52:34 2021 -0400
Rework planning and execution of UPDATE and DELETE
To make sure that join pushdown path selected, one can patch
contrib/postgres_fdw/postgres_fdw.c in the following way:
diff --git a/contrib/postgres_fdw/postgres_fdw.c
b/contrib/postgres_fdw/postgres_fdw.c
index c48a421e88b..c2bf6833050 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5959,6 +5959,8 @@ postgresGetForeignJoinPaths(PlannerInfo *root,
/* Estimate costs for bare join relation */
estimate_path_cost_size(root, joinrel, NIL, NIL, NULL,
&rows, &width,
&startup_cost, &total_cost);
+
+ startup_cost = total_cost = 0;
/* Now update this information in the joinrel */
joinrel->rows = rows;
joinrel->reltarget->width = width;Now, this simple test shows the issue:
create extension postgres_fdw;
DO $d$
BEGIN
EXECUTE $$CREATE SERVER loopback FOREIGN DATA WRAPPER
postgres_fdw
OPTIONS (dbname '$$||current_database()||$$',
port '$$||current_setting('port')||$$')$$;
END;
$d$;
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
CREATE TABLE base_tbl (a int, b int);
CREATE FOREIGN TABLE remote_tbl (a int, b int)
SERVER loopback OPTIONS (table_name 'base_tbl');
insert into remote_tbl select generate_series(1,100),
generate_series(1,100);
explain verbose update remote_tbl d set a= case when current_timestamp>
'2012-02-02'::timestamp then 5 else 6 end FROM remote_tbl AS t (a, b)
WHERE d.a = (t.a);
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Update on public.remote_tbl d (cost=0.00..42.35 rows=0 width=0)
Remote SQL: UPDATE public.base_tbl SET a = $2 WHERE ctid = $1
-> Foreign Scan (cost=0.00..42.35 rows=8470 width=74)
Output: CASE WHEN (CURRENT_TIMESTAMP > '2012-02-02
00:00:00'::timestamp without time zone) THEN 5 ELSE 6 END, d.ctid, d.*,
t.*
Relations: (public.remote_tbl d) INNER JOIN (public.remote_tbl
t)
Remote SQL: SELECT r1.ctid, CASE WHEN (r1.*)::text IS NOT NULL
THEN ROW(r1.a, r1.b) END, CASE WHEN (r2.*)::text IS NOT NULL THEN
ROW(r2.a, r2.b) END FROM (public.base_tbl r1 INNER JOIN public.base_tbl
r2 ON (((r1.a = r2.a)))) FOR UPDATE OF r1
-> Merge Join (cost=433.03..566.29 rows=8470 width=70)
Output: d.ctid, d.*, t.*
Merge Cond: (d.a = t.a)
-> Sort (cost=211.00..214.10 rows=1241 width=42)
Output: d.ctid, d.*, d.a
Sort Key: d.a
-> Foreign Scan on public.remote_tbl d
(cost=100.00..147.23 rows=1241 width=42)
Output: d.ctid, d.*, d.a
Remote SQL: SELECT a, b, ctid FROM
public.base_tbl FOR UPDATE
-> Sort (cost=222.03..225.44 rows=1365 width=36)
Output: t.*, t.a
Sort Key: t.a
-> Foreign Scan on public.remote_tbl t
(cost=100.00..150.95 rows=1365 width=36)
Output: t.*, t.a
Remote SQL: SELECT a, b FROM public.base_tbl
update remote_tbl d set a= case when current_timestamp>
'2012-02-02'::timestamp then 5 else 6 end FROM remote_tbl AS t (a, b)
WHERE d.a = (t.a);
You'll get
ERROR: input of anonymous composite types is not implemented
CONTEXT: whole-row reference to foreign table "remote_tbl"
make_tuple_from_result_row() (called by fetch_more_data()), will try to
call InputFunctionCall() for ROW(r1.a, r1.b) and will get error in
record_in().
Here ROW(r2.a, r2.b) would have attribute type id, corresponding to
remote_tbl, but ROW(r1.a, r1.b) would have atttypid 2249 (RECORD).
Before 86dc90056dfdbd9d1b891718d2e5614e3e432f35 the plan would be
different and looked like
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Update on public.remote_tbl d (cost=0.00..73.54 rows=14708 width=46)
Remote SQL: UPDATE public.base_tbl SET a = $2 WHERE ctid = $1
-> Foreign Scan (cost=0.00..73.54 rows=14708 width=46)
Output: CASE WHEN (CURRENT_TIMESTAMP > '2012-02-02
00:00:00'::timestamp without time zone) THEN d.a ELSE 6 END, d.b,
d.ctid, t.*
Relations: (public.remote_tbl d) INNER JOIN (public.remote_tbl
t)
Remote SQL: SELECT r1.a, r1.b, r1.ctid, CASE WHEN (r2.*)::text
IS NOT NULL THEN ROW(r2.a, r2.b) END FROM (public.base_tbl r1 INNER JOIN
public.base_tbl r2 ON (((r1.a = r2.a)))) FOR UPDATE OF r1
-> Merge Join (cost=516.00..747.39 rows=14708 width=46)
Output: d.a, d.b, d.ctid, t.*
Merge Cond: (d.a = t.a)
-> Sort (cost=293.97..299.35 rows=2155 width=14)
Output: d.a, d.b, d.ctid
Sort Key: d.a
-> Foreign Scan on public.remote_tbl d
(cost=100.00..174.65 rows=2155 width=14)
Output: d.a, d.b, d.ctid
Remote SQL: SELECT a, b, ctid FROM
public.base_tbl FOR UPDATE
-> Sort (cost=222.03..225.44 rows=1365 width=36)
Output: t.*, t.a
Sort Key: t.a
-> Foreign Scan on public.remote_tbl t
(cost=100.00..150.95 rows=1365 width=36)
Output: t.*, t.a
Remote SQL: SELECT a, b FROM public.base_tbl
Here ROW(r2.a, r2.b) would have attribute type id, corresponding to
remote_tbl.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
Alexander Pyhalov писал 2021-05-31 15:39:
Hi.
There's issue with join pushdown after
commit 86dc90056dfdbd9d1b891718d2e5614e3e432f35
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed Mar 31 11:52:34 2021 -0400
...
You'll get
ERROR: input of anonymous composite types is not implemented
CONTEXT: whole-row reference to foreign table "remote_tbl"make_tuple_from_result_row() (called by fetch_more_data()), will try
to call InputFunctionCall() for ROW(r1.a, r1.b) and will get error in
record_in().Here ROW(r2.a, r2.b) would have attribute type id, corresponding to
remote_tbl, but ROW(r1.a, r1.b) would have atttypid 2249 (RECORD).
The issue seems to be that add_row_identity_columns() adds RECORD var to
the query.
Adding var with table's relation type fixes this issue, but breaks
update of
partitioned tables, as we add "wholerow" with type of one child relation
and then
try to use it with another child (of different table type).
--
Best regards,
Alexander Pyhalov,
Postgres Professional
Hi,
On Tue, Jun 1, 2021 at 1:04 AM Alexander Pyhalov
<a.pyhalov@postgrespro.ru> wrote:
Alexander Pyhalov писал 2021-05-31 15:39:
Hi.
There's issue with join pushdown after
commit 86dc90056dfdbd9d1b891718d2e5614e3e432f35
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed Mar 31 11:52:34 2021 -0400...
You'll get
ERROR: input of anonymous composite types is not implemented
CONTEXT: whole-row reference to foreign table "remote_tbl"
Interesting, thanks for reporting this. This sounds like a regression
on 86dc90056's part.
make_tuple_from_result_row() (called by fetch_more_data()), will try
to call InputFunctionCall() for ROW(r1.a, r1.b) and will get error in
record_in().Here ROW(r2.a, r2.b) would have attribute type id, corresponding to
remote_tbl, but ROW(r1.a, r1.b) would have atttypid 2249 (RECORD).The issue seems to be that add_row_identity_columns() adds RECORD var to
the query.
Adding var with table's relation type fixes this issue, but breaks
update of
partitioned tables, as we add "wholerow" with type of one child relation
and then
try to use it with another child (of different table type).
Perhaps, we can get away with adding the wholerow Var with the target
relation's reltype when the target foreign table is not a "child"
relation, but the root target relation itself. Maybe like the
attached?
--
Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
wholerow-rowid-var-vartype-fix.patchapplication/octet-stream; name=wholerow-rowid-var-vartype-fix.patchDownload+35-10
Amit Langote писал 2021-06-01 15:47:
Perhaps, we can get away with adding the wholerow Var with the target
relation's reltype when the target foreign table is not a "child"
relation, but the root target relation itself. Maybe like the
attached?
Hi.
I think the patch fixes this issue, but it still preserves chances to
get RECORD in fetch_more_data()
(at least with combination with asymmetric partition-wise join).
What about the following patch?
--
Best regards,
Alexander Pyhalov,
Postgres Professional
Attachments:
0001-Fix-foreign-update.patchtext/x-diff; name=0001-Fix-foreign-update.patchDownload+78-37
Alexander Pyhalov <a.pyhalov@postgrespro.ru> writes:
What about the following patch?
ISTM that using a specific rowtype rather than RECORD would be
quite disastrous from the standpoint of bloating the number of
distinct resjunk columns we need for a partition tree with a
lot of children. Maybe we'll have to go that way, but it seems
like an absolute last resort.
I think a preferable fix involves making sure that the correct
record-type typmod is propagated to record_in in this context.
Alternatively, maybe we could insert the foreign table's rowtype
during execution of the input operation, without touching the
plan as such.
Could we start by creating a test case that doesn't involve
uncommittable hacks to the source code?
regards, tom lane
Tom Lane писал 2021-06-01 21:19:
Alexander Pyhalov <a.pyhalov@postgrespro.ru> writes:
What about the following patch?
ISTM that using a specific rowtype rather than RECORD would be
quite disastrous from the standpoint of bloating the number of
distinct resjunk columns we need for a partition tree with a
lot of children. Maybe we'll have to go that way, but it seems
like an absolute last resort.
Why do you think they are distinct?
In suggested patch all of them will have type of the common ancestor
(root of the partition tree).
I think a preferable fix involves making sure that the correct
record-type typmod is propagated to record_in in this context.
Alternatively, maybe we could insert the foreign table's rowtype
during execution of the input operation, without touching the
plan as such.Could we start by creating a test case that doesn't involve
uncommittable hacks to the source code?
Yes, it seems the following works fine to reproduce the issue.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
Attachments:
example.difftext/x-diff; name=example.diffDownload+13-0
Alexander Pyhalov <a.pyhalov@postgrespro.ru> writes:
Tom Lane писал 2021-06-01 21:19:
ISTM that using a specific rowtype rather than RECORD would be
quite disastrous from the standpoint of bloating the number of
distinct resjunk columns we need for a partition tree with a
lot of children. Maybe we'll have to go that way, but it seems
like an absolute last resort.
Why do you think they are distinct?
In suggested patch all of them will have type of the common ancestor
(root of the partition tree).
Seems moderately unlikely that that will work in cases where the
partition children have rowtypes different from the ancestor
(different column order etc). It'll also cause the problem we
originally sought to avoid for selects across traditional inheritance
trees, where there isn't a common partition ancestor.
regards, tom lane
I wrote:
I think a preferable fix involves making sure that the correct
record-type typmod is propagated to record_in in this context.
Alternatively, maybe we could insert the foreign table's rowtype
during execution of the input operation, without touching the
plan as such.
Here's a draft-quality patch based on that idea. It resolves
the offered test case, but I haven't beat on it beyond that.
regards, tom lane
Attachments:
use-proper-rowtype-for-postgres_fdw-row-ids.patchtext/x-diff; charset=us-ascii; name=use-proper-rowtype-for-postgres_fdw-row-ids.patchDownload+117-2
On 2/6/21 02:32, Tom Lane wrote:
I wrote:
I think a preferable fix involves making sure that the correct
record-type typmod is propagated to record_in in this context.
Alternatively, maybe we could insert the foreign table's rowtype
during execution of the input operation, without touching the
plan as such.Here's a draft-quality patch based on that idea. It resolves
the offered test case, but I haven't beat on it beyond that.regards, tom lane
I played with your patch and couldn't find any errors. But what if ROW
operation were allowed to be pushed to a foreign server?
Potentially, I can imagine pushed-down JOIN with arbitrary ROW function
in its target list.
Amit's approach looks more safe for me.
--
regards,
Andrey Lepikhov
Postgres Professional
On Wed, Jun 2, 2021 at 6:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
I think a preferable fix involves making sure that the correct
record-type typmod is propagated to record_in in this context.
Alternatively, maybe we could insert the foreign table's rowtype
during execution of the input operation, without touching the
plan as such.Here's a draft-quality patch based on that idea.
This looks good to me. Yeah, I agree that reversing our decision to
mark row-id wholerow Vars in as RECORD rather than a specific reltype
will have to wait until we hear more complaints than just this one,
which seems fixable with a patch like this.
It resolves
the offered test case, but I haven't beat on it beyond that.
Given that we don't (no longer) support pushing down the join of child
target relations with other relations, I don't think we have other
cases that are affected at this point. I have a feeling that your
patch will have fixed things enough that the same problem will not
occur when we have join pushdown under UPDATE occurring in more cases.
--
Amit Langote
EDB: http://www.enterprisedb.com
Tom Lane писал 2021-06-02 00:32:
I wrote:
I think a preferable fix involves making sure that the correct
record-type typmod is propagated to record_in in this context.
Alternatively, maybe we could insert the foreign table's rowtype
during execution of the input operation, without touching the
plan as such.Here's a draft-quality patch based on that idea. It resolves
the offered test case, but I haven't beat on it beyond that.regards, tom lane
Hi.
The patch seems to work fine for mentioned case.
For now I'm working on function pushdown. When record-returning function
(like unnest())
is pushed down, on this stage we've already lost any type information,
so get the issue again.
So far I'm not sure how to fix the issue, perhaps just avoid pushing
foreign join if we have
record, corresponding to function RTE var in joinrel->reltarget?
--
Best regards,
Alexander Pyhalov,
Postgres Professional
On Wed, Jun 2, 2021 at 4:39 PM Andrey Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
On 2/6/21 02:32, Tom Lane wrote:
I wrote:
I think a preferable fix involves making sure that the correct
record-type typmod is propagated to record_in in this context.
Alternatively, maybe we could insert the foreign table's rowtype
during execution of the input operation, without touching the
plan as such.Here's a draft-quality patch based on that idea. It resolves
the offered test case, but I haven't beat on it beyond that.I played with your patch and couldn't find any errors. But what if ROW
operation were allowed to be pushed to a foreign server?Potentially, I can imagine pushed-down JOIN with arbitrary ROW function
in its target list.
Are you saying that a pushed down ROW() expression may not correspond
with the Var chosen by the following code?
+ /*
+ * If we can't identify the referenced table, do nothing. This'll
+ * likely lead to failure later, but perhaps we can muddle through.
+ */
+ var = (Var *) list_nth_node(TargetEntry, fsplan->fdw_scan_tlist,
+ i)->expr;
+ if (!IsA(var, Var))
+ continue;
+ rte = list_nth(estate->es_range_table, var->varno - 1);
+ if (rte->rtekind != RTE_RELATION)
+ continue;
+ reltype = get_rel_type_id(rte->relid);
+ if (!OidIsValid(reltype))
+ continue;
+ att->atttypid = reltype;That may be a valid concern. I wonder if it would make sense to also
check varattno == 0 here somewhere for good measure.
--
Amit Langote
EDB: http://www.enterprisedb.com
Amit Langote <amitlangote09@gmail.com> writes:
On Wed, Jun 2, 2021 at 4:39 PM Andrey Lepikhov
<a.lepikhov@postgrespro.ru> wrote:I played with your patch and couldn't find any errors. But what if ROW
operation were allowed to be pushed to a foreign server?
Potentially, I can imagine pushed-down JOIN with arbitrary ROW function
in its target list.
I thought about this for awhile and I don't think it's a real concern.
There's nothing stopping us from pushing an expression of the form
"func(row(...))" or "row(...) op row(...)", because we're not asking
to retrieve the value of the ROW() expression. Whether the remote
server can handle that is strictly its concern. (Probably, it's
going to do something involving a locally-assigned typmod to keep
track of the rowtype, but it's not our problem.) Where things get
sticky is if we try to *retrieve the value* of a ROW() expression.
And except in this specific context, I don't see why we'd do that.
There's no advantage compared to retrieving the component Vars
or expressions.
... I wonder if it would make sense to also
check varattno == 0 here somewhere for good measure.
Yeah, I considered doing that but left it off in this version.
It's not clear to me how there could be a table column of type RECORD,
so it seemed unnecessary. On the other hand, it's also cheap
insurance, so I'll put it back.
regards, tom lane