Обсуждение: BUG #15613: Bug in PG Planner for Foreign Data Wrappers
The following bug has been logged on the website:
Bug reference: 15613
Logged by: Srinivasan S A
Email address: srinivasan.sa@zohocorp.com
PostgreSQL version: 9.6.3
Operating system: Linux and macOS
Description:
Foreign scan of a table (part of join), has a column of another table
involved in its qual. This column is of type T_Var (instead of T_Param)
which could lead to errors or wrong results. This issue is reproducible in
all foreign data wrappers, including file_fdw. The same query runs fine for
PG Tables.
Steps to reproduce in file_fdw:
1. Create 3 tables and analyze it:
CREATE FOREIGN TABLE table1 (
t1_col1 bigint,
t1_col2 bigint)
SERVER pglog options (
filename 'table1.csv',
format 'csv',
DELIMITER '|'
);
CREATE FOREIGN TABLE table2 (
t2_col1 bigint)
SERVER pglog options (
filename 'table2.csv',
format 'csv',
DELIMITER '|'
);
CREATE FOREIGN TABLE table3 (
t3_col1 bigint)
SERVER pglog options (
filename 'table3.csv',
format 'csv',
DELIMITER '|'
);
2. Run the query:
SELECT
subq_1.c1 AS c1
FROM
table1 AS ref_0,
LATERAL (
SELECT
ref_0.t1_col1 AS c1,
subq_0.c1 AS c2,
subq_0.c2 AS c3
FROM (
SELECT
ref_1.t2_col1 AS c1,
ref_0.t1_col2 AS c2
FROM
table2 AS ref_1
WHERE
TRUE) AS subq_0
RIGHT JOIN table3 AS ref_3 ON (subq_0.c1 = ref_3.t3_col1)
WHERE
pg_catalog.inet_client_port() < subq_0.c1) AS subq_1
WHERE
subq_1.c3 IS NOT NULL
LIMIT 108;
This throws an error:
ERROR: attribute number 2 exceeds number of columns 1
Explain gives
QUERY PLAN
----------------------------------------------------------------------------------
Hash Join (cost=10000000001.11..10000000003.34 rows=1 width=8)
Hash Cond: (ref_1.t2_col1 = ref_3.t3_col1)
-> Nested Loop (cost=10000000000.00..10000000002.22 rows=1 width=16)
-> Foreign Scan on table1 ref_0 (cost=0.00..1.10 rows=1
width=16)
Foreign File:
/Volumes/Official/workspace/Datasets/table1.csv
Foreign File Size: 4
-> Foreign Scan on table2 ref_1 (cost=0.00..1.10 rows=1
width=8)
Filter: ((t1_col2 IS NOT NULL) AND (inet_client_port() <
t2_col1))
Foreign File:
/Volumes/Official/workspace/Datasets/table2.csv
Foreign File Size: 2
-> Hash (cost=1.10..1.10 rows=1 width=8)
-> Foreign Scan on table3 ref_3 (cost=0.00..1.10 rows=1
width=8)
Foreign File:
/Volumes/Official/workspace/Datasets/table2.csv
Foreign File Size: 2
Here, table2's qual involves table1's column t1_col2. In the plan, this
column should be of type T_Param instead of T_Var. Query plan below:
DETAIL: {PLANNEDSTMT
:commandType 1
:queryId 2207256765
:hasReturning false
:hasModifyingCTE false
:canSetTag true
:transientPlan false
:dependsOnRole false
:parallelModeNeeded false
:planTree
{HASHJOIN
:startup_cost 10000000001.11
:total_cost 10000000003.34
:plan_rows 1
:plan_width 8
:parallel_aware false
:plan_node_id 0
:targetlist (
{TARGETENTRY
:expr
{VAR
:varno 65001
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 1
:varoattno 1
:location 94
}
:resno 1
:resname c1
:ressortgroupref 0
:resorigtbl 4626056
:resorigcol 1
:resjunk false
}
)
:qual <>
:lefttree
{NESTLOOP
:startup_cost 10000000000.00
:total_cost 10000000002.22
:plan_rows 1
:plan_width 16
:parallel_aware false
:plan_node_id 1
:targetlist (
{TARGETENTRY
:expr
{VAR
:varno 65001
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 1
:varoattno 1
:location 94
}
:resno 1
:resname <>
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
{TARGETENTRY
:expr
{VAR
:varno 65000
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 6
:varoattno 1
:location 222
}
:resno 2
:resname <>
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
)
:qual <>
:lefttree
{FOREIGNSCAN
:startup_cost 0.00
:total_cost 1.10
:plan_rows 1
:plan_width 16
:parallel_aware false
:plan_node_id 2
:targetlist (
{TARGETENTRY
:expr
{VAR
:varno 1
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 1
:varoattno 1
:location -1
}
:resno 1
:resname <>
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
{TARGETENTRY
:expr
{VAR
:varno 1
:varattno 2
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 1
:varoattno 2
:location -1
}
:resno 2
:resname <>
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
)
:qual <>
:lefttree <>
:righttree <>
:initPlan <>
:extParam (b)
:allParam (b)
:scanrelid 1
:operation 1
:fs_server 4625991
:fdw_exprs <>
:fdw_private <>
:fdw_scan_tlist <>
:fdw_recheck_quals <>
:fs_relids (b 1)
:fsSystemCol false
}
:righttree
{FOREIGNSCAN
:startup_cost 0.00
:total_cost 1.10
:plan_rows 1
:plan_width 8
:parallel_aware false
:plan_node_id 3
:targetlist (
{TARGETENTRY
:expr
{VAR
:varno 6
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 6
:varoattno 1
:location -1
}
:resno 1
:resname <>
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
)
:qual (
{NULLTEST
:arg
{VAR
:varno 1
:varattno 2
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 1
:varoattno 2
:location 259
}
:nulltesttype 1
:argisrow false
:location 535
}
{OPEXPR
:opno 37
:opfuncid 854
:opresulttype 16
:opretset false
:opcollid 0
:inputcollid 0
:args (
{FUNCEXPR
:funcid 2197
:funcresulttype 23
:funcretset false
:funcvariadic false
:funcformat 0
:funccollid 0
:inputcollid 0
:args <>
:location 462
}
{VAR
:varno 6
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 6
:varoattno 1
:location 222
}
)
:location 492
}
)
:lefttree <>
:righttree <>
:initPlan <>
:extParam (b)
:allParam (b)
:scanrelid 6
:operation 1
:fs_server 4625991
:fdw_exprs <>
:fdw_private <>
:fdw_scan_tlist <>
:fdw_recheck_quals <>
:fs_relids (b 6)
:fsSystemCol false
}
:initPlan <>
:extParam (b)
:allParam (b)
:jointype 0
:joinqual <>
:nestParams <>
}
:righttree
{HASH
:startup_cost 1.10
:total_cost 1.10
:plan_rows 1
:plan_width 8
:parallel_aware false
:plan_node_id 4
:targetlist (
{TARGETENTRY
:expr
{VAR
:varno 65001
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 4
:varoattno 1
:location -1
}
:resno 1
:resname <>
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
)
:qual <>
:lefttree
{FOREIGNSCAN
:startup_cost 0.00
:total_cost 1.10
:plan_rows 1
:plan_width 8
:parallel_aware false
:plan_node_id 5
:targetlist (
{TARGETENTRY
:expr
{VAR
:varno 4
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 4
:varoattno 1
:location 429
}
:resno 1
:resname <>
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
)
:qual <>
:lefttree <>
:righttree <>
:initPlan <>
:extParam (b)
:allParam (b)
:scanrelid 4
:operation 1
:fs_server 4625991
:fdw_exprs <>
:fdw_private <>
:fdw_scan_tlist <>
:fdw_recheck_quals <>
:fs_relids (b 4)
:fsSystemCol false
}
:righttree <>
:initPlan <>
:extParam (b)
:allParam (b)
:skewTable 4626059
:skewColumn 1
:skewInherit false
:skewColType 20
:skewColTypmod -1
}
:initPlan <>
:extParam (b)
:allParam (b)
:jointype 0
:joinqual <>
:hashclauses (
{OPEXPR
:opno 410
:opfuncid 467
:opresulttype 16
:opretset false
:opcollid 0
:inputcollid 0
:args (
{VAR
:varno 65001
:varattno 2
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 6
:varoattno 1
:location 222
}
{VAR
:varno 65000
:varattno 1
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 4
:varoattno 1
:location 429
}
)
:location -1
}
)
}
:rtable (
{RTE
:alias
{ALIAS
:aliasname ref_0
:colnames <>
}
:eref
{ALIAS
:aliasname ref_0
:colnames ("t1_col1" "t1_col2")
}
:rtekind 0
:relid 4626056
:relkind f
:tablesample <>
:lateral false
:inh false
:inFromCl true
:requiredPerms 2
:checkAsUser 0
:selectedCols (b 9 10)
:insertedCols (b)
:updatedCols (b)
:securityQuals <>
}
{RTE
:alias
{ALIAS
:aliasname subq_1
:colnames <>
}
:eref
{ALIAS
:aliasname subq_1
:colnames ("c1" "c2" "c3")
}
:rtekind 1
:subquery <>
:security_barrier false
:lateral true
:inh false
:inFromCl true
:requiredPerms 0
:checkAsUser 0
:selectedCols (b)
:insertedCols (b)
:updatedCols (b)
:securityQuals <>
}
{RTE
:alias
{ALIAS
:aliasname subq_0
:colnames <>
}
:eref
{ALIAS
:aliasname subq_0
:colnames ("c1" "c2")
}
:rtekind 1
:subquery <>
:security_barrier false
:lateral true
:inh false
:inFromCl true
:requiredPerms 0
:checkAsUser 0
:selectedCols (b)
:insertedCols (b)
:updatedCols (b)
:securityQuals <>
}
{RTE
:alias
{ALIAS
:aliasname ref_3
:colnames <>
}
:eref
{ALIAS
:aliasname ref_3
:colnames ("t3_col1")
}
:rtekind 0
:relid 4626062
:relkind f
:tablesample <>
:lateral false
:inh false
:inFromCl true
:requiredPerms 2
:checkAsUser 0
:selectedCols (b 9)
:insertedCols (b)
:updatedCols (b)
:securityQuals <>
}
{RTE
:alias <>
:eref
{ALIAS
:aliasname unnamed_join
:colnames ("c1" "c2" "t3_col1")
}
:rtekind 2
:jointype 0
:joinaliasvars <>
:lateral false
:inh false
:inFromCl true
:requiredPerms 0
:checkAsUser 0
:selectedCols (b)
:insertedCols (b)
:updatedCols (b)
:securityQuals <>
}
{RTE
:alias
{ALIAS
:aliasname ref_1
:colnames <>
}
:eref
{ALIAS
:aliasname ref_1
:colnames ("t2_col1")
}
:rtekind 0
:relid 4626059
:relkind f
:tablesample <>
:lateral false
:inh false
:inFromCl true
:requiredPerms 2
:checkAsUser 0
:selectedCols (b 9)
:insertedCols (b)
:updatedCols (b)
:securityQuals <>
}
)
:resultRelations <>
:utilityStmt <>
:subplans <>
:rewindPlanIDs (b)
:rowMarks <>
:relationOids (o 4626056 4626062 4626059)
:invalItems <>
:nParamExec 0
}
(2019/01/30 18:46), PG Bug reporting form wrote: > The following bug has been logged on the website: > > Bug reference: 15613 > Logged by: Srinivasan S A > Email address: srinivasan.sa@zohocorp.com > PostgreSQL version: 9.6.3 > Operating system: Linux and macOS > Description: > > Foreign scan of a table (part of join), has a column of another table > involved in its qual. This column is of type T_Var (instead of T_Param) > which could lead to errors or wrong results. This issue is reproducible in > all foreign data wrappers, including file_fdw. The same query runs fine for > PG Tables. > > Steps to reproduce in file_fdw: > > 1. Create 3 tables and analyze it: > > CREATE FOREIGN TABLE table1 ( > t1_col1 bigint, > t1_col2 bigint) > SERVER pglog options ( > filename 'table1.csv', > format 'csv', > DELIMITER '|' > ); > > CREATE FOREIGN TABLE table2 ( > t2_col1 bigint) > SERVER pglog options ( > filename 'table2.csv', > format 'csv', > DELIMITER '|' > ); > > CREATE FOREIGN TABLE table3 ( > t3_col1 bigint) > SERVER pglog options ( > filename 'table3.csv', > format 'csv', > DELIMITER '|' > ); > > 2. Run the query: > > SELECT > subq_1.c1 AS c1 > FROM > table1 AS ref_0, > LATERAL ( > SELECT > ref_0.t1_col1 AS c1, > subq_0.c1 AS c2, > subq_0.c2 AS c3 > FROM ( > SELECT > ref_1.t2_col1 AS c1, > ref_0.t1_col2 AS c2 > FROM > table2 AS ref_1 > WHERE > TRUE) AS subq_0 > RIGHT JOIN table3 AS ref_3 ON (subq_0.c1 = ref_3.t3_col1) > WHERE > pg_catalog.inet_client_port()< subq_0.c1) AS subq_1 > WHERE > subq_1.c3 IS NOT NULL > LIMIT 108; > > This throws an error: > > ERROR: attribute number 2 exceeds number of columns 1 > > Explain gives > > QUERY PLAN > > ---------------------------------------------------------------------------------- > Hash Join (cost=10000000001.11..10000000003.34 rows=1 width=8) > Hash Cond: (ref_1.t2_col1 = ref_3.t3_col1) > -> Nested Loop (cost=10000000000.00..10000000002.22 rows=1 width=16) > -> Foreign Scan on table1 ref_0 (cost=0.00..1.10 rows=1 > width=16) > Foreign File: > /Volumes/Official/workspace/Datasets/table1.csv > Foreign File Size: 4 > -> Foreign Scan on table2 ref_1 (cost=0.00..1.10 rows=1 > width=8) > Filter: ((t1_col2 IS NOT NULL) AND (inet_client_port()< > t2_col1)) > Foreign File: > /Volumes/Official/workspace/Datasets/table2.csv > Foreign File Size: 2 > -> Hash (cost=1.10..1.10 rows=1 width=8) > -> Foreign Scan on table3 ref_3 (cost=0.00..1.10 rows=1 > width=8) > Foreign File: > /Volumes/Official/workspace/Datasets/table2.csv > Foreign File Size: 2 > > Here, table2's qual involves table1's column t1_col2. In the plan, this > column should be of type T_Param instead of T_Var. Query plan below: I haven't looked into this in details yet, but do you have this issue in the latest release of 9.6 (ie 9.6.11), not in 9.6.3? I just thought this might have been fixed already by commits [1][2]. Best regards, Etsuro Fujita [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e5d83995e;hp=79b2e52615faa768d8436c1795e445541460e9d2 [2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c792c7db41466ff02107e3233ec9d92d8e3df866
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> (2019/01/30 18:46), PG Bug reporting form wrote:
>> Foreign scan of a table (part of join), has a column of another table
>> involved in its qual.
> I haven't looked into this in details yet, but do you have this issue in
> the latest release of 9.6 (ie 9.6.11), not in 9.6.3? I just thought
> this might have been fixed already by commits [1][2].
Nope, cause it's still there in HEAD :-(. What I'm wondering is why
the ref_0.t1_col2 IS NOT NULL condition is being pushed down to the
ref_1 scan in the first place, rather than ref_0 where you'd think it
should go. Yes, this plan shape could have been made to work if the
Var reference had been converted to a NestLoopParam reference, but
it wasn't. So there are a few interesting questions here:
* Why was the IS NOT NULL pushed down to the wrong place?
* Having made that mistake, why didn't the NLP mechanism fix it
and create a valid plan anyway?
* I find that s/RIGHT JOIN/JOIN/ makes the problem go away: the
IS NOT NULL is pushed to the right place and there's no error.
This is even weirder, because the planner successfully simplifies
the outer join to a plain join on its own, and you'd think that
that would happen before any mistake that could cause the other
two problems. Join strength reduction runs before
distribute_qual_to_rels, so how's that happening?
Probably the answers to these questions are interrelated and there's
only one bug at bottom. But maybe not. I've not really dug into
it yet.
regards, tom lane
I wrote:
> So there are a few interesting questions here:
> * Why was the IS NOT NULL pushed down to the wrong place?
> * Having made that mistake, why didn't the NLP mechanism fix it
> and create a valid plan anyway?
Hmm ... it looks like there is indeed more than one bug here.
The source of all the problems seems to be that the IS NOT NULL
variable is getting wrapped into a PlaceHolderVar incorrectly(?):
by the time it gets to distribute_qual_to_rels, it looks like
{NULLTEST
:arg
{PLACEHOLDERVAR
:phexpr
{VAR
:varno 1
:varattno 2
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 1
:varoattno 2
:location 275
}
:phrels (b 6)
:phid 1
:phlevelsup 0
}
:nulltesttype 1
:argisrow false
:location 456
}
I haven't tracked down why that's happening, but this parsetree
clearly says that Var 1/2 (that is, ref_0.t1_col2) should get
evaluated at the scan of RTE 6 (ref_1), which would then make
ref_1 be the natural place to do the filter check too. So given
that we built this PHV, the plan should come out the way it did.
The reason that we fail to make an NLP to make it work is that
create_foreignscan_plan doesn't think it needs to perform
replace_nestloop_params, because the Path is marked with null
param_info. And that is because fileGetForeignPaths unconditionally
passes required_outer = NULL to create_foreignscan_path.
What *ought* to be happening, of course, is what's documented in
most of the core path-creation functions, such as set_plain_rel_pathlist:
/*
* We don't support pushing join clauses into the quals of a seqscan, but
* it could still have required parameterization due to LATERAL refs in
* its tlist.
*/
required_outer = rel->lateral_relids;
and indeed changing fileGetForeignPaths like this:
startup_cost,
total_cost,
NIL, /* no pathkeys */
- NULL, /* no outer rel either */
+ baserel->lateral_relids,
NULL, /* no extra plan */
coptions));
is enough to make the failure go away: we still see the IS NOT NULL
getting applied in the arguably-wrong place, but the reference is
correctly parameterized so that the plan executes without error.
Observe that this *is* a bug independently of whether the PlaceHolderVar
generation is correct or not. Now that we've recognized that these calls
are wrong, it should be possible to create test cases that fail for these
FDWs without relying on that.
So I see two alternatives for fixing this aspect of the problem:
1. Just change file_fdw and postgres_fdw as above, and hope that
authors of extension FDWs get the word.
2. Modify create_foreignscan_path so that it doesn't simply trust
required_outer to be correct, but forcibly adds rel->lateral_relids
into it. This would fix the problem without requiring FDWs to be
on board, but it seems kinda grotty, and it penalizes FDWs that
have gone to the trouble of computing the right required_outer relids
in the first place. (It's somewhat amusing that postgres_fdw
appears to get this right when it generates parameterized paths,
but not in the base case.)
A variant of #2 that might make it seem less grotty is to decide that
*all* the path-node-creation functions in pathnode.c should be
responsible for adding rel->lateral_relids into the path's outerrels,
instead of expecting callers to do so, which would allow for some
simplification in (at least some of) the callers. This would be
a bit invasive, so I'd be inclined not to do it like that in the
back branches, but perhaps it's sensible in HEAD.
I don't have a huge amount of faith in #1, so I think we ought to do
one or the other variant of #2.
Thoughts?
regards, tom lane
I wrote:
> The source of all the problems seems to be that the IS NOT NULL
> variable is getting wrapped into a PlaceHolderVar incorrectly(?):
> by the time it gets to distribute_qual_to_rels, it looks like
> ...
> I haven't tracked down why that's happening, but this parsetree
> clearly says that Var 1/2 (that is, ref_0.t1_col2) should get
> evaluated at the scan of RTE 6 (ref_1), which would then make
> ref_1 be the natural place to do the filter check too.
Ah: that is expected after all. I hadn't looked closely enough at the
structure of the test query, but what the IS NOT NULL is supposed to test
is actually a lateral reference in the tlist of the ref_1 scan (subq_0).
That's underneath an outer join that could potentially force it to null,
so it needs to be evaluated under the outer join, and the PlaceHolderVar
is inserted to ensure that that happens.
Of course, since the outer join gets strength-reduced to a plain join,
we didn't really need to do any such thing. But PlaceHolderVar insertion
happens during pull_up_subqueries, which runs before reduce_outer_joins,
so the where-to-evaluate decision is made first. (And that explains why
the bug goes away if we manually reduce the join type.)
Switching the order of those two steps would not be an improvement,
either. In this example, for instance, the fact that we can reduce
the outer join depends on the WHERE IS NOT NULL test, which initially
is one subquery level above the outer join, so if we hadn't flattened
the subqueries we'd fail to prove that we could reduce the outer join.
In short, while this could theoretically be better it's pretty hard
to see how we could make it so, short of a vast and complex rewrite.
And the case is weird enough that it seems unlikely we'd be improving
very many real-world queries. (I'm betting, given what the test case
looks like, that this is a sqlsmith find rather than an anonymization
of a real query.)
So we're down to one bug: the FDWs are being careless about including
lateral_relids in their path parameterization.
regards, tom lane
(2019/01/31 2:48), Tom Lane wrote: > I wrote: >> So there are a few interesting questions here: >> * Why was the IS NOT NULL pushed down to the wrong place? >> * Having made that mistake, why didn't the NLP mechanism fix it >> and create a valid plan anyway? > The reason that we fail to make an NLP to make it work is that > create_foreignscan_plan doesn't think it needs to perform > replace_nestloop_params, because the Path is marked with null > param_info. And that is because fileGetForeignPaths unconditionally > passes required_outer = NULL to create_foreignscan_path. > > What *ought* to be happening, of course, is what's documented in > most of the core path-creation functions, such as set_plain_rel_pathlist: > > /* > * We don't support pushing join clauses into the quals of a seqscan, but > * it could still have required parameterization due to LATERAL refs in > * its tlist. > */ > required_outer = rel->lateral_relids; > > and indeed changing fileGetForeignPaths like this: > > startup_cost, > total_cost, > NIL, /* no pathkeys */ > - NULL, /* no outer rel either */ > + baserel->lateral_relids, > NULL, /* no extra plan */ > coptions)); > > is enough to make the failure go away: we still see the IS NOT NULL > getting applied in the arguably-wrong place, but the reference is > correctly parameterized so that the plan executes without error. > > Observe that this *is* a bug independently of whether the PlaceHolderVar > generation is correct or not. Now that we've recognized that these calls > are wrong, it should be possible to create test cases that fail for these > FDWs without relying on that. > > So I see two alternatives for fixing this aspect of the problem: > > 1. Just change file_fdw and postgres_fdw as above, and hope that > authors of extension FDWs get the word. > > 2. Modify create_foreignscan_path so that it doesn't simply trust > required_outer to be correct, but forcibly adds rel->lateral_relids > into it. This would fix the problem without requiring FDWs to be > on board, but it seems kinda grotty, and it penalizes FDWs that > have gone to the trouble of computing the right required_outer relids > in the first place. (It's somewhat amusing that postgres_fdw > appears to get this right when it generates parameterized paths, > but not in the base case.) #2 seems like a good idea, as it would make FDW authors' life easy. > A variant of #2 that might make it seem less grotty is to decide that > *all* the path-node-creation functions in pathnode.c should be > responsible for adding rel->lateral_relids into the path's outerrels, > instead of expecting callers to do so, which would allow for some > simplification in (at least some of) the callers. This would be > a bit invasive, so I'd be inclined not to do it like that in the > back branches, but perhaps it's sensible in HEAD. > > I don't have a huge amount of faith in #1, so I think we ought to do > one or the other variant of #2. Agreed. Thank you for working on this issue, as usual! Best regards, Etsuro Fujita
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> (2019/01/31 2:48), Tom Lane wrote:
>> So I see two alternatives for fixing this aspect of the problem:
>>
>> 1. Just change file_fdw and postgres_fdw as above, and hope that
>> authors of extension FDWs get the word.
>>
>> 2. Modify create_foreignscan_path so that it doesn't simply trust
>> required_outer to be correct, but forcibly adds rel->lateral_relids
>> into it. This would fix the problem without requiring FDWs to be
>> on board, but it seems kinda grotty, and it penalizes FDWs that
>> have gone to the trouble of computing the right required_outer relids
>> in the first place. (It's somewhat amusing that postgres_fdw
>> appears to get this right when it generates parameterized paths,
>> but not in the base case.)
> #2 seems like a good idea, as it would make FDW authors' life easy.
I started to fix this, and soon noticed what seems a worse problem:
postgres_fdw is using create_foreignscan_path to construct Paths for
join relations and upperrels. This is utterly broken. That function
was only designed to produce paths for baserels, which is why it uses
get_baserel_parampathinfo. At the very least we're getting wrong
rowcount estimates for parameterized joinrels (compare
get_joinrel_parampathinfo), and it seems possible that we're actually
getting wrong plans with the wrong set of movable join clauses being
applied. And I have no idea what might go wrong for upperrels, though
I think those are never parameterized so it might accidentally not fail.
We could either split the function into two or three functions, or add
still more overhead to it to notice what kind of relation has been
passed and adjust its behavior for that. I'm not really thrilled with
the latter: the fact that it's called create_foreignSCAN_path means,
to me, that it's not supposed to be used for anything but baserel
cases.
I think one big question here is how many external FDWs may have
copied postgres_fdw's remote-join handling. If we just have to
fix postgres_fdw, my thoughts about what to do are probably
different than if we have to try to avoid making third-party callers
more broken than they are already.
regards, tom lane
(2019/02/01 3:06), Tom Lane wrote: > Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp> writes: >> (2019/01/31 2:48), Tom Lane wrote: >>> So I see two alternatives for fixing this aspect of the problem: >>> >>> 1. Just change file_fdw and postgres_fdw as above, and hope that >>> authors of extension FDWs get the word. >>> >>> 2. Modify create_foreignscan_path so that it doesn't simply trust >>> required_outer to be correct, but forcibly adds rel->lateral_relids >>> into it. This would fix the problem without requiring FDWs to be >>> on board, but it seems kinda grotty, and it penalizes FDWs that >>> have gone to the trouble of computing the right required_outer relids >>> in the first place. (It's somewhat amusing that postgres_fdw >>> appears to get this right when it generates parameterized paths, >>> but not in the base case.) > >> #2 seems like a good idea, as it would make FDW authors' life easy. > > I started to fix this, and soon noticed what seems a worse problem: > postgres_fdw is using create_foreignscan_path to construct Paths for > join relations and upperrels. This is utterly broken. That function > was only designed to produce paths for baserels, which is why it uses > get_baserel_parampathinfo. At the very least we're getting wrong > rowcount estimates for parameterized joinrels (compare > get_joinrel_parampathinfo), and it seems possible that we're actually > getting wrong plans with the wrong set of movable join clauses being > applied. And I have no idea what might go wrong for upperrels, though > I think those are never parameterized so it might accidentally not fail. Ah, you are right. I also noticed that when I proposed parameterized foreign joins for postgres_fdw two years ago, but I forgot that. > We could either split the function into two or three functions, or add > still more overhead to it to notice what kind of relation has been > passed and adjust its behavior for that. I'm not really thrilled with > the latter: the fact that it's called create_foreignSCAN_path means, > to me, that it's not supposed to be used for anything but baserel > cases. I don't have any strong opinion on that. > I think one big question here is how many external FDWs may have > copied postgres_fdw's remote-join handling. If we just have to > fix postgres_fdw, my thoughts about what to do are probably > different than if we have to try to avoid making third-party callers > more broken than they are already. As far as I know oracle_fdw supports join pushdown the same way as postgres_fdw [1], but other than that, I guess there are few if any. Best regards, Etsuro Fujita [1] http://laurenz.github.io/oracle_fdw/
(2019/02/06 13:15), Etsuro Fujita wrote: > (2019/02/01 3:06), Tom Lane wrote: >> Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp> writes: >>> (2019/01/31 2:48), Tom Lane wrote: >>>> So I see two alternatives for fixing this aspect of the problem: >>>> >>>> 1. Just change file_fdw and postgres_fdw as above, and hope that >>>> authors of extension FDWs get the word. >>>> >>>> 2. Modify create_foreignscan_path so that it doesn't simply trust >>>> required_outer to be correct, but forcibly adds rel->lateral_relids >>>> into it. This would fix the problem without requiring FDWs to be >>>> on board, but it seems kinda grotty, and it penalizes FDWs that >>>> have gone to the trouble of computing the right required_outer relids >>>> in the first place. (It's somewhat amusing that postgres_fdw >>>> appears to get this right when it generates parameterized paths, >>>> but not in the base case.) >> >>> #2 seems like a good idea, as it would make FDW authors' life easy. >> >> I started to fix this, and soon noticed what seems a worse problem: >> postgres_fdw is using create_foreignscan_path to construct Paths for >> join relations and upperrels. This is utterly broken. That function >> was only designed to produce paths for baserels, which is why it uses >> get_baserel_parampathinfo. At the very least we're getting wrong >> rowcount estimates for parameterized joinrels (compare >> get_joinrel_parampathinfo), and it seems possible that we're actually >> getting wrong plans with the wrong set of movable join clauses being >> applied. And I have no idea what might go wrong for upperrels, though >> I think those are never parameterized so it might accidentally not fail. > > Ah, you are right. I also noticed that when I proposed parameterized > foreign joins for postgres_fdw two years ago, but I forgot that. > >> We could either split the function into two or three functions, or add >> still more overhead to it to notice what kind of relation has been >> passed and adjust its behavior for that. I'm not really thrilled with >> the latter: the fact that it's called create_foreignSCAN_path means, >> to me, that it's not supposed to be used for anything but baserel >> cases. > > I don't have any strong opinion on that. On second thoughts, I think it would be a good idea to split that function, because we can minimize the parameters list passed to each function, making it easy to call that function; as you mentioned, 'required_outer' isn't required for upperrels, and 'fdw_outerpath' isn't required for baserels and upperrels. Not sure we should do that for PG12. >> I think one big question here is how many external FDWs may have >> copied postgres_fdw's remote-join handling. If we just have to >> fix postgres_fdw, my thoughts about what to do are probably >> different than if we have to try to avoid making third-party callers >> more broken than they are already. > > As far as I know oracle_fdw supports join pushdown the same way as > postgres_fdw [1], but other than that, I guess there are few if any. Maybe I'm missing something, but even if we just fix postgres_fdw (and oracle_fdw), I'm not sure we can fix the foreign-join case fully without extending the fdw_outerpath infrastructure so that we can create *parameterized* local join paths. Best regards, Etsuro Fujita
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
>>> We could either split the function into two or three functions, or add
>>> still more overhead to it to notice what kind of relation has been
>>> passed and adjust its behavior for that. I'm not really thrilled with
>>> the latter: the fact that it's called create_foreignSCAN_path means,
>>> to me, that it's not supposed to be used for anything but baserel
>>> cases.
>> I don't have any strong opinion on that.
> On second thoughts, I think it would be a good idea to split that
> function, because we can minimize the parameters list passed to each
> function, making it easy to call that function; as you mentioned,
> 'required_outer' isn't required for upperrels, and 'fdw_outerpath' isn't
> required for baserels and upperrels. Not sure we should do that for PG12.
Yeah, I agree. Attached is a draft of that. I've not thought very hard
about how we would want to manage parameterization of foreign joins, so
for now create_foreign_join_path doesn't support that. We'll have to
change its API whenever we do want to support that, which is a good reason
why it should be separate from create_foreignscan_path: that way we don't
break API for FDWs that are only implementing plain scans.
I propose that we apply this or something much like it to HEAD, and leave
the problem of actually working out parameterized foreign joins for later.
Not sure whether we should do the same in the back branches. It might be
fine to decide that we're never going to support parameterized foreign
joins in the back branches, in which case I think just adding the Assert
to create_foreignscan_path would be enough there.
regards, tom lane
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index be626be..391cd0d 100644
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
*************** fileGetForeignPaths(PlannerInfo *root,
*** 556,561 ****
--- 556,565 ----
* Create a ForeignPath node and add it as only possible path. We use the
* fdw_private list of the path to carry the convert_selectively option;
* it will be propagated into the fdw_private list of the Plan node.
+ *
+ * We don't support pushing join clauses into the quals of this path, but
+ * it could still have required parameterization due to LATERAL refs in
+ * its tlist.
*/
add_path(baserel, (Path *)
create_foreignscan_path(root, baserel,
*************** fileGetForeignPaths(PlannerInfo *root,
*** 564,570 ****
startup_cost,
total_cost,
NIL, /* no pathkeys */
! NULL, /* no outer rel either */
NULL, /* no extra plan */
coptions));
--- 568,574 ----
startup_cost,
total_cost,
NIL, /* no pathkeys */
! baserel->lateral_relids,
NULL, /* no extra plan */
coptions));
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 7fcac81..994cec5 100644
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
*************** postgresGetForeignPaths(PlannerInfo *roo
*** 937,942 ****
--- 937,945 ----
* baserestrict conditions we were able to send to remote, there might
* actually be an indexscan happening there). We already did all the work
* to estimate cost and size of this path.
+ *
+ * Although this path uses no join clauses, it could still have required
+ * parameterization due to LATERAL refs in its tlist.
*/
path = create_foreignscan_path(root, baserel,
NULL, /* default pathtarget */
*************** postgresGetForeignPaths(PlannerInfo *roo
*** 944,950 ****
fpinfo->startup_cost,
fpinfo->total_cost,
NIL, /* no pathkeys */
! NULL, /* no outer rel either */
NULL, /* no extra plan */
NIL); /* no fdw_private list */
add_path(baserel, (Path *) path);
--- 947,953 ----
fpinfo->startup_cost,
fpinfo->total_cost,
NIL, /* no pathkeys */
! baserel->lateral_relids,
NULL, /* no extra plan */
NIL); /* no fdw_private list */
add_path(baserel, (Path *) path);
*************** add_paths_with_pathkeys_for_rel(PlannerI
*** 4943,4958 ****
useful_pathkeys,
-1.0);
! add_path(rel, (Path *)
! create_foreignscan_path(root, rel,
! NULL,
! rows,
! startup_cost,
! total_cost,
! useful_pathkeys,
! NULL,
! sorted_epq_path,
! NIL));
}
}
--- 4947,4974 ----
useful_pathkeys,
-1.0);
! if (IS_SIMPLE_REL(rel))
! add_path(rel, (Path *)
! create_foreignscan_path(root, rel,
! NULL,
! rows,
! startup_cost,
! total_cost,
! useful_pathkeys,
! rel->lateral_relids,
! sorted_epq_path,
! NIL));
! else
! add_path(rel, (Path *)
! create_foreign_join_path(root, rel,
! NULL,
! rows,
! startup_cost,
! total_cost,
! useful_pathkeys,
! rel->lateral_relids,
! sorted_epq_path,
! NIL));
}
}
*************** postgresGetForeignJoinPaths(PlannerInfo
*** 5088,5093 ****
--- 5104,5116 ----
return;
/*
+ * This code does not work for joins with lateral references, since those
+ * must have parameterized paths, which we don't generate yet.
+ */
+ if (!bms_is_empty(joinrel->lateral_relids))
+ return;
+
+ /*
* Create unfinished PgFdwRelationInfo entry which is used to indicate
* that the join relation is already considered, so that we won't waste
* time in judging safety of join pushdown and adding the same paths again
*************** postgresGetForeignJoinPaths(PlannerInfo
*** 5171,5186 ****
* Create a new join path and add it to the joinrel which represents a
* join between foreign tables.
*/
! joinpath = create_foreignscan_path(root,
! joinrel,
! NULL, /* default pathtarget */
! rows,
! startup_cost,
! total_cost,
! NIL, /* no pathkeys */
! NULL, /* no required_outer */
! epq_path,
! NIL); /* no fdw_private */
/* Add generated path into joinrel by add_path(). */
add_path(joinrel, (Path *) joinpath);
--- 5194,5209 ----
* Create a new join path and add it to the joinrel which represents a
* join between foreign tables.
*/
! joinpath = create_foreign_join_path(root,
! joinrel,
! NULL, /* default pathtarget */
! rows,
! startup_cost,
! total_cost,
! NIL, /* no pathkeys */
! joinrel->lateral_relids,
! epq_path,
! NIL); /* no fdw_private */
/* Add generated path into joinrel by add_path(). */
add_path(joinrel, (Path *) joinpath);
*************** add_foreign_grouping_paths(PlannerInfo *
*** 5515,5530 ****
fpinfo->total_cost = total_cost;
/* Create and add foreign path to the grouping relation. */
! grouppath = create_foreignscan_path(root,
! grouped_rel,
! grouped_rel->reltarget,
! rows,
! startup_cost,
! total_cost,
! NIL, /* no pathkeys */
! NULL, /* no required_outer */
! NULL,
! NIL); /* no fdw_private */
/* Add generated path into grouped_rel by add_path(). */
add_path(grouped_rel, (Path *) grouppath);
--- 5538,5552 ----
fpinfo->total_cost = total_cost;
/* Create and add foreign path to the grouping relation. */
! grouppath = create_foreign_upper_path(root,
! grouped_rel,
! grouped_rel->reltarget,
! rows,
! startup_cost,
! total_cost,
! NIL, /* no pathkeys */
! NULL,
! NIL); /* no fdw_private */
/* Add generated path into grouped_rel by add_path(). */
add_path(grouped_rel, (Path *) grouppath);
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 452b776..77038f5 100644
*** a/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
*************** GetForeignJoinPaths(PlannerInfo *root,
*** 309,315 ****
function is called during query planning. As
with <function>GetForeignPaths</function>, this function should
generate <structname>ForeignPath</structname> path(s) for the
! supplied <literal>joinrel</literal>, and call <function>add_path</function> to add these
paths to the set of paths considered for the join. But unlike
<function>GetForeignPaths</function>, it is not necessary that this function
succeed in creating at least one path, since paths involving local
--- 309,317 ----
function is called during query planning. As
with <function>GetForeignPaths</function>, this function should
generate <structname>ForeignPath</structname> path(s) for the
! supplied <literal>joinrel</literal>
! (use <function>create_foreign_join_path</function> to build them),
! and call <function>add_path</function> to add these
paths to the set of paths considered for the join. But unlike
<function>GetForeignPaths</function>, it is not necessary that this function
succeed in creating at least one path, since paths involving local
*************** GetForeignUpperPaths(PlannerInfo *root,
*** 369,375 ****
called only if all base relation(s) involved in the query belong to the
same FDW. This function should generate <structname>ForeignPath</structname>
path(s) for any post-scan/join processing that the FDW knows how to
! perform remotely, and call <function>add_path</function> to add these paths to
the indicated upper relation. As with <function>GetForeignJoinPaths</function>,
it is not necessary that this function succeed in creating any paths,
since paths involving local processing are always possible.
--- 371,379 ----
called only if all base relation(s) involved in the query belong to the
same FDW. This function should generate <structname>ForeignPath</structname>
path(s) for any post-scan/join processing that the FDW knows how to
! perform remotely
! (use <function>create_foreign_upper_path</function> to build them),
! and call <function>add_path</function> to add these paths to
the indicated upper relation. As with <function>GetForeignJoinPaths</function>,
it is not necessary that this function succeed in creating any paths,
since paths involving local processing are always possible.
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index b57de6b..9a53e04 100644
*** a/src/backend/optimizer/util/pathnode.c
--- b/src/backend/optimizer/util/pathnode.c
*************** create_worktablescan_path(PlannerInfo *r
*** 2079,2093 ****
/*
* create_foreignscan_path
! * Creates a path corresponding to a scan of a foreign table, foreign join,
! * or foreign upper-relation processing, returning the pathnode.
*
* This function is never called from core Postgres; rather, it's expected
! * to be called by the GetForeignPaths, GetForeignJoinPaths, or
! * GetForeignUpperPaths function of a foreign data wrapper. We make the FDW
! * supply all fields of the path, since we do not have any way to calculate
! * them in core. However, there is a usually-sane default for the pathtarget
! * (rel->reltarget), so we let a NULL for "target" select that.
*/
ForeignPath *
create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
--- 2079,2092 ----
/*
* create_foreignscan_path
! * Creates a path corresponding to a scan of a foreign base table,
! * returning the pathnode.
*
* This function is never called from core Postgres; rather, it's expected
! * to be called by the GetForeignPaths function of a foreign data wrapper.
! * We make the FDW supply all fields of the path, since we do not have any way
! * to calculate them in core. However, there is a usually-sane default for
! * the pathtarget (rel->reltarget), so we let a NULL for "target" select that.
*/
ForeignPath *
create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
*************** create_foreignscan_path(PlannerInfo *roo
*** 2100,2105 ****
--- 2099,2125 ----
{
ForeignPath *pathnode = makeNode(ForeignPath);
+ /*
+ * Since the path's required_outer should always include all the rel's
+ * lateral_relids, forcibly add those if necessary. This is a bit of a
+ * hack, but up till early 2019 the contrib FDWs failed to ensure that,
+ * and it's likely that the same error has propagated into many external
+ * FDWs. Don't risk modifying the passed-in relid set here.
+ */
+ if (rel->lateral_relids && !bms_is_subset(rel->lateral_relids,
+ required_outer))
+ required_outer = bms_union(required_outer, rel->lateral_relids);
+
+ /*
+ * Although this function is only meant to be used for scans of baserels,
+ * up till early 2019 postgres_fdw abused it to make paths for join and
+ * upper rels, and the same error may have propagated elsewhere. It will
+ * work for such cases as long as required_outer is empty (otherwise
+ * get_baserel_parampathinfo does the wrong thing), so this assertion lets
+ * that past.
+ */
+ Assert(bms_is_empty(required_outer) || IS_SIMPLE_REL(rel));
+
pathnode->path.pathtype = T_ForeignScan;
pathnode->path.parent = rel;
pathnode->path.pathtarget = target ? target : rel->reltarget;
*************** create_foreignscan_path(PlannerInfo *roo
*** 2120,2125 ****
--- 2140,2251 ----
}
/*
+ * create_foreign_join_path
+ * Creates a path corresponding to a scan of a foreign join,
+ * returning the pathnode.
+ *
+ * This function is never called from core Postgres; rather, it's expected
+ * to be called by the GetForeignJoinPaths function of a foreign data wrapper.
+ * We make the FDW supply all fields of the path, since we do not have any way
+ * to calculate them in core. However, there is a usually-sane default for
+ * the pathtarget (rel->reltarget), so we let a NULL for "target" select that.
+ */
+ ForeignPath *
+ create_foreign_join_path(PlannerInfo *root, RelOptInfo *rel,
+ PathTarget *target,
+ double rows, Cost startup_cost, Cost total_cost,
+ List *pathkeys,
+ Relids required_outer,
+ Path *fdw_outerpath,
+ List *fdw_private)
+ {
+ ForeignPath *pathnode = makeNode(ForeignPath);
+
+ /*
+ * Since the path's required_outer should always include all the rel's
+ * lateral_relids, forcibly add those if necessary. This is a bit of a
+ * hack, but up till early 2019 the contrib FDWs failed to ensure that,
+ * and it's likely that the same error has propagated into many external
+ * FDWs. Don't risk modifying the passed-in relid set here.
+ */
+ if (rel->lateral_relids && !bms_is_subset(rel->lateral_relids,
+ required_outer))
+ required_outer = bms_union(required_outer, rel->lateral_relids);
+
+ /*
+ * We should use get_joinrel_parampathinfo to handle parameterized paths,
+ * but the API of this function doesn't support it, and existing
+ * extensions aren't trying to build such paths yet anyway. For the
+ * moment just throw an error if someone tries it; eventually we should
+ * revisit this.
+ */
+ if (!bms_is_empty(required_outer))
+ elog(ERROR, "parameterized foreign joins are not supported yet");
+
+ pathnode->path.pathtype = T_ForeignScan;
+ pathnode->path.parent = rel;
+ pathnode->path.pathtarget = target ? target : rel->reltarget;
+ pathnode->path.param_info = NULL; /* XXX see above */
+ pathnode->path.parallel_aware = false;
+ pathnode->path.parallel_safe = rel->consider_parallel;
+ pathnode->path.parallel_workers = 0;
+ pathnode->path.rows = rows;
+ pathnode->path.startup_cost = startup_cost;
+ pathnode->path.total_cost = total_cost;
+ pathnode->path.pathkeys = pathkeys;
+
+ pathnode->fdw_outerpath = fdw_outerpath;
+ pathnode->fdw_private = fdw_private;
+
+ return pathnode;
+ }
+
+ /*
+ * create_foreign_upper_path
+ * Creates a path corresponding to an upper relation that's computed
+ * directly by an FDW, returning the pathnode.
+ *
+ * This function is never called from core Postgres; rather, it's expected to
+ * be called by the GetForeignUpperPaths function of a foreign data wrapper.
+ * We make the FDW supply all fields of the path, since we do not have any way
+ * to calculate them in core. However, there is a usually-sane default for
+ * the pathtarget (rel->reltarget), so we let a NULL for "target" select that.
+ */
+ ForeignPath *
+ create_foreign_upper_path(PlannerInfo *root, RelOptInfo *rel,
+ PathTarget *target,
+ double rows, Cost startup_cost, Cost total_cost,
+ List *pathkeys,
+ Path *fdw_outerpath,
+ List *fdw_private)
+ {
+ ForeignPath *pathnode = makeNode(ForeignPath);
+
+ /*
+ * Upper relations should never have any lateral references, since joining
+ * is complete.
+ */
+ Assert(bms_is_empty(rel->lateral_relids));
+
+ pathnode->path.pathtype = T_ForeignScan;
+ pathnode->path.parent = rel;
+ pathnode->path.pathtarget = target ? target : rel->reltarget;
+ pathnode->path.param_info = NULL;
+ pathnode->path.parallel_aware = false;
+ pathnode->path.parallel_safe = rel->consider_parallel;
+ pathnode->path.parallel_workers = 0;
+ pathnode->path.rows = rows;
+ pathnode->path.startup_cost = startup_cost;
+ pathnode->path.total_cost = total_cost;
+ pathnode->path.pathkeys = pathkeys;
+
+ pathnode->fdw_outerpath = fdw_outerpath;
+ pathnode->fdw_private = fdw_private;
+
+ return pathnode;
+ }
+
+ /*
* calc_nestloop_required_outer
* Compute the required_outer set for a nestloop join path
*
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index f04c6b7..4130514 100644
*** a/src/backend/optimizer/util/relnode.c
--- b/src/backend/optimizer/util/relnode.c
*************** get_baserel_parampathinfo(PlannerInfo *r
*** 1225,1230 ****
--- 1225,1233 ----
double rows;
ListCell *lc;
+ /* If rel has LATERAL refs, every path for it should account for them */
+ Assert(bms_is_subset(baserel->lateral_relids, required_outer));
+
/* Unparameterized paths have no ParamPathInfo */
if (bms_is_empty(required_outer))
return NULL;
*************** get_joinrel_parampathinfo(PlannerInfo *r
*** 1320,1325 ****
--- 1323,1331 ----
double rows;
ListCell *lc;
+ /* If rel has LATERAL refs, every path for it should account for them */
+ Assert(bms_is_subset(joinrel->lateral_relids, required_outer));
+
/* Unparameterized paths have no ParamPathInfo or extra join clauses */
if (bms_is_empty(required_outer))
return NULL;
*************** get_appendrel_parampathinfo(RelOptInfo *
*** 1511,1516 ****
--- 1517,1525 ----
{
ParamPathInfo *ppi;
+ /* If rel has LATERAL refs, every path for it should account for them */
+ Assert(bms_is_subset(appendrel->lateral_relids, required_outer));
+
/* Unparameterized paths have no ParamPathInfo */
if (bms_is_empty(required_outer))
return NULL;
diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h
index d0c8f99..ef2c9b4 100644
*** a/src/include/optimizer/pathnode.h
--- b/src/include/optimizer/pathnode.h
*************** extern ForeignPath *create_foreignscan_p
*** 118,123 ****
--- 118,136 ----
Relids required_outer,
Path *fdw_outerpath,
List *fdw_private);
+ extern ForeignPath *create_foreign_join_path(PlannerInfo *root, RelOptInfo *rel,
+ PathTarget *target,
+ double rows, Cost startup_cost, Cost total_cost,
+ List *pathkeys,
+ Relids required_outer,
+ Path *fdw_outerpath,
+ List *fdw_private);
+ extern ForeignPath *create_foreign_upper_path(PlannerInfo *root, RelOptInfo *rel,
+ PathTarget *target,
+ double rows, Cost startup_cost, Cost total_cost,
+ List *pathkeys,
+ Path *fdw_outerpath,
+ List *fdw_private);
extern Relids calc_nestloop_required_outer(Relids outerrelids,
Relids outer_paramrels,
(2019/02/07 4:15), Tom Lane wrote: > Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp> writes: >>>> We could either split the function into two or three functions, or add >>>> still more overhead to it to notice what kind of relation has been >>>> passed and adjust its behavior for that. I'm not really thrilled with >>>> the latter: the fact that it's called create_foreignSCAN_path means, >>>> to me, that it's not supposed to be used for anything but baserel >>>> cases. > >>> I don't have any strong opinion on that. > >> On second thoughts, I think it would be a good idea to split that >> function, because we can minimize the parameters list passed to each >> function, making it easy to call that function; as you mentioned, >> 'required_outer' isn't required for upperrels, and 'fdw_outerpath' isn't >> required for baserels and upperrels. Not sure we should do that for PG12. > > Yeah, I agree. Attached is a draft of that. I've not thought very hard > about how we would want to manage parameterization of foreign joins, so > for now create_foreign_join_path doesn't support that. We'll have to > change its API whenever we do want to support that, which is a good reason > why it should be separate from create_foreignscan_path: that way we don't > break API for FDWs that are only implementing plain scans. > > I propose that we apply this or something much like it to HEAD, and leave > the problem of actually working out parameterized foreign joins for later. Agreed. One thing I noticed is this bit in create_foreign_join_path: + /* + * Since the path's required_outer should always include all the rel's + * lateral_relids, forcibly add those if necessary. This is a bit of a + * hack, but up till early 2019 the contrib FDWs failed to ensure that, + * and it's likely that the same error has propagated into many external + * FDWs. Don't risk modifying the passed-in relid set here. + */ + if (rel->lateral_relids && !bms_is_subset(rel->lateral_relids, + required_outer)) + required_outer = bms_union(required_outer, rel->lateral_relids); I think this would waste cycles. Do we really need this? How about just throwing an error instead of doing this, if the joinrel has lateral references? Another thing is that it might be better to add regression test cases. Other than that, the patch looks good to me. > Not sure whether we should do the same in the back branches. It might be > fine to decide that we're never going to support parameterized foreign > joins in the back branches, in which case I think just adding the Assert > to create_foreignscan_path would be enough there. I might be missing something, but I don't see any need to support that in the back branches in future. Thank you for taking the time to create the patch! Best regards, Etsuro Fujita
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
>> I propose that we apply this or something much like it to HEAD, and leave
>> the problem of actually working out parameterized foreign joins for later.
> Agreed. One thing I noticed is this bit in create_foreign_join_path:
> + /*
> + * Since the path's required_outer should always include all the rel's
> + * lateral_relids, forcibly add those if necessary. This is a bit of a
> + * hack, but up till early 2019 the contrib FDWs failed to ensure that,
> + * and it's likely that the same error has propagated into many external
> + * FDWs. Don't risk modifying the passed-in relid set here.
> + */
> + if (rel->lateral_relids && !bms_is_subset(rel->lateral_relids,
> + required_outer))
> + required_outer = bms_union(required_outer, rel->lateral_relids);
> I think this would waste cycles. Do we really need this? How about
> just throwing an error instead of doing this, if the joinrel has lateral
> references?
It ends up being the same thing, because of the test below it, but
at least in HEAD yes this could be simplified. What I posted is a bit
schizophrenic as to whether it's intended for HEAD or back branches.
Probably, in HEAD there is no good reason to cater for buggy FDWs at
all; the new Asserts in relnode.c are enough, assuming that FDW authors
run some tests while updating to v12. And if we decide not to backpatch
create_foreign_join_path at all, then it doesn't need a defense like the
above either. So we just need the above-quoted bit in the back branch
versions of create_foreignscan_path.
regards, tom lane