Обсуждение: "could not find pathkey item to sort" for TPC-DS queries 94-96
Hi, When trying to run on master (but afaik also PG-13) TPC-DS queries 94, 95 and 96 on a SF10 I get the error "could not find pathkey item to sort". When I disable enable_gathermerge the problem goes away and then the plan for query 94 looks like below. I tried figuring out what the problem is but to be honest I would need some pointers as the code that tries to matching equivalence members in prepare_sort_from_pathkeys is something i'm really not familiar with. To reproduce you can either ingest and test using the toolkit I used too (see https://github.com/swarm64/s64da-benchmark-toolkit/), or alternatively just use the schema (see https://github.com/swarm64/s64da-benchmark-toolkit/tree/master/benchmarks/tpcds/schemas/psql_native) Best, Luc ------------------------------------------------------------------------------------------------------------------------------------------------ Limit (cost=229655.62..229655.63 rows=1 width=72) -> Sort (cost=229655.62..229655.63 rows=1 width=72) Sort Key: (count(DISTINCT ws1.ws_order_number)) -> Aggregate (cost=229655.60..229655.61 rows=1 width=72) -> Nested Loop Semi Join (cost=1012.65..229655.59 rows=1 width=16) -> Nested Loop (cost=1012.22..229653.73 rows=1 width=20) Join Filter: (ws1.ws_web_site_sk = web_site.web_site_sk) -> Nested Loop (cost=1012.22..229651.08 rows=1 width=24) -> Gather (cost=1011.80..229650.64 rows=1 width=28) Workers Planned: 2 -> Nested Loop Anti Join (cost=11.80..228650.54 rows=1 width=28) -> Hash Join (cost=11.37..227438.35 rows=2629 width=28) Hash Cond: (ws1.ws_ship_date_sk = date_dim.d_date_sk) -> Parallel Seq Scan on web_sales ws1 (cost=0.00..219548.92 rows=3000992 width=32) -> Hash (cost=10.57..10.57 rows=64 width=4) -> Index Scan using idx_d_date on date_dim (cost=0.29..10.57 rows=64 width=4) Index Cond: ((d_date >= '2000-03-01'::date) AND (d_date <= '2000-04-30'::date)) -> Index Only Scan using idx_wr_order_number on web_returns wr1 (cost=0.42..0.46 rows=2 width=4) Index Cond: (wr_order_number = ws1.ws_order_number) -> Index Scan using customer_address_pkey on customer_address (cost=0.42..0.44 rows=1 width=4) Index Cond: (ca_address_sk = ws1.ws_ship_addr_sk) Filter: ((ca_state)::text = 'GA'::text) -> Seq Scan on web_site (cost=0.00..2.52 rows=10 width=4) Filter: ((web_company_name)::text = 'pri'::text) -> Index Scan using idx_ws_order_number on web_sales ws2 (cost=0.43..1.84 rows=59 width=8) Index Cond: (ws_order_number = ws1.ws_order_number) Filter: (ws1.ws_warehouse_sk <> ws_warehouse_sk) The top of the stacktrace is: #0 errfinish (filename=0x5562dc1a5125 "createplan.c", lineno=6186, funcname=0x5562dc1a54d0 <__func__.14> "prepare_sort_from_pathkeys") at elog.c:514 #1 0x00005562dbc2d7de in prepare_sort_from_pathkeys (lefttree=0x5562dc5a2f58, pathkeys=0x5562dc4eabc8, relids=0x0, reqColIdx=0x0, adjust_tlist_in_place=<optimized out>, p_numsortkeys=0x7ffc0b8cda84, p_sortColIdx=0x7ffc0b8cda88, p_sortOperators=0x7ffc0b8cda90, p_collations=0x7ffc0b8cda98, p_nullsFirst=0x7ffc0b8cdaa0) at createplan.c:6186 #2 0x00005562dbe8d695 in make_sort_from_pathkeys (lefttree=<optimized out>, pathkeys=<optimized out>, relids=<optimized out>) at createplan.c:6313 #3 0x00005562dbe8eba3 in create_sort_plan (flags=1, best_path=0x5562dc548d68, root=0x5562dc508cf8) at createplan.c:2118 #4 create_plan_recurse (root=0x5562dc508cf8, best_path=0x5562dc548d68, flags=1) at createplan.c:489 #5 0x00005562dbe8f315 in create_gather_merge_plan (best_path=0x5562dc5782f8, root=0x5562dc508cf8) at createplan.c:1885 #6 create_plan_recurse (root=0x5562dc508cf8, best_path=0x5562dc5782f8, flags=<optimized out>) at createplan.c:541 #7 0x00005562dbe8ddad in create_nestloop_plan (best_path=0x5562dc585668, root=0x5562dc508cf8) at createplan.c:4237 #8 create_join_plan (best_path=0x5562dc585668, root=0x5562dc508cf8) at createplan.c:1062 #9 create_plan_recurse (root=0x5562dc508cf8, best_path=0x5562dc585668, flags=<optimized out>) at createplan.c:418 #10 0x00005562dbe8ddad in create_nestloop_plan (best_path=0x5562dc5c4428, root=0x5562dc508cf8) at createplan.c:4237 #11 create_join_plan (best_path=0x5562dc5c4428, root=0x5562dc508cf8) at createplan.c:1062 #12 create_plan_recurse (root=0x5562dc508cf8, best_path=0x5562dc5c4428, flags=<optimized out>) at createplan.c:418 #13 0x00005562dbe8ddad in create_nestloop_plan (best_path=0x5562dc5d3bd8, root=0x5562dc508cf8) at createplan.c:4237 #14 create_join_plan (best_path=0x5562dc5d3bd8, root=0x5562dc508cf8) at createplan.c:1062 #15 create_plan_recurse (root=0x5562dc508cf8, best_path=0x5562dc5d3bd8, flags=<optimized out>) at createplan.c:418 #16 0x00005562dbe8e428 in create_agg_plan (best_path=0x5562dc5d6f08, root=0x5562dc508cf8) at createplan.c:2238 #17 create_plan_recurse (root=0x5562dc508cf8, best_path=0x5562dc5d6f08, flags=3) at createplan.c:509 #18 0x00005562dbe8eb73 in create_sort_plan (flags=1, best_path=0x5562dc5d7378, root=0x5562dc508cf8) at createplan.c:2109 #19 create_plan_recurse (root=0x5562dc508cf8, best_path=0x5562dc5d7378, flags=1) at createplan.c:489 #20 0x00005562dbe8e7e8 in create_limit_plan (flags=1, best_path=0x5562dc5d7a08, root=0x5562dc508cf8) at createplan.c:2784 #21 create_plan_recurse (root=0x5562dc508cf8, best_path=0x5562dc5d7a08, flags=1) at createplan.c:536 #22 0x00005562dbe914ae in create_plan (root=root@entry=0x5562dc508cf8, best_path=<optimized out>) at createplan.c:349
On 4/12/21 2:24 PM, Luc Vlaming wrote: > Hi, > > When trying to run on master (but afaik also PG-13) TPC-DS queries 94, > 95 and 96 on a SF10 I get the error "could not find pathkey item to sort". > When I disable enable_gathermerge the problem goes away and then the > plan for query 94 looks like below. I tried figuring out what the > problem is but to be honest I would need some pointers as the code that > tries to matching equivalence members in prepare_sort_from_pathkeys is > something i'm really not familiar with. > Could be related to incremental sort, which allowed some gather merge paths that were impossible before. We had a couple issues related to that fixed in November, IIRC. > To reproduce you can either ingest and test using the toolkit I used too > (see https://github.com/swarm64/s64da-benchmark-toolkit/), or > alternatively just use the schema (see > https://github.com/swarm64/s64da-benchmark-toolkit/tree/master/benchmarks/tpcds/schemas/psql_native) > Thanks, I'll see if I can reproduce that with your schema. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Apr 12, 2021 at 8:37 AM Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > On 4/12/21 2:24 PM, Luc Vlaming wrote: > > Hi, > > > > When trying to run on master (but afaik also PG-13) TPC-DS queries 94, > > 95 and 96 on a SF10 I get the error "could not find pathkey item to sort". > > When I disable enable_gathermerge the problem goes away and then the > > plan for query 94 looks like below. I tried figuring out what the > > problem is but to be honest I would need some pointers as the code that > > tries to matching equivalence members in prepare_sort_from_pathkeys is > > something i'm really not familiar with. > > > > Could be related to incremental sort, which allowed some gather merge > paths that were impossible before. We had a couple issues related to > that fixed in November, IIRC. > > > To reproduce you can either ingest and test using the toolkit I used too > > (see https://github.com/swarm64/s64da-benchmark-toolkit/), or > > alternatively just use the schema (see > > https://github.com/swarm64/s64da-benchmark-toolkit/tree/master/benchmarks/tpcds/schemas/psql_native) > > > > Thanks, I'll see if I can reproduce that with your schema. > > > regards > > -- > Tomas Vondra > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company The query in question is: select count(*) from store_sales ,household_demographics ,time_dim, store where ss_sold_time_sk = time_dim.t_time_sk and ss_hdemo_sk = household_demographics.hd_demo_sk and ss_store_sk = s_store_sk and time_dim.t_hour = 15 and time_dim.t_minute >= 30 and household_demographics.hd_dep_count = 7 and store.s_store_name = 'ese' order by count(*) limit 100; From debugging output it looks like this is the plan being chosen (cheapest total path): Gather(store_sales household_demographics time_dim) rows=60626 cost=3145.73..699910.15 HashJoin(store_sales household_demographics time_dim) rows=25261 cost=2145.73..692847.55 clauses: store_sales.ss_hdemo_sk = household_demographics.hd_demo_sk HashJoin(store_sales time_dim) rows=252609 cost=1989.73..692028.08 clauses: store_sales.ss_sold_time_sk = time_dim.t_time_sk SeqScan(store_sales) rows=11998564 cost=0.00..658540.64 SeqScan(time_dim) rows=1070 cost=0.00..1976.35 SeqScan(household_demographics) rows=720 cost=0.00..147.00 prepare_sort_from_pathkeys fails to find a pathkey because tlist_member_ignore_relabel returns null -- which seemed weird because the sortexpr is an Aggref (in a single member equivalence class) and the tlist contains a single member that's also an Aggref. It turns out that the only difference between the two Aggrefs is that the tlist entry has "aggsplit = AGGSPLIT_INITIAL_SERIAL" while the sortexpr has aggsplit = AGGSPLIT_SIMPLE. That's as far as I've gotten so far, but I figured I'd get that info out to see if it means anything obvious to anyone else. James
On Mon, Apr 12, 2021 at 8:37 AM Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > Could be related to incremental sort, which allowed some gather merge > paths that were impossible before. We had a couple issues related to > that fixed in November, IIRC. Hmm, could be. Although, the stack trace at issue doesn't seem to show a call to create_incrementalsort_plan(). -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Apr 14, 2021 at 8:16 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Apr 12, 2021 at 8:37 AM Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: > > Could be related to incremental sort, which allowed some gather merge > > paths that were impossible before. We had a couple issues related to > > that fixed in November, IIRC. > > Hmm, could be. Although, the stack trace at issue doesn't seem to show > a call to create_incrementalsort_plan(). The changes to gather merge path generation made it possible to use those paths in more cases for both incremental sort and regular sort, so by "incremental sort" I read Tomas as saying "the patches that brought in incremental sort" not specifically "incremental sort itself". James
On Wed, Apr 14, 2021 at 5:43 PM James Coleman <jtc331@gmail.com> wrote: > The query in question is: > select count(*) > from store_sales > ,household_demographics > ,time_dim, store > where ss_sold_time_sk = time_dim.t_time_sk > and ss_hdemo_sk = household_demographics.hd_demo_sk > and ss_store_sk = s_store_sk > and time_dim.t_hour = 15 > and time_dim.t_minute >= 30 > and household_demographics.hd_dep_count = 7 > and store.s_store_name = 'ese' > order by count(*) > limit 100; > > From debugging output it looks like this is the plan being chosen > (cheapest total path): > Gather(store_sales household_demographics time_dim) rows=60626 > cost=3145.73..699910.15 > HashJoin(store_sales household_demographics time_dim) > rows=25261 cost=2145.73..692847.55 > clauses: store_sales.ss_hdemo_sk = > household_demographics.hd_demo_sk > HashJoin(store_sales time_dim) rows=252609 > cost=1989.73..692028.08 > clauses: store_sales.ss_sold_time_sk = > time_dim.t_time_sk > SeqScan(store_sales) rows=11998564 > cost=0.00..658540.64 > SeqScan(time_dim) rows=1070 > cost=0.00..1976.35 > SeqScan(household_demographics) rows=720 > cost=0.00..147.00 This doesn't really make sense to me given the strack trace in the OP. That seems to go Limit -> Sort -> Agg -> NestLoop -> NestLoop -> NestLoop -> GatherMerge -> Sort. If the plan were as you have it here, there would be no Sort and no Gather Merge, so where would be getting a failure related to pathkeys? I think if we can get the correct plan the thing to look at would be the tlists at the relevant levels of the plan. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Apr 14, 2021 at 8:20 PM James Coleman <jtc331@gmail.com> wrote: > > Hmm, could be. Although, the stack trace at issue doesn't seem to show > > a call to create_incrementalsort_plan(). > > The changes to gather merge path generation made it possible to use > those paths in more cases for both incremental sort and regular sort, > so by "incremental sort" I read Tomas as saying "the patches that > brought in incremental sort" not specifically "incremental sort > itself". I agree. That's why I said "hmm, could be" even though the plan doesn't involve one. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Apr 14, 2021 at 8:21 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Apr 14, 2021 at 5:43 PM James Coleman <jtc331@gmail.com> wrote:
> > The query in question is:
> > select count(*)
> > from store_sales
> > ,household_demographics
> > ,time_dim, store
> > where ss_sold_time_sk = time_dim.t_time_sk
> > and ss_hdemo_sk = household_demographics.hd_demo_sk
> > and ss_store_sk = s_store_sk
> > and time_dim.t_hour = 15
> > and time_dim.t_minute >= 30
> > and household_demographics.hd_dep_count = 7
> > and store.s_store_name = 'ese'
> > order by count(*)
> > limit 100;
> >
> > From debugging output it looks like this is the plan being chosen
> > (cheapest total path):
> > Gather(store_sales household_demographics time_dim) rows=60626
> > cost=3145.73..699910.15
> > HashJoin(store_sales household_demographics time_dim)
> > rows=25261 cost=2145.73..692847.55
> > clauses: store_sales.ss_hdemo_sk =
> > household_demographics.hd_demo_sk
> > HashJoin(store_sales time_dim) rows=252609
> > cost=1989.73..692028.08
> > clauses: store_sales.ss_sold_time_sk =
> > time_dim.t_time_sk
> > SeqScan(store_sales) rows=11998564
> > cost=0.00..658540.64
> > SeqScan(time_dim) rows=1070
> > cost=0.00..1976.35
> > SeqScan(household_demographics) rows=720
> > cost=0.00..147.00
>
> This doesn't really make sense to me given the strack trace in the OP.
> That seems to go Limit -> Sort -> Agg -> NestLoop -> NestLoop ->
> NestLoop -> GatherMerge -> Sort. If the plan were as you have it here,
> there would be no Sort and no Gather Merge, so where would be getting
> a failure related to pathkeys?
Ah, yeah, I'm not sure where the original stacktrace came from, but
here's the stack for the query I reproduced it with (perhaps it does
so on different queries or there was some other GUC change in the
reported plan):
#0 errfinish (filename=filename@entry=0x56416eefa845 "createplan.c",
lineno=lineno@entry=6186,
funcname=funcname@entry=0x56416eefb660 <__func__.24872>
"prepare_sort_from_pathkeys") at elog.c:514
#1 0x000056416eb6ed52 in prepare_sort_from_pathkeys
(lefttree=0x564170552658, pathkeys=0x5641704f2640, relids=0x0,
reqColIdx=reqColIdx@entry=0x0,
adjust_tlist_in_place=adjust_tlist_in_place@entry=false,
p_numsortkeys=p_numsortkeys@entry=0x7fff1252817c,
p_sortColIdx=0x7fff12528170,
p_sortOperators=0x7fff12528168, p_collations=0x7fff12528160,
p_nullsFirst=0x7fff12528158) at createplan.c:6186
#2 0x000056416eb6ee69 in make_sort_from_pathkeys (lefttree=<optimized
out>, pathkeys=<optimized out>, relids=<optimized out>) at
createplan.c:6313
#3 0x000056416eb71fc7 in create_sort_plan
(root=root@entry=0x564170511a70,
best_path=best_path@entry=0x56417054f650, flags=flags@entry=1)
at createplan.c:2118
#4 0x000056416eb6f638 in create_plan_recurse
(root=root@entry=0x564170511a70, best_path=0x56417054f650,
flags=flags@entry=1) at createplan.c:489
#5 0x000056416eb72e06 in create_gather_merge_plan
(root=root@entry=0x564170511a70,
best_path=best_path@entry=0x56417054f6e8) at createplan.c:1885
#6 0x000056416eb6f723 in create_plan_recurse
(root=root@entry=0x564170511a70, best_path=0x56417054f6e8,
flags=flags@entry=4) at createplan.c:541
#7 0x000056416eb726fb in create_agg_plan
(root=root@entry=0x564170511a70,
best_path=best_path@entry=0x56417054f8c8) at createplan.c:2238
#8 0x000056416eb6f67b in create_plan_recurse
(root=root@entry=0x564170511a70, best_path=0x56417054f8c8,
flags=flags@entry=3) at createplan.c:509
#9 0x000056416eb71f8e in create_sort_plan
(root=root@entry=0x564170511a70,
best_path=best_path@entry=0x56417054f560, flags=flags@entry=1)
at createplan.c:2109
#10 0x000056416eb6f638 in create_plan_recurse
(root=root@entry=0x564170511a70, best_path=0x56417054f560,
flags=flags@entry=1) at createplan.c:489
#11 0x000056416eb72c83 in create_limit_plan
(root=root@entry=0x564170511a70,
best_path=best_path@entry=0x56417054ffa0, flags=flags@entry=1)
at createplan.c:2784
#12 0x000056416eb6f713 in create_plan_recurse
(root=root@entry=0x564170511a70, best_path=0x56417054ffa0,
flags=flags@entry=1) at createplan.c:536
#13 0x000056416eb6f79d in create_plan (root=root@entry=0x564170511a70,
best_path=<optimized out>) at createplan.c:349
#14 0x000056416eb7fe93 in standard_planner (parse=0x564170437268,
query_string=<optimized out>, cursorOptions=2048,
boundParams=<optimized out>)
at planner.c:407
> I think if we can get the correct plan the thing to look at would be
> the tlists at the relevant levels of the plan.
Does the information in [1] help at all? The tlist does have an
Aggref, as expected, but its aggsplit value doesn't match the
pathkey's Aggref's aggsplit value.
James
1: https://www.postgresql.org/message-id/CAAaqYe_NU4hO9COoJdcXWqjtH%3DdGMknYdsSdJjZ%3DJOHPTea-Nw%40mail.gmail.com
On Wed, Apr 14, 2021 at 8:21 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Apr 14, 2021 at 5:43 PM James Coleman <jtc331@gmail.com> wrote: > > The query in question is: > > select count(*) > > from store_sales > > ,household_demographics > > ,time_dim, store > > where ss_sold_time_sk = time_dim.t_time_sk > > and ss_hdemo_sk = household_demographics.hd_demo_sk > > and ss_store_sk = s_store_sk > > and time_dim.t_hour = 15 > > and time_dim.t_minute >= 30 > > and household_demographics.hd_dep_count = 7 > > and store.s_store_name = 'ese' > > order by count(*) > > limit 100; > > > > From debugging output it looks like this is the plan being chosen > > (cheapest total path): > > Gather(store_sales household_demographics time_dim) rows=60626 > > cost=3145.73..699910.15 > > HashJoin(store_sales household_demographics time_dim) > > rows=25261 cost=2145.73..692847.55 > > clauses: store_sales.ss_hdemo_sk = > > household_demographics.hd_demo_sk > > HashJoin(store_sales time_dim) rows=252609 > > cost=1989.73..692028.08 > > clauses: store_sales.ss_sold_time_sk = > > time_dim.t_time_sk > > SeqScan(store_sales) rows=11998564 > > cost=0.00..658540.64 > > SeqScan(time_dim) rows=1070 > > cost=0.00..1976.35 > > SeqScan(household_demographics) rows=720 > > cost=0.00..147.00 > > This doesn't really make sense to me given the strack trace in the OP. > That seems to go Limit -> Sort -> Agg -> NestLoop -> NestLoop -> > NestLoop -> GatherMerge -> Sort. If the plan were as you have it here, > there would be no Sort and no Gather Merge, so where would be getting > a failure related to pathkeys? Also I just realized why this didn't make sense -- I'm not sure what the above path is. It'd gotten logged as part of the debug options I have configured, but it must be 1.) incomplete (perhaps at a lower level of path generation) and/or not the final path selected. My apologies for the confusion. James
On Wed, Apr 14, 2021 at 5:42 PM James Coleman <jtc331@gmail.com> wrote: > > On Mon, Apr 12, 2021 at 8:37 AM Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: > > > > On 4/12/21 2:24 PM, Luc Vlaming wrote: > > > Hi, > > > > > > When trying to run on master (but afaik also PG-13) TPC-DS queries 94, > > > 95 and 96 on a SF10 I get the error "could not find pathkey item to sort". > > > When I disable enable_gathermerge the problem goes away and then the > > > plan for query 94 looks like below. I tried figuring out what the > > > problem is but to be honest I would need some pointers as the code that > > > tries to matching equivalence members in prepare_sort_from_pathkeys is > > > something i'm really not familiar with. > > > > > > > Could be related to incremental sort, which allowed some gather merge > > paths that were impossible before. We had a couple issues related to > > that fixed in November, IIRC. > > > > > To reproduce you can either ingest and test using the toolkit I used too > > > (see https://github.com/swarm64/s64da-benchmark-toolkit/), or > > > alternatively just use the schema (see > > > https://github.com/swarm64/s64da-benchmark-toolkit/tree/master/benchmarks/tpcds/schemas/psql_native) > > > > > > > Thanks, I'll see if I can reproduce that with your schema. > > > > > > regards > > > > -- > > Tomas Vondra > > EnterpriseDB: http://www.enterprisedb.com > > The Enterprise PostgreSQL Company > > The query in question is: > > select count(*) > from store_sales > ,household_demographics > ,time_dim, store > where ss_sold_time_sk = time_dim.t_time_sk > and ss_hdemo_sk = household_demographics.hd_demo_sk > and ss_store_sk = s_store_sk > and time_dim.t_hour = 15 > and time_dim.t_minute >= 30 > and household_demographics.hd_dep_count = 7 > and store.s_store_name = 'ese' > order by count(*) > limit 100; > > From debugging output it looks like this is the plan being chosen > (cheapest total path): > Gather(store_sales household_demographics time_dim) rows=60626 > cost=3145.73..699910.15 > HashJoin(store_sales household_demographics time_dim) > rows=25261 cost=2145.73..692847.55 > clauses: store_sales.ss_hdemo_sk = > household_demographics.hd_demo_sk > HashJoin(store_sales time_dim) rows=252609 > cost=1989.73..692028.08 > clauses: store_sales.ss_sold_time_sk = > time_dim.t_time_sk > SeqScan(store_sales) rows=11998564 > cost=0.00..658540.64 > SeqScan(time_dim) rows=1070 > cost=0.00..1976.35 > SeqScan(household_demographics) rows=720 > cost=0.00..147.00 > > prepare_sort_from_pathkeys fails to find a pathkey because > tlist_member_ignore_relabel returns null -- which seemed weird because > the sortexpr is an Aggref (in a single member equivalence class) and > the tlist contains a single member that's also an Aggref. It turns out > that the only difference between the two Aggrefs is that the tlist > entry has "aggsplit = AGGSPLIT_INITIAL_SERIAL" while the sortexpr has > aggsplit = AGGSPLIT_SIMPLE. > > That's as far as I've gotten so far, but I figured I'd get that info > out to see if it means anything obvious to anyone else. This really goes back to [1] where we fixed a similar issue by making find_em_expr_usable_for_sorting_rel parallel the rules in prepare_sort_from_pathkeys. Most of those conditions got copied, and the case we were trying to handle is the fact that prepare_sort_from_pathkeys can generate a target list entry under those conditions if one doesn't exist. However there's a further restriction there I don't remember looking at: it uses pull_var_clause and tlist_member_ignore_relabel to ensure that all of the vars that feed into the sort expression are found in the target list. As I understand it, that is: it will build a target list entry for something like "md5(column)" if "column" (and that was one of our test cases for the previous fix) is in the target list already. But there's an additional detail here: the call to pull_var_clause requests aggregates, window functions, and placeholders be treated as vars. That means for our Aggref case it would require that the two Aggrefs be fully equal, so the differing aggsplit member would cause a target list entry not to be built, hence our error here. I've attached a quick and dirty patch that encodes that final rule from prepare_sort_from_pathkeys into find_em_expr_usable_for_sorting_rel. I can't help but think that there's a cleaner way to do with this with less code duplication, but hindering that is that prepare_sort_from_pathkeys is working with a TargetList while find_em_expr_usable_for_sorting_rel is working with a list of expressions. James 1: https://www.postgresql.org/message-id/CAAaqYe9C3f6A_tZCRfr9Dm7hPpgGwpp4i-K_%3DNS9GWXuNiFANg%40mail.gmail.com
Вложения
On 15-04-2021 04:01, James Coleman wrote: > On Wed, Apr 14, 2021 at 5:42 PM James Coleman <jtc331@gmail.com> wrote: >> >> On Mon, Apr 12, 2021 at 8:37 AM Tomas Vondra >> <tomas.vondra@enterprisedb.com> wrote: >>> >>> On 4/12/21 2:24 PM, Luc Vlaming wrote: >>>> Hi, >>>> >>>> When trying to run on master (but afaik also PG-13) TPC-DS queries 94, >>>> 95 and 96 on a SF10 I get the error "could not find pathkey item to sort". >>>> When I disable enable_gathermerge the problem goes away and then the >>>> plan for query 94 looks like below. I tried figuring out what the >>>> problem is but to be honest I would need some pointers as the code that >>>> tries to matching equivalence members in prepare_sort_from_pathkeys is >>>> something i'm really not familiar with. >>>> >>> >>> Could be related to incremental sort, which allowed some gather merge >>> paths that were impossible before. We had a couple issues related to >>> that fixed in November, IIRC. >>> >>>> To reproduce you can either ingest and test using the toolkit I used too >>>> (see https://github.com/swarm64/s64da-benchmark-toolkit/), or >>>> alternatively just use the schema (see >>>> https://github.com/swarm64/s64da-benchmark-toolkit/tree/master/benchmarks/tpcds/schemas/psql_native) >>>> >>> >>> Thanks, I'll see if I can reproduce that with your schema. >>> >>> >>> regards >>> >>> -- >>> Tomas Vondra >>> EnterpriseDB: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >> >> The query in question is: >> >> select count(*) >> from store_sales >> ,household_demographics >> ,time_dim, store >> where ss_sold_time_sk = time_dim.t_time_sk >> and ss_hdemo_sk = household_demographics.hd_demo_sk >> and ss_store_sk = s_store_sk >> and time_dim.t_hour = 15 >> and time_dim.t_minute >= 30 >> and household_demographics.hd_dep_count = 7 >> and store.s_store_name = 'ese' >> order by count(*) >> limit 100; >> >> From debugging output it looks like this is the plan being chosen >> (cheapest total path): >> Gather(store_sales household_demographics time_dim) rows=60626 >> cost=3145.73..699910.15 >> HashJoin(store_sales household_demographics time_dim) >> rows=25261 cost=2145.73..692847.55 >> clauses: store_sales.ss_hdemo_sk = >> household_demographics.hd_demo_sk >> HashJoin(store_sales time_dim) rows=252609 >> cost=1989.73..692028.08 >> clauses: store_sales.ss_sold_time_sk = >> time_dim.t_time_sk >> SeqScan(store_sales) rows=11998564 >> cost=0.00..658540.64 >> SeqScan(time_dim) rows=1070 >> cost=0.00..1976.35 >> SeqScan(household_demographics) rows=720 >> cost=0.00..147.00 >> >> prepare_sort_from_pathkeys fails to find a pathkey because >> tlist_member_ignore_relabel returns null -- which seemed weird because >> the sortexpr is an Aggref (in a single member equivalence class) and >> the tlist contains a single member that's also an Aggref. It turns out >> that the only difference between the two Aggrefs is that the tlist >> entry has "aggsplit = AGGSPLIT_INITIAL_SERIAL" while the sortexpr has >> aggsplit = AGGSPLIT_SIMPLE. >> >> That's as far as I've gotten so far, but I figured I'd get that info >> out to see if it means anything obvious to anyone else. > > This really goes back to [1] where we fixed a similar issue by making > find_em_expr_usable_for_sorting_rel parallel the rules in > prepare_sort_from_pathkeys. > > Most of those conditions got copied, and the case we were trying to > handle is the fact that prepare_sort_from_pathkeys can generate a > target list entry under those conditions if one doesn't exist. However > there's a further restriction there I don't remember looking at: it > uses pull_var_clause and tlist_member_ignore_relabel to ensure that > all of the vars that feed into the sort expression are found in the > target list. As I understand it, that is: it will build a target list > entry for something like "md5(column)" if "column" (and that was one > of our test cases for the previous fix) is in the target list already. > > But there's an additional detail here: the call to pull_var_clause > requests aggregates, window functions, and placeholders be treated as > vars. That means for our Aggref case it would require that the two > Aggrefs be fully equal, so the differing aggsplit member would cause a > target list entry not to be built, hence our error here. > > I've attached a quick and dirty patch that encodes that final rule > from prepare_sort_from_pathkeys into > find_em_expr_usable_for_sorting_rel. I can't help but think that > there's a cleaner way to do with this with less code duplication, but > hindering that is that prepare_sort_from_pathkeys is working with a > TargetList while find_em_expr_usable_for_sorting_rel is working with a > list of expressions. > > James > > 1: https://www.postgresql.org/message-id/CAAaqYe9C3f6A_tZCRfr9Dm7hPpgGwpp4i-K_%3DNS9GWXuNiFANg%40mail.gmail.com > Hi, The patch seems to make the planner proceed and not error out anymore. Cannot judge if it's doing the right thing however or if its enough :) It works for me for all reported queries however (queries 94-96). And sorry for the confusion wrt the stacktrace and plan. I tried to produce a plan to possibly help with debugging but that would ofc then not have the problem of the missing sortkey as otherwise i cannot present a plan :) The stacktrace was however correct, and the plan considered involved a gather-merge with a sort. Unfortunately I could not (easily) get the plan outputted in the end; even when setting the costs to 0 somehow... Regards, Luc
On Thu, Apr 15, 2021 at 5:33 AM Luc Vlaming <luc@swarm64.com> wrote: > > On 15-04-2021 04:01, James Coleman wrote: > > On Wed, Apr 14, 2021 at 5:42 PM James Coleman <jtc331@gmail.com> wrote: > >> > >> On Mon, Apr 12, 2021 at 8:37 AM Tomas Vondra > >> <tomas.vondra@enterprisedb.com> wrote: > >>> > >>> On 4/12/21 2:24 PM, Luc Vlaming wrote: > >>>> Hi, > >>>> > >>>> When trying to run on master (but afaik also PG-13) TPC-DS queries 94, > >>>> 95 and 96 on a SF10 I get the error "could not find pathkey item to sort". > >>>> When I disable enable_gathermerge the problem goes away and then the > >>>> plan for query 94 looks like below. I tried figuring out what the > >>>> problem is but to be honest I would need some pointers as the code that > >>>> tries to matching equivalence members in prepare_sort_from_pathkeys is > >>>> something i'm really not familiar with. > >>>> > >>> > >>> Could be related to incremental sort, which allowed some gather merge > >>> paths that were impossible before. We had a couple issues related to > >>> that fixed in November, IIRC. > >>> > >>>> To reproduce you can either ingest and test using the toolkit I used too > >>>> (see https://github.com/swarm64/s64da-benchmark-toolkit/), or > >>>> alternatively just use the schema (see > >>>> https://github.com/swarm64/s64da-benchmark-toolkit/tree/master/benchmarks/tpcds/schemas/psql_native) > >>>> > >>> > >>> Thanks, I'll see if I can reproduce that with your schema. > >>> > >>> > >>> regards > >>> > >>> -- > >>> Tomas Vondra > >>> EnterpriseDB: http://www.enterprisedb.com > >>> The Enterprise PostgreSQL Company > >> > >> The query in question is: > >> > >> select count(*) > >> from store_sales > >> ,household_demographics > >> ,time_dim, store > >> where ss_sold_time_sk = time_dim.t_time_sk > >> and ss_hdemo_sk = household_demographics.hd_demo_sk > >> and ss_store_sk = s_store_sk > >> and time_dim.t_hour = 15 > >> and time_dim.t_minute >= 30 > >> and household_demographics.hd_dep_count = 7 > >> and store.s_store_name = 'ese' > >> order by count(*) > >> limit 100; > >> > >> From debugging output it looks like this is the plan being chosen > >> (cheapest total path): > >> Gather(store_sales household_demographics time_dim) rows=60626 > >> cost=3145.73..699910.15 > >> HashJoin(store_sales household_demographics time_dim) > >> rows=25261 cost=2145.73..692847.55 > >> clauses: store_sales.ss_hdemo_sk = > >> household_demographics.hd_demo_sk > >> HashJoin(store_sales time_dim) rows=252609 > >> cost=1989.73..692028.08 > >> clauses: store_sales.ss_sold_time_sk = > >> time_dim.t_time_sk > >> SeqScan(store_sales) rows=11998564 > >> cost=0.00..658540.64 > >> SeqScan(time_dim) rows=1070 > >> cost=0.00..1976.35 > >> SeqScan(household_demographics) rows=720 > >> cost=0.00..147.00 > >> > >> prepare_sort_from_pathkeys fails to find a pathkey because > >> tlist_member_ignore_relabel returns null -- which seemed weird because > >> the sortexpr is an Aggref (in a single member equivalence class) and > >> the tlist contains a single member that's also an Aggref. It turns out > >> that the only difference between the two Aggrefs is that the tlist > >> entry has "aggsplit = AGGSPLIT_INITIAL_SERIAL" while the sortexpr has > >> aggsplit = AGGSPLIT_SIMPLE. > >> > >> That's as far as I've gotten so far, but I figured I'd get that info > >> out to see if it means anything obvious to anyone else. > > > > This really goes back to [1] where we fixed a similar issue by making > > find_em_expr_usable_for_sorting_rel parallel the rules in > > prepare_sort_from_pathkeys. > > > > Most of those conditions got copied, and the case we were trying to > > handle is the fact that prepare_sort_from_pathkeys can generate a > > target list entry under those conditions if one doesn't exist. However > > there's a further restriction there I don't remember looking at: it > > uses pull_var_clause and tlist_member_ignore_relabel to ensure that > > all of the vars that feed into the sort expression are found in the > > target list. As I understand it, that is: it will build a target list > > entry for something like "md5(column)" if "column" (and that was one > > of our test cases for the previous fix) is in the target list already. > > > > But there's an additional detail here: the call to pull_var_clause > > requests aggregates, window functions, and placeholders be treated as > > vars. That means for our Aggref case it would require that the two > > Aggrefs be fully equal, so the differing aggsplit member would cause a > > target list entry not to be built, hence our error here. > > > > I've attached a quick and dirty patch that encodes that final rule > > from prepare_sort_from_pathkeys into > > find_em_expr_usable_for_sorting_rel. I can't help but think that > > there's a cleaner way to do with this with less code duplication, but > > hindering that is that prepare_sort_from_pathkeys is working with a > > TargetList while find_em_expr_usable_for_sorting_rel is working with a > > list of expressions. > > > > James > > > > 1: https://www.postgresql.org/message-id/CAAaqYe9C3f6A_tZCRfr9Dm7hPpgGwpp4i-K_%3DNS9GWXuNiFANg%40mail.gmail.com > > > > Hi, > > The patch seems to make the planner proceed and not error out anymore. > Cannot judge if it's doing the right thing however or if its enough :) > It works for me for all reported queries however (queries 94-96). > > And sorry for the confusion wrt the stacktrace and plan. I tried to > produce a plan to possibly help with debugging but that would ofc then > not have the problem of the missing sortkey as otherwise i cannot > present a plan :) The stacktrace was however correct, and the plan > considered involved a gather-merge with a sort. Unfortunately I could > not (easily) get the plan outputted in the end; even when setting the > costs to 0 somehow... > > Regards, > Luc Same patch, but with a test case now. James
Вложения
On 4/15/21 2:21 AM, Robert Haas wrote: > On Wed, Apr 14, 2021 at 8:20 PM James Coleman <jtc331@gmail.com> wrote: >>> Hmm, could be. Although, the stack trace at issue doesn't seem to show >>> a call to create_incrementalsort_plan(). >> >> The changes to gather merge path generation made it possible to use >> those paths in more cases for both incremental sort and regular sort, >> so by "incremental sort" I read Tomas as saying "the patches that >> brought in incremental sort" not specifically "incremental sort >> itself". > > I agree. That's why I said "hmm, could be" even though the plan > doesn't involve one. > Yeah, that's what I meant. The difference to pre-13 behavior is that we now call generate_useful_gather_paths, which also considers adding extra sort (unlike plain generate_gather_paths). So now we can end up with "Gather Merge -> Sort" paths that would not be considered before. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 4/15/21 7:35 PM, James Coleman wrote: > On Thu, Apr 15, 2021 at 5:33 AM Luc Vlaming <luc@swarm64.com> wrote: >> >> On 15-04-2021 04:01, James Coleman wrote: >>> On Wed, Apr 14, 2021 at 5:42 PM James Coleman <jtc331@gmail.com> wrote: >>>> >>>> On Mon, Apr 12, 2021 at 8:37 AM Tomas Vondra >>>> <tomas.vondra@enterprisedb.com> wrote: >>>>> >>>>> On 4/12/21 2:24 PM, Luc Vlaming wrote: >>>>>> Hi, >>>>>> >>>>>> When trying to run on master (but afaik also PG-13) TPC-DS queries 94, >>>>>> 95 and 96 on a SF10 I get the error "could not find pathkey item to sort". >>>>>> When I disable enable_gathermerge the problem goes away and then the >>>>>> plan for query 94 looks like below. I tried figuring out what the >>>>>> problem is but to be honest I would need some pointers as the code that >>>>>> tries to matching equivalence members in prepare_sort_from_pathkeys is >>>>>> something i'm really not familiar with. >>>>>> >>>>> >>>>> Could be related to incremental sort, which allowed some gather merge >>>>> paths that were impossible before. We had a couple issues related to >>>>> that fixed in November, IIRC. >>>>> >>>>>> To reproduce you can either ingest and test using the toolkit I used too >>>>>> (see https://github.com/swarm64/s64da-benchmark-toolkit/), or >>>>>> alternatively just use the schema (see >>>>>> https://github.com/swarm64/s64da-benchmark-toolkit/tree/master/benchmarks/tpcds/schemas/psql_native) >>>>>> >>>>> >>>>> Thanks, I'll see if I can reproduce that with your schema. >>>>> >>>>> >>>>> regards >>>>> >>>>> -- >>>>> Tomas Vondra >>>>> EnterpriseDB: http://www.enterprisedb.com >>>>> The Enterprise PostgreSQL Company >>>> >>>> The query in question is: >>>> >>>> select count(*) >>>> from store_sales >>>> ,household_demographics >>>> ,time_dim, store >>>> where ss_sold_time_sk = time_dim.t_time_sk >>>> and ss_hdemo_sk = household_demographics.hd_demo_sk >>>> and ss_store_sk = s_store_sk >>>> and time_dim.t_hour = 15 >>>> and time_dim.t_minute >= 30 >>>> and household_demographics.hd_dep_count = 7 >>>> and store.s_store_name = 'ese' >>>> order by count(*) >>>> limit 100; >>>> >>>> From debugging output it looks like this is the plan being chosen >>>> (cheapest total path): >>>> Gather(store_sales household_demographics time_dim) rows=60626 >>>> cost=3145.73..699910.15 >>>> HashJoin(store_sales household_demographics time_dim) >>>> rows=25261 cost=2145.73..692847.55 >>>> clauses: store_sales.ss_hdemo_sk = >>>> household_demographics.hd_demo_sk >>>> HashJoin(store_sales time_dim) rows=252609 >>>> cost=1989.73..692028.08 >>>> clauses: store_sales.ss_sold_time_sk = >>>> time_dim.t_time_sk >>>> SeqScan(store_sales) rows=11998564 >>>> cost=0.00..658540.64 >>>> SeqScan(time_dim) rows=1070 >>>> cost=0.00..1976.35 >>>> SeqScan(household_demographics) rows=720 >>>> cost=0.00..147.00 >>>> >>>> prepare_sort_from_pathkeys fails to find a pathkey because >>>> tlist_member_ignore_relabel returns null -- which seemed weird because >>>> the sortexpr is an Aggref (in a single member equivalence class) and >>>> the tlist contains a single member that's also an Aggref. It turns out >>>> that the only difference between the two Aggrefs is that the tlist >>>> entry has "aggsplit = AGGSPLIT_INITIAL_SERIAL" while the sortexpr has >>>> aggsplit = AGGSPLIT_SIMPLE. >>>> >>>> That's as far as I've gotten so far, but I figured I'd get that info >>>> out to see if it means anything obvious to anyone else. >>> >>> This really goes back to [1] where we fixed a similar issue by making >>> find_em_expr_usable_for_sorting_rel parallel the rules in >>> prepare_sort_from_pathkeys. >>> >>> Most of those conditions got copied, and the case we were trying to >>> handle is the fact that prepare_sort_from_pathkeys can generate a >>> target list entry under those conditions if one doesn't exist. However >>> there's a further restriction there I don't remember looking at: it >>> uses pull_var_clause and tlist_member_ignore_relabel to ensure that >>> all of the vars that feed into the sort expression are found in the >>> target list. As I understand it, that is: it will build a target list >>> entry for something like "md5(column)" if "column" (and that was one >>> of our test cases for the previous fix) is in the target list already. >>> >>> But there's an additional detail here: the call to pull_var_clause >>> requests aggregates, window functions, and placeholders be treated as >>> vars. That means for our Aggref case it would require that the two >>> Aggrefs be fully equal, so the differing aggsplit member would cause a >>> target list entry not to be built, hence our error here. >>> >>> I've attached a quick and dirty patch that encodes that final rule >>> from prepare_sort_from_pathkeys into >>> find_em_expr_usable_for_sorting_rel. I can't help but think that >>> there's a cleaner way to do with this with less code duplication, but >>> hindering that is that prepare_sort_from_pathkeys is working with a >>> TargetList while find_em_expr_usable_for_sorting_rel is working with a >>> list of expressions. >>> Yeah, I think it'll be difficult to reuse code from later planner stages exactly because it operates on different representation. So something like your patch is likely necessary. As for the patch, I have a couple comments: 1) expr_list_member_ignore_relabel would deserve a better comment, and maybe a reference to tlist_member_ignore_relabel which it copies 2) I suppose the comment before "if (ec->ec_has_volatile)" needs updating, because now it says we're done as long as the expression is not volatile (but we're doing more stuff). 3) Shouldn't find_em_expr_usable_for_sorting_rel now mostly mimic what prepare_sort_from_pathkeys does? That is, try to match the entries directly first, before the new pull_vars() business? 4) I've simplified the foreach() loop a bit. prepare_sort_from_pathkeys does it differently, but that's because there are multiple foreach levels, I think. Yes, we'll not free the list, but I that's what most other places in planner do ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
On Thu, Apr 15, 2021 at 6:27 PM Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
On 4/15/21 7:35 PM, James Coleman wrote:
> On Thu, Apr 15, 2021 at 5:33 AM Luc Vlaming <luc@swarm64.com> wrote:
>>
>> On 15-04-2021 04:01, James Coleman wrote:
>>> On Wed, Apr 14, 2021 at 5:42 PM James Coleman <jtc331@gmail.com> wrote:
>>>>
>>>> On Mon, Apr 12, 2021 at 8:37 AM Tomas Vondra
>>>> <tomas.vondra@enterprisedb.com> wrote:
>>>>>
>>>>> On 4/12/21 2:24 PM, Luc Vlaming wrote:
>>>>>> Hi,
>>>>>>
>>>>>> When trying to run on master (but afaik also PG-13) TPC-DS queries 94,
>>>>>> 95 and 96 on a SF10 I get the error "could not find pathkey item to sort".
>>>>>> When I disable enable_gathermerge the problem goes away and then the
>>>>>> plan for query 94 looks like below. I tried figuring out what the
>>>>>> problem is but to be honest I would need some pointers as the code that
>>>>>> tries to matching equivalence members in prepare_sort_from_pathkeys is
>>>>>> something i'm really not familiar with.
>>>>>>
>>>>>
>>>>> Could be related to incremental sort, which allowed some gather merge
>>>>> paths that were impossible before. We had a couple issues related to
>>>>> that fixed in November, IIRC.
>>>>>
>>>>>> To reproduce you can either ingest and test using the toolkit I used too
>>>>>> (see https://github.com/swarm64/s64da-benchmark-toolkit/), or
>>>>>> alternatively just use the schema (see
>>>>>> https://github.com/swarm64/s64da-benchmark-toolkit/tree/master/benchmarks/tpcds/schemas/psql_native)
>>>>>>
>>>>>
>>>>> Thanks, I'll see if I can reproduce that with your schema.
>>>>>
>>>>>
>>>>> regards
>>>>>
>>>>> --
>>>>> Tomas Vondra
>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>
>>>> The query in question is:
>>>>
>>>> select count(*)
>>>> from store_sales
>>>> ,household_demographics
>>>> ,time_dim, store
>>>> where ss_sold_time_sk = time_dim.t_time_sk
>>>> and ss_hdemo_sk = household_demographics.hd_demo_sk
>>>> and ss_store_sk = s_store_sk
>>>> and time_dim.t_hour = 15
>>>> and time_dim.t_minute >= 30
>>>> and household_demographics.hd_dep_count = 7
>>>> and store.s_store_name = 'ese'
>>>> order by count(*)
>>>> limit 100;
>>>>
>>>> From debugging output it looks like this is the plan being chosen
>>>> (cheapest total path):
>>>> Gather(store_sales household_demographics time_dim) rows=60626
>>>> cost=3145.73..699910.15
>>>> HashJoin(store_sales household_demographics time_dim)
>>>> rows=25261 cost=2145.73..692847.55
>>>> clauses: store_sales.ss_hdemo_sk =
>>>> household_demographics.hd_demo_sk
>>>> HashJoin(store_sales time_dim) rows=252609
>>>> cost=1989.73..692028.08
>>>> clauses: store_sales.ss_sold_time_sk =
>>>> time_dim.t_time_sk
>>>> SeqScan(store_sales) rows=11998564
>>>> cost=0.00..658540.64
>>>> SeqScan(time_dim) rows=1070
>>>> cost=0.00..1976.35
>>>> SeqScan(household_demographics) rows=720
>>>> cost=0.00..147.00
>>>>
>>>> prepare_sort_from_pathkeys fails to find a pathkey because
>>>> tlist_member_ignore_relabel returns null -- which seemed weird because
>>>> the sortexpr is an Aggref (in a single member equivalence class) and
>>>> the tlist contains a single member that's also an Aggref. It turns out
>>>> that the only difference between the two Aggrefs is that the tlist
>>>> entry has "aggsplit = AGGSPLIT_INITIAL_SERIAL" while the sortexpr has
>>>> aggsplit = AGGSPLIT_SIMPLE.
>>>>
>>>> That's as far as I've gotten so far, but I figured I'd get that info
>>>> out to see if it means anything obvious to anyone else.
>>>
>>> This really goes back to [1] where we fixed a similar issue by making
>>> find_em_expr_usable_for_sorting_rel parallel the rules in
>>> prepare_sort_from_pathkeys.
>>>
>>> Most of those conditions got copied, and the case we were trying to
>>> handle is the fact that prepare_sort_from_pathkeys can generate a
>>> target list entry under those conditions if one doesn't exist. However
>>> there's a further restriction there I don't remember looking at: it
>>> uses pull_var_clause and tlist_member_ignore_relabel to ensure that
>>> all of the vars that feed into the sort expression are found in the
>>> target list. As I understand it, that is: it will build a target list
>>> entry for something like "md5(column)" if "column" (and that was one
>>> of our test cases for the previous fix) is in the target list already.
>>>
>>> But there's an additional detail here: the call to pull_var_clause
>>> requests aggregates, window functions, and placeholders be treated as
>>> vars. That means for our Aggref case it would require that the two
>>> Aggrefs be fully equal, so the differing aggsplit member would cause a
>>> target list entry not to be built, hence our error here.
>>>
>>> I've attached a quick and dirty patch that encodes that final rule
>>> from prepare_sort_from_pathkeys into
>>> find_em_expr_usable_for_sorting_rel. I can't help but think that
>>> there's a cleaner way to do with this with less code duplication, but
>>> hindering that is that prepare_sort_from_pathkeys is working with a
>>> TargetList while find_em_expr_usable_for_sorting_rel is working with a
>>> list of expressions.
>>>
Yeah, I think it'll be difficult to reuse code from later planner stages
exactly because it operates on different representation. So something
like your patch is likely necessary.
As for the patch, I have a couple comments:
1) expr_list_member_ignore_relabel would deserve a better comment, and
maybe a reference to tlist_member_ignore_relabel which it copies
2) I suppose the comment before "if (ec->ec_has_volatile)" needs
updating, because now it says we're done as long as the expression is
not volatile (but we're doing more stuff).
3) Shouldn't find_em_expr_usable_for_sorting_rel now mostly mimic what
prepare_sort_from_pathkeys does? That is, try to match the entries
directly first, before the new pull_vars() business?
4) I've simplified the foreach() loop a bit. prepare_sort_from_pathkeys
does it differently, but that's because there are multiple foreach
levels, I think. Yes, we'll not free the list, but I that's what most
other places in planner do ...
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
- break;
+ return NULL;
I think it would be better if list_free(exprvars) is called before the return.
Cheers
[ sorry for not getting to this thread till now ]
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> 3) Shouldn't find_em_expr_usable_for_sorting_rel now mostly mimic what
> prepare_sort_from_pathkeys does? That is, try to match the entries
> directly first, before the new pull_vars() business?
Yeah. I concur that the problem here is that
find_em_expr_usable_for_sorting_rel isn't fully accounting for what
prepare_sort_from_pathkeys can and can't do. However, I don't like this
patch much:
* As written, I think it may just move the pain somewhere else. The point
of the logic in prepare_sort_from_pathkeys is to handle either full
expression matches (e.g. sort by "A+B" when "A+B" is an expression in
the input tlist) or computable expressions (sort by "A+B" when A and B
are individually available). I think you've fixed the second case and
broken the first one. Now it's possible that the case never arises,
and certainly failing to generate an early sort isn't catastrophic anyway.
But we ought to get it right.
* If the goal is to match what prepare_sort_from_pathkeys can do, I
think that doubling down on the strategy of having a duplicate copy
is not the path to a maintainable fix.
I think it's time for some refactoring of this code so that we can
actually share the logic. Accordingly, I propose the attached.
It's really not that hard to share, as long as you accept the idea
that the list passed to the shared subroutine can be either a list of
TargetEntries or of bare expressions.
Also, I don't much care for either the name or API of
find_em_expr_usable_for_sorting_rel. The sole current caller only
really needs a boolean result, and if it did need more than that
it'd likely need the whole EquivalenceMember not just the em_expr
(certainly createplan.c does). So 0002 attached is some bikeshedding
on that. I kept that separate because it might be wise to do it only
in HEAD, just in case somebody out there is calling the function from
an extension.
(BTW, responding to an upthread question: I think the looping to
remove multiple levels of RelabelType is probably now redundant,
but I didn't remove it. If we want to do that there are more
places to touch than just this code.)
regards, tom lane
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 0188c1e9a1..5f7e505950 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -35,6 +35,7 @@
static EquivalenceMember *add_eq_member(EquivalenceClass *ec,
Expr *expr, Relids relids, Relids nullable_relids,
bool is_child, Oid datatype);
+static bool exprlist_member_ignore_relabel(Expr *node, List *exprs);
static void generate_base_implied_equalities_const(PlannerInfo *root,
EquivalenceClass *ec);
static void generate_base_implied_equalities_no_const(PlannerInfo *root,
@@ -769,6 +770,149 @@ get_eclass_for_sort_expr(PlannerInfo *root,
return newec;
}
+/*
+ * find_ec_member_matching_expr
+ * Locate an EquivalenceClass member matching the given expr, if any;
+ * return NULL if no match.
+ *
+ * "Matching" is defined as "equal after stripping RelabelTypes".
+ * This is used for identifying sort expressions, and we need to allow
+ * binary-compatible relabeling for some cases involving binary-compatible
+ * sort operators.
+ *
+ * Child EC members are ignored unless they belong to given 'relids'.
+ */
+EquivalenceMember *
+find_ec_member_matching_expr(EquivalenceClass *ec,
+ Expr *expr,
+ Relids relids)
+{
+ ListCell *lc;
+
+ /* We ignore binary-compatible relabeling on both ends */
+ while (expr && IsA(expr, RelabelType))
+ expr = ((RelabelType *) expr)->arg;
+
+ foreach(lc, ec->ec_members)
+ {
+ EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
+ Expr *emexpr;
+
+ /*
+ * We shouldn't be trying to sort by an equivalence class that
+ * contains a constant, so no need to consider such cases any further.
+ */
+ if (em->em_is_const)
+ continue;
+
+ /*
+ * Ignore child members unless they belong to the requested rel.
+ */
+ if (em->em_is_child &&
+ !bms_is_subset(em->em_relids, relids))
+ continue;
+
+ /* Match if same expression (after stripping relabel) */
+ emexpr = em->em_expr;
+ while (emexpr && IsA(emexpr, RelabelType))
+ emexpr = ((RelabelType *) emexpr)->arg;
+
+ if (equal(emexpr, expr))
+ return em;
+ }
+
+ return NULL;
+}
+
+/*
+ * find_computable_ec_member
+ * Locate an EquivalenceClass member that can be computed from the
+ * expressions appearing in "exprs"; return NULL if no match.
+ *
+ * "exprs" can be either a list of bare expression trees, or a list of
+ * TargetEntry nodes. Either way, it should contain Vars and possibly
+ * Aggrefs and WindowFuncs, which are matched to the corresponding elements
+ * of the EquivalenceClass's expressions. As in find_ec_member_matching_expr,
+ * we ignore binary-compatible relabeling.
+ *
+ * Child EC members are ignored unless they belong to given 'relids'.
+ */
+EquivalenceMember *
+find_computable_ec_member(EquivalenceClass *ec,
+ List *exprs,
+ Relids relids)
+{
+ ListCell *lc;
+
+ foreach(lc, ec->ec_members)
+ {
+ EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
+ List *exprvars;
+ ListCell *lc2;
+
+ /*
+ * We shouldn't be trying to sort by an equivalence class that
+ * contains a constant, so no need to consider such cases any further.
+ */
+ if (em->em_is_const)
+ continue;
+
+ /*
+ * Ignore child members unless they belong to the requested rel.
+ */
+ if (em->em_is_child &&
+ !bms_is_subset(em->em_relids, relids))
+ continue;
+
+ /* Match if all Vars and quasi-Vars are available in exprs */
+ exprvars = pull_var_clause((Node *) em->em_expr,
+ PVC_INCLUDE_AGGREGATES |
+ PVC_INCLUDE_WINDOWFUNCS |
+ PVC_INCLUDE_PLACEHOLDERS);
+ foreach(lc2, exprvars)
+ {
+ if (!exprlist_member_ignore_relabel(lfirst(lc2), exprs))
+ break;
+ }
+ list_free(exprvars);
+ if (!lc2)
+ return em; /* found usable expression */
+ }
+
+ return NULL;
+}
+
+/*
+ * exprlist_member_ignore_relabel
+ * Subroutine for find_computable_ec_member: is "node" in "exprs"?
+ *
+ * Per the requirements of that function, "exprs" might or might not have
+ * TargetEntry superstructure, and we ignore RelabelType.
+ */
+static bool
+exprlist_member_ignore_relabel(Expr *node, List *exprs)
+{
+ ListCell *lc;
+
+ while (node && IsA(node, RelabelType))
+ node = ((RelabelType *) node)->arg;
+
+ foreach(lc, exprs)
+ {
+ Expr *expr = (Expr *) lfirst(lc);
+
+ if (expr && IsA(expr, TargetEntry))
+ expr = ((TargetEntry *) expr)->expr;
+
+ while (expr && IsA(expr, RelabelType))
+ expr = ((RelabelType *) expr)->arg;
+
+ if (equal(node, expr))
+ return true;
+ }
+ return false;
+}
+
/*
* Find an equivalence class member expression, all of whose Vars, come from
* the indicated relation.
@@ -799,71 +943,80 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
}
/*
- * Find an equivalence class member expression that can be safely used to build
- * a sort node using the provided relation. The rules are a subset of those
- * applied in prepare_sort_from_pathkeys since that function deals with sorts
- * that must be delayed until the last stages of query execution, while here
- * we only care about proactive sorts.
+ * Find an equivalence class member expression that can be used to build
+ * a sort node using the provided relation; return NULL if no candidate.
+ *
+ * The selected expression must be one that prepare_sort_from_pathkeys knows
+ * how to deal with, and we also apply a few additional constraints based on
+ * the fact that the desired sort will be done within the scan/join part of
+ * the plan.
*/
Expr *
find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
RelOptInfo *rel, bool require_parallel_safe)
{
- ListCell *lc_em;
+ PathTarget *target = rel->reltarget;
+ EquivalenceMember *em;
+ ListCell *lc;
/*
- * If there is more than one equivalence member matching these
- * requirements we'll be content to choose any one of them.
+ * Reject volatile ECs immediately; such sorts must always be postponed.
*/
- foreach(lc_em, ec->ec_members)
+ if (ec->ec_has_volatile)
+ return NULL;
+
+ /*
+ * Try to find an EM directly matching some reltarget member.
+ */
+ foreach(lc, target->exprs)
{
- EquivalenceMember *em = lfirst(lc_em);
- Expr *em_expr = em->em_expr;
+ Expr *targetexpr = (Expr *) lfirst(lc);
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any further.
- */
- if (em->em_is_const)
+ em = find_ec_member_matching_expr(ec, targetexpr, rel->relids);
+ if (!em)
continue;
/*
- * Any Vars in the equivalence class member need to come from this
- * relation. This is a superset of prepare_sort_from_pathkeys ignoring
- * child members unless they belong to the rel being sorted.
+ * Reject expressions involving set-returning functions, as those
+ * can't be computed early either.
*/
- if (!bms_is_subset(em->em_relids, rel->relids))
+ if (IS_SRF_CALL((Node *) em->em_expr))
continue;
/*
* If requested, reject expressions that are not parallel-safe.
*/
- if (require_parallel_safe && !is_parallel_safe(root, (Node *) em_expr))
- continue;
-
- /*
- * Disallow SRFs so that all of them can be evaluated at the correct
- * time as determined by make_sort_input_target.
- */
- if (IS_SRF_CALL((Node *) em_expr))
+ if (require_parallel_safe && !is_parallel_safe(root,
+ (Node *) em->em_expr))
continue;
- /*
- * As long as the expression isn't volatile then
- * prepare_sort_from_pathkeys is able to generate a new target entry,
- * so there's no need to verify that one already exists.
- *
- * While prepare_sort_from_pathkeys has to be concerned about matching
- * up a volatile expression to the proper tlist entry, it doesn't seem
- * valuable here to expend the work trying to find a match in the
- * target's exprs since such a sort will have to be postponed anyway.
- */
- if (!ec->ec_has_volatile)
- return em->em_expr;
+ return em->em_expr;
}
- /* We didn't find any suitable equivalence class expression */
- return NULL;
+ /*
+ * Try to find a computable expression.
+ */
+ em = find_computable_ec_member(ec, target->exprs, rel->relids);
+ if (!em)
+ return NULL;
+
+ /*
+ * Reject expressions involving set-returning functions, as those can't be
+ * computed early either. (In principle, we could look for another EC
+ * member that isn't a SRF, but it's quite unlikely there'd be one.)
+ */
+ if (IS_SRF_CALL((Node *) em->em_expr))
+ return NULL;
+
+ /*
+ * If requested, reject expressions that are not parallel-safe. (Again,
+ * it doesn't seem worth the trouble to see if there is another option.)
+ */
+ if (require_parallel_safe && !is_parallel_safe(root,
+ (Node *) em->em_expr))
+ return NULL;
+
+ return em->em_expr;
}
/*
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 22f10fa339..77c30e4c0e 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -269,9 +269,6 @@ static Plan *prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
Oid **p_sortOperators,
Oid **p_collations,
bool **p_nullsFirst);
-static EquivalenceMember *find_ec_member_for_tle(EquivalenceClass *ec,
- TargetEntry *tle,
- Relids relids);
static Sort *make_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
Relids relids);
static IncrementalSort *make_incrementalsort_from_pathkeys(Plan *lefttree,
@@ -2110,7 +2107,7 @@ create_sort_plan(PlannerInfo *root, SortPath *best_path, int flags)
flags | CP_SMALL_TLIST);
/*
- * make_sort_from_pathkeys() indirectly calls find_ec_member_for_tle(),
+ * make_sort_from_pathkeys indirectly calls find_ec_member_matching_expr,
* which will ignore any child EC members that don't belong to the given
* relids. Thus, if this sort path is based on a child relation, we must
* pass its relids.
@@ -6017,9 +6014,6 @@ make_incrementalsort(Plan *lefttree, int numCols, int nPresortedCols,
*
* Returns the node which is to be the input to the Sort (either lefttree,
* or a Result stacked atop lefttree).
- *
- * Note: Restrictions on what expressions are safely sortable may also need to
- * be added to find_em_expr_usable_for_sorting_rel.
*/
static Plan *
prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
@@ -6089,7 +6083,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
tle = get_tle_by_resno(tlist, reqColIdx[numsortkeys]);
if (tle)
{
- em = find_ec_member_for_tle(ec, tle, relids);
+ em = find_ec_member_matching_expr(ec, tle->expr, relids);
if (em)
{
/* found expr at right place in tlist */
@@ -6120,7 +6114,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
foreach(j, tlist)
{
tle = (TargetEntry *) lfirst(j);
- em = find_ec_member_for_tle(ec, tle, relids);
+ em = find_ec_member_matching_expr(ec, tle->expr, relids);
if (em)
{
/* found expr already in tlist */
@@ -6134,56 +6128,12 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
if (!tle)
{
/*
- * No matching tlist item; look for a computable expression. Note
- * that we treat Aggrefs as if they were variables; this is
- * necessary when attempting to sort the output from an Agg node
- * for use in a WindowFunc (since grouping_planner will have
- * treated the Aggrefs as variables, too). Likewise, if we find a
- * WindowFunc in a sort expression, treat it as a variable.
+ * No matching tlist item; look for a computable expression.
*/
- Expr *sortexpr = NULL;
-
- foreach(j, ec->ec_members)
- {
- EquivalenceMember *em = (EquivalenceMember *) lfirst(j);
- List *exprvars;
- ListCell *k;
-
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any
- * further.
- */
- if (em->em_is_const)
- continue;
-
- /*
- * Ignore child members unless they belong to the rel being
- * sorted.
- */
- if (em->em_is_child &&
- !bms_is_subset(em->em_relids, relids))
- continue;
-
- sortexpr = em->em_expr;
- exprvars = pull_var_clause((Node *) sortexpr,
- PVC_INCLUDE_AGGREGATES |
- PVC_INCLUDE_WINDOWFUNCS |
- PVC_INCLUDE_PLACEHOLDERS);
- foreach(k, exprvars)
- {
- if (!tlist_member_ignore_relabel(lfirst(k), tlist))
- break;
- }
- list_free(exprvars);
- if (!k)
- {
- pk_datatype = em->em_datatype;
- break; /* found usable expression */
- }
- }
- if (!j)
+ em = find_computable_ec_member(ec, tlist, relids);
+ if (!em)
elog(ERROR, "could not find pathkey item to sort");
+ pk_datatype = em->em_datatype;
/*
* Do we need to insert a Result node?
@@ -6203,7 +6153,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
/*
* Add resjunk entry to input's tlist
*/
- tle = makeTargetEntry(sortexpr,
+ tle = makeTargetEntry(copyObject(em->em_expr),
list_length(tlist) + 1,
NULL,
true);
@@ -6242,56 +6192,6 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
return lefttree;
}
-/*
- * find_ec_member_for_tle
- * Locate an EquivalenceClass member matching the given TLE, if any
- *
- * Child EC members are ignored unless they belong to given 'relids'.
- */
-static EquivalenceMember *
-find_ec_member_for_tle(EquivalenceClass *ec,
- TargetEntry *tle,
- Relids relids)
-{
- Expr *tlexpr;
- ListCell *lc;
-
- /* We ignore binary-compatible relabeling on both ends */
- tlexpr = tle->expr;
- while (tlexpr && IsA(tlexpr, RelabelType))
- tlexpr = ((RelabelType *) tlexpr)->arg;
-
- foreach(lc, ec->ec_members)
- {
- EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
- Expr *emexpr;
-
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any further.
- */
- if (em->em_is_const)
- continue;
-
- /*
- * Ignore child members unless they belong to the rel being sorted.
- */
- if (em->em_is_child &&
- !bms_is_subset(em->em_relids, relids))
- continue;
-
- /* Match if same expression (after stripping relabel) */
- emexpr = em->em_expr;
- while (emexpr && IsA(emexpr, RelabelType))
- emexpr = ((RelabelType *) emexpr)->arg;
-
- if (equal(emexpr, tlexpr))
- return em;
- }
-
- return NULL;
-}
-
/*
* make_sort_from_pathkeys
* Create sort plan to sort according to given pathkeys
@@ -6753,7 +6653,7 @@ make_unique_from_pathkeys(Plan *lefttree, List *pathkeys, int numCols)
foreach(j, plan->targetlist)
{
tle = (TargetEntry *) lfirst(j);
- em = find_ec_member_for_tle(ec, tle, NULL);
+ em = find_ec_member_matching_expr(ec, tle->expr, NULL);
if (em)
{
/* found expr already in tlist */
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 8a26288070..311579d059 100644
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/tlist.c
@@ -79,34 +79,6 @@ tlist_member(Expr *node, List *targetlist)
return NULL;
}
-/*
- * tlist_member_ignore_relabel
- * Same as above, except that we ignore top-level RelabelType nodes
- * while checking for a match. This is needed for some scenarios
- * involving binary-compatible sort operations.
- */
-TargetEntry *
-tlist_member_ignore_relabel(Expr *node, List *targetlist)
-{
- ListCell *temp;
-
- while (node && IsA(node, RelabelType))
- node = ((RelabelType *) node)->arg;
-
- foreach(temp, targetlist)
- {
- TargetEntry *tlentry = (TargetEntry *) lfirst(temp);
- Expr *tlexpr = tlentry->expr;
-
- while (tlexpr && IsA(tlexpr, RelabelType))
- tlexpr = ((RelabelType *) tlexpr)->arg;
-
- if (equal(node, tlexpr))
- return tlentry;
- }
- return NULL;
-}
-
/*
* tlist_member_match_var
* Same as above, except that we match the provided Var on the basis
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 035d3e1206..1d2a608ad9 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -135,6 +135,12 @@ extern EquivalenceClass *get_eclass_for_sort_expr(PlannerInfo *root,
Index sortref,
Relids rel,
bool create_it);
+extern EquivalenceMember *find_ec_member_matching_expr(EquivalenceClass *ec,
+ Expr *expr,
+ Relids relids);
+extern EquivalenceMember *find_computable_ec_member(EquivalenceClass *ec,
+ List *exprs,
+ Relids relids);
extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
extern Expr *find_em_expr_usable_for_sorting_rel(PlannerInfo *root,
EquivalenceClass *ec,
diff --git a/src/include/optimizer/tlist.h b/src/include/optimizer/tlist.h
index e081ef2d5e..d62a09665a 100644
--- a/src/include/optimizer/tlist.h
+++ b/src/include/optimizer/tlist.h
@@ -18,7 +18,6 @@
extern TargetEntry *tlist_member(Expr *node, List *targetlist);
-extern TargetEntry *tlist_member_ignore_relabel(Expr *node, List *targetlist);
extern List *add_to_flat_tlist(List *tlist, List *exprs);
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index a417b566d9..545e301e48 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -1579,6 +1579,32 @@ order by 1, 2;
-> Function Scan on generate_series
(7 rows)
+-- Parallel sort with an aggregate that can be safely generated in parallel,
+-- but we can't sort by partial aggregate values.
+explain (costs off) select count(*)
+from tenk1 t1
+join tenk1 t2 on t1.unique1 = t2.unique2
+join tenk1 t3 on t2.unique1 = t3.unique1
+order by count(*);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------
+ Sort
+ Sort Key: (count(*))
+ -> Finalize Aggregate
+ -> Gather
+ Workers Planned: 2
+ -> Partial Aggregate
+ -> Parallel Hash Join
+ Hash Cond: (t2.unique1 = t3.unique1)
+ -> Parallel Hash Join
+ Hash Cond: (t1.unique1 = t2.unique2)
+ -> Parallel Index Only Scan using tenk1_unique1 on tenk1 t1
+ -> Parallel Hash
+ -> Parallel Index Scan using tenk1_unique2 on tenk1 t2
+ -> Parallel Hash
+ -> Parallel Index Only Scan using tenk1_unique1 on tenk1 t3
+(15 rows)
+
-- Parallel sort but with expression (correlated subquery) that
-- is prohibited in parallel plans.
explain (costs off) select distinct
diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql
index 81429164d4..d8768a6b54 100644
--- a/src/test/regress/sql/incremental_sort.sql
+++ b/src/test/regress/sql/incremental_sort.sql
@@ -257,6 +257,13 @@ from tenk1, lateral (select tenk1.unique1 from generate_series(1, 1000)) as sub;
explain (costs off) select sub.unique1, md5(stringu1)
from tenk1, lateral (select tenk1.unique1 from generate_series(1, 1000)) as sub
order by 1, 2;
+-- Parallel sort with an aggregate that can be safely generated in parallel,
+-- but we can't sort by partial aggregate values.
+explain (costs off) select count(*)
+from tenk1 t1
+join tenk1 t2 on t1.unique1 = t2.unique2
+join tenk1 t3 on t2.unique1 = t3.unique1
+order by count(*);
-- Parallel sort but with expression (correlated subquery) that
-- is prohibited in parallel plans.
explain (costs off) select distinct
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index edba5e49a8..286052e3d6 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2697,20 +2697,19 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel,
EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
/*
- * We can only build a sort for pathkeys which contain an EC
- * member in the current relation's target, so ignore any suffix
- * of the list as soon as we find a pathkey without an EC member
- * in the relation.
+ * We can only build a sort for pathkeys that contain a safe EC
+ * member computable from the current relation's reltarget, so
+ * ignore the remainder of the list as soon as we find a pathkey
+ * without such a member.
*
- * By still returning the prefix of the pathkeys list that does
- * meet criteria of EC membership in the current relation, we
- * enable not just an incremental sort on the entirety of
- * query_pathkeys but also incremental sort below a JOIN.
+ * It's still worthwhile to return any prefix of the pathkeys list
+ * that meets this requirement, as we may be able to do an
+ * incremental sort.
*
* If requested, ensure the expression is parallel safe too.
*/
- if (!find_em_expr_usable_for_sorting_rel(root, pathkey_ec, rel,
- require_parallel_safe))
+ if (!relation_has_safe_ec_member(root, rel, pathkey_ec,
+ require_parallel_safe))
break;
npathkeys++;
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 5f7e505950..9d2d1022d1 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -943,17 +943,20 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
}
/*
- * Find an equivalence class member expression that can be used to build
- * a sort node using the provided relation; return NULL if no candidate.
+ * relation_has_safe_ec_member
+ * Can this relation be sorted on a "safe" member of this EC?
*
- * The selected expression must be one that prepare_sort_from_pathkeys knows
- * how to deal with, and we also apply a few additional constraints based on
- * the fact that the desired sort will be done within the scan/join part of
- * the plan.
+ * To succeed, we must find an EC member that prepare_sort_from_pathkeys knows
+ * how to sort on, given the rel's reltarget as input. There are also a few
+ * additional constraints based on the fact that the desired sort will be done
+ * within the scan/join part of the plan.
+ *
+ * At some point we might want to return the identified EquivalenceMember,
+ * but for now, callers only want to know if there is one.
*/
-Expr *
-find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
- RelOptInfo *rel, bool require_parallel_safe)
+bool
+relation_has_safe_ec_member(PlannerInfo *root, RelOptInfo *rel,
+ EquivalenceClass *ec, bool require_parallel_safe)
{
PathTarget *target = rel->reltarget;
EquivalenceMember *em;
@@ -963,7 +966,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
* Reject volatile ECs immediately; such sorts must always be postponed.
*/
if (ec->ec_has_volatile)
- return NULL;
+ return false;
/*
* Try to find an EM directly matching some reltarget member.
@@ -990,7 +993,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
(Node *) em->em_expr))
continue;
- return em->em_expr;
+ return true;
}
/*
@@ -998,7 +1001,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
*/
em = find_computable_ec_member(ec, target->exprs, rel->relids);
if (!em)
- return NULL;
+ return false;
/*
* Reject expressions involving set-returning functions, as those can't be
@@ -1006,7 +1009,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
* member that isn't a SRF, but it's quite unlikely there'd be one.)
*/
if (IS_SRF_CALL((Node *) em->em_expr))
- return NULL;
+ return false;
/*
* If requested, reject expressions that are not parallel-safe. (Again,
@@ -1014,9 +1017,9 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
*/
if (require_parallel_safe && !is_parallel_safe(root,
(Node *) em->em_expr))
- return NULL;
+ return false;
- return em->em_expr;
+ return true;
}
/*
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 1d2a608ad9..f13b9a37e9 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -142,10 +142,9 @@ extern EquivalenceMember *find_computable_ec_member(EquivalenceClass *ec,
List *exprs,
Relids relids);
extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
-extern Expr *find_em_expr_usable_for_sorting_rel(PlannerInfo *root,
- EquivalenceClass *ec,
- RelOptInfo *rel,
- bool require_parallel_safe);
+extern bool relation_has_safe_ec_member(PlannerInfo *root, RelOptInfo *rel,
+ EquivalenceClass *ec,
+ bool require_parallel_safe);
extern void generate_base_implied_equalities(PlannerInfo *root);
extern List *generate_join_implied_equalities(PlannerInfo *root,
Relids join_relids,
I wrote:
> I think it's time for some refactoring of this code so that we can
> actually share the logic. Accordingly, I propose the attached.
After sleeping on it, here's an improved version that gets rid of
an unnecessary assumption about ECs usually not containing both
parallel-safe and parallel-unsafe members. I'd tried to do this
yesterday but didn't like the amount of side-effects on createplan.c
(caused by the direct call sites not being passed the "root" pointer).
However, we can avoid refactoring createplan.c APIs by saying that it's
okay to pass root = NULL to find_computable_ec_member if you're not
asking it to check parallel safety. And there's not really a need to
put a parallel-safety check into find_ec_member_matching_expr at all;
that task can be left with the one caller that cares.
regards, tom lane
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 0188c1e9a1..6627491519 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -35,6 +35,7 @@
static EquivalenceMember *add_eq_member(EquivalenceClass *ec,
Expr *expr, Relids relids, Relids nullable_relids,
bool is_child, Oid datatype);
+static bool exprlist_member_ignore_relabel(Expr *node, List *exprs);
static void generate_base_implied_equalities_const(PlannerInfo *root,
EquivalenceClass *ec);
static void generate_base_implied_equalities_no_const(PlannerInfo *root,
@@ -769,6 +770,169 @@ get_eclass_for_sort_expr(PlannerInfo *root,
return newec;
}
+/*
+ * find_ec_member_matching_expr
+ * Locate an EquivalenceClass member matching the given expr, if any;
+ * return NULL if no match.
+ *
+ * "Matching" is defined as "equal after stripping RelabelTypes".
+ * This is used for identifying sort expressions, and we need to allow
+ * binary-compatible relabeling for some cases involving binary-compatible
+ * sort operators.
+ *
+ * Child EC members are ignored unless they belong to given 'relids'.
+ */
+EquivalenceMember *
+find_ec_member_matching_expr(EquivalenceClass *ec,
+ Expr *expr,
+ Relids relids)
+{
+ ListCell *lc;
+
+ /* We ignore binary-compatible relabeling on both ends */
+ while (expr && IsA(expr, RelabelType))
+ expr = ((RelabelType *) expr)->arg;
+
+ foreach(lc, ec->ec_members)
+ {
+ EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
+ Expr *emexpr;
+
+ /*
+ * We shouldn't be trying to sort by an equivalence class that
+ * contains a constant, so no need to consider such cases any further.
+ */
+ if (em->em_is_const)
+ continue;
+
+ /*
+ * Ignore child members unless they belong to the requested rel.
+ */
+ if (em->em_is_child &&
+ !bms_is_subset(em->em_relids, relids))
+ continue;
+
+ /*
+ * Match if same expression (after stripping relabel).
+ */
+ emexpr = em->em_expr;
+ while (emexpr && IsA(emexpr, RelabelType))
+ emexpr = ((RelabelType *) emexpr)->arg;
+
+ if (equal(emexpr, expr))
+ return em;
+ }
+
+ return NULL;
+}
+
+/*
+ * find_computable_ec_member
+ * Locate an EquivalenceClass member that can be computed from the
+ * expressions appearing in "exprs"; return NULL if no match.
+ *
+ * "exprs" can be either a list of bare expression trees, or a list of
+ * TargetEntry nodes. Either way, it should contain Vars and possibly
+ * Aggrefs and WindowFuncs, which are matched to the corresponding elements
+ * of the EquivalenceClass's expressions. As in find_ec_member_matching_expr,
+ * we ignore binary-compatible relabeling.
+ *
+ * Child EC members are ignored unless they belong to given 'relids'.
+ * Also, non-parallel-safe expressions are ignored if 'require_parallel_safe'.
+ *
+ * Note: some callers pass root == NULL for notational reasons. This is OK
+ * when require_parallel_safe is false.
+ */
+EquivalenceMember *
+find_computable_ec_member(PlannerInfo *root,
+ EquivalenceClass *ec,
+ List *exprs,
+ Relids relids,
+ bool require_parallel_safe)
+{
+ ListCell *lc;
+
+ foreach(lc, ec->ec_members)
+ {
+ EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
+ List *exprvars;
+ ListCell *lc2;
+
+ /*
+ * We shouldn't be trying to sort by an equivalence class that
+ * contains a constant, so no need to consider such cases any further.
+ */
+ if (em->em_is_const)
+ continue;
+
+ /*
+ * Ignore child members unless they belong to the requested rel.
+ */
+ if (em->em_is_child &&
+ !bms_is_subset(em->em_relids, relids))
+ continue;
+
+ /*
+ * Match if all Vars and quasi-Vars are available in "exprs".
+ */
+ exprvars = pull_var_clause((Node *) em->em_expr,
+ PVC_INCLUDE_AGGREGATES |
+ PVC_INCLUDE_WINDOWFUNCS |
+ PVC_INCLUDE_PLACEHOLDERS);
+ foreach(lc2, exprvars)
+ {
+ if (!exprlist_member_ignore_relabel(lfirst(lc2), exprs))
+ break;
+ }
+ list_free(exprvars);
+ if (lc2)
+ continue; /* we hit a non-available Var */
+
+ /*
+ * If requested, reject expressions that are not parallel-safe. We
+ * check this last because it's a rather expensive test.
+ */
+ if (require_parallel_safe &&
+ !is_parallel_safe(root, (Node *) em->em_expr))
+ continue;
+
+ return em; /* found usable expression */
+ }
+
+ return NULL;
+}
+
+/*
+ * exprlist_member_ignore_relabel
+ * Subroutine for find_computable_ec_member: is "node" in "exprs"?
+ *
+ * Per the requirements of that function, "exprs" might or might not have
+ * TargetEntry superstructure, and we ignore RelabelType.
+ */
+static bool
+exprlist_member_ignore_relabel(Expr *node, List *exprs)
+{
+ ListCell *lc;
+
+ while (node && IsA(node, RelabelType))
+ node = ((RelabelType *) node)->arg;
+
+ foreach(lc, exprs)
+ {
+ Expr *expr = (Expr *) lfirst(lc);
+
+ if (expr && IsA(expr, TargetEntry))
+ expr = ((TargetEntry *) expr)->expr;
+
+ while (expr && IsA(expr, RelabelType))
+ expr = ((RelabelType *) expr)->arg;
+
+ if (equal(node, expr))
+ return true;
+ }
+ return false;
+}
+
/*
* Find an equivalence class member expression, all of whose Vars, come from
* the indicated relation.
@@ -799,71 +963,78 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
}
/*
- * Find an equivalence class member expression that can be safely used to build
- * a sort node using the provided relation. The rules are a subset of those
- * applied in prepare_sort_from_pathkeys since that function deals with sorts
- * that must be delayed until the last stages of query execution, while here
- * we only care about proactive sorts.
+ * Find an equivalence class member expression that can be used to build
+ * a sort node using the provided relation; return NULL if no candidate.
+ *
+ * To succeed, we must find an EC member that prepare_sort_from_pathkeys knows
+ * how to sort on, given the rel's reltarget as input. There are also a few
+ * additional constraints based on the fact that the desired sort will be done
+ * within the scan/join part of the plan. Also, non-parallel-safe expressions
+ * are ignored if 'require_parallel_safe'.
*/
Expr *
find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
RelOptInfo *rel, bool require_parallel_safe)
{
- ListCell *lc_em;
+ PathTarget *target = rel->reltarget;
+ EquivalenceMember *em;
+ ListCell *lc;
/*
- * If there is more than one equivalence member matching these
- * requirements we'll be content to choose any one of them.
+ * Reject volatile ECs immediately; such sorts must always be postponed.
*/
- foreach(lc_em, ec->ec_members)
- {
- EquivalenceMember *em = lfirst(lc_em);
- Expr *em_expr = em->em_expr;
+ if (ec->ec_has_volatile)
+ return NULL;
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any further.
- */
- if (em->em_is_const)
- continue;
+ /*
+ * Try to find an EM directly matching some reltarget member.
+ */
+ foreach(lc, target->exprs)
+ {
+ Expr *targetexpr = (Expr *) lfirst(lc);
- /*
- * Any Vars in the equivalence class member need to come from this
- * relation. This is a superset of prepare_sort_from_pathkeys ignoring
- * child members unless they belong to the rel being sorted.
- */
- if (!bms_is_subset(em->em_relids, rel->relids))
+ em = find_ec_member_matching_expr(ec, targetexpr, rel->relids);
+ if (!em)
continue;
/*
- * If requested, reject expressions that are not parallel-safe.
+ * Reject expressions involving set-returning functions, as those
+ * can't be computed early either. (Note: this test and the following
+ * one are effectively checking properties of targetexpr, so there's
+ * no point in asking whether some other EC member would be better.)
*/
- if (require_parallel_safe && !is_parallel_safe(root, (Node *) em_expr))
+ if (IS_SRF_CALL((Node *) em->em_expr))
continue;
/*
- * Disallow SRFs so that all of them can be evaluated at the correct
- * time as determined by make_sort_input_target.
+ * If requested, reject expressions that are not parallel-safe. We
+ * check this last because it's a rather expensive test.
*/
- if (IS_SRF_CALL((Node *) em_expr))
+ if (require_parallel_safe &&
+ !is_parallel_safe(root, (Node *) em->em_expr))
continue;
- /*
- * As long as the expression isn't volatile then
- * prepare_sort_from_pathkeys is able to generate a new target entry,
- * so there's no need to verify that one already exists.
- *
- * While prepare_sort_from_pathkeys has to be concerned about matching
- * up a volatile expression to the proper tlist entry, it doesn't seem
- * valuable here to expend the work trying to find a match in the
- * target's exprs since such a sort will have to be postponed anyway.
- */
- if (!ec->ec_has_volatile)
- return em->em_expr;
+ return em->em_expr;
}
- /* We didn't find any suitable equivalence class expression */
- return NULL;
+ /*
+ * Try to find a expression computable from the reltarget.
+ */
+ em = find_computable_ec_member(root, ec, target->exprs, rel->relids,
+ require_parallel_safe);
+ if (!em)
+ return NULL;
+
+ /*
+ * Reject expressions involving set-returning functions, as those can't be
+ * computed early either. (There's no point in looking for another EC
+ * member in this case; since SRFs can't appear in WHERE, they cannot
+ * belong to multi-member ECs.)
+ */
+ if (IS_SRF_CALL((Node *) em->em_expr))
+ return NULL;
+
+ return em->em_expr;
}
/*
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 22f10fa339..a9aff24831 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -269,9 +269,6 @@ static Plan *prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
Oid **p_sortOperators,
Oid **p_collations,
bool **p_nullsFirst);
-static EquivalenceMember *find_ec_member_for_tle(EquivalenceClass *ec,
- TargetEntry *tle,
- Relids relids);
static Sort *make_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
Relids relids);
static IncrementalSort *make_incrementalsort_from_pathkeys(Plan *lefttree,
@@ -2110,7 +2107,7 @@ create_sort_plan(PlannerInfo *root, SortPath *best_path, int flags)
flags | CP_SMALL_TLIST);
/*
- * make_sort_from_pathkeys() indirectly calls find_ec_member_for_tle(),
+ * make_sort_from_pathkeys indirectly calls find_ec_member_matching_expr,
* which will ignore any child EC members that don't belong to the given
* relids. Thus, if this sort path is based on a child relation, we must
* pass its relids.
@@ -6017,9 +6014,6 @@ make_incrementalsort(Plan *lefttree, int numCols, int nPresortedCols,
*
* Returns the node which is to be the input to the Sort (either lefttree,
* or a Result stacked atop lefttree).
- *
- * Note: Restrictions on what expressions are safely sortable may also need to
- * be added to find_em_expr_usable_for_sorting_rel.
*/
static Plan *
prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
@@ -6089,7 +6083,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
tle = get_tle_by_resno(tlist, reqColIdx[numsortkeys]);
if (tle)
{
- em = find_ec_member_for_tle(ec, tle, relids);
+ em = find_ec_member_matching_expr(ec, tle->expr, relids);
if (em)
{
/* found expr at right place in tlist */
@@ -6120,7 +6114,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
foreach(j, tlist)
{
tle = (TargetEntry *) lfirst(j);
- em = find_ec_member_for_tle(ec, tle, relids);
+ em = find_ec_member_matching_expr(ec, tle->expr, relids);
if (em)
{
/* found expr already in tlist */
@@ -6134,56 +6128,12 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
if (!tle)
{
/*
- * No matching tlist item; look for a computable expression. Note
- * that we treat Aggrefs as if they were variables; this is
- * necessary when attempting to sort the output from an Agg node
- * for use in a WindowFunc (since grouping_planner will have
- * treated the Aggrefs as variables, too). Likewise, if we find a
- * WindowFunc in a sort expression, treat it as a variable.
+ * No matching tlist item; look for a computable expression.
*/
- Expr *sortexpr = NULL;
-
- foreach(j, ec->ec_members)
- {
- EquivalenceMember *em = (EquivalenceMember *) lfirst(j);
- List *exprvars;
- ListCell *k;
-
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any
- * further.
- */
- if (em->em_is_const)
- continue;
-
- /*
- * Ignore child members unless they belong to the rel being
- * sorted.
- */
- if (em->em_is_child &&
- !bms_is_subset(em->em_relids, relids))
- continue;
-
- sortexpr = em->em_expr;
- exprvars = pull_var_clause((Node *) sortexpr,
- PVC_INCLUDE_AGGREGATES |
- PVC_INCLUDE_WINDOWFUNCS |
- PVC_INCLUDE_PLACEHOLDERS);
- foreach(k, exprvars)
- {
- if (!tlist_member_ignore_relabel(lfirst(k), tlist))
- break;
- }
- list_free(exprvars);
- if (!k)
- {
- pk_datatype = em->em_datatype;
- break; /* found usable expression */
- }
- }
- if (!j)
+ em = find_computable_ec_member(NULL, ec, tlist, relids, false);
+ if (!em)
elog(ERROR, "could not find pathkey item to sort");
+ pk_datatype = em->em_datatype;
/*
* Do we need to insert a Result node?
@@ -6203,7 +6153,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
/*
* Add resjunk entry to input's tlist
*/
- tle = makeTargetEntry(sortexpr,
+ tle = makeTargetEntry(copyObject(em->em_expr),
list_length(tlist) + 1,
NULL,
true);
@@ -6242,56 +6192,6 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
return lefttree;
}
-/*
- * find_ec_member_for_tle
- * Locate an EquivalenceClass member matching the given TLE, if any
- *
- * Child EC members are ignored unless they belong to given 'relids'.
- */
-static EquivalenceMember *
-find_ec_member_for_tle(EquivalenceClass *ec,
- TargetEntry *tle,
- Relids relids)
-{
- Expr *tlexpr;
- ListCell *lc;
-
- /* We ignore binary-compatible relabeling on both ends */
- tlexpr = tle->expr;
- while (tlexpr && IsA(tlexpr, RelabelType))
- tlexpr = ((RelabelType *) tlexpr)->arg;
-
- foreach(lc, ec->ec_members)
- {
- EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
- Expr *emexpr;
-
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any further.
- */
- if (em->em_is_const)
- continue;
-
- /*
- * Ignore child members unless they belong to the rel being sorted.
- */
- if (em->em_is_child &&
- !bms_is_subset(em->em_relids, relids))
- continue;
-
- /* Match if same expression (after stripping relabel) */
- emexpr = em->em_expr;
- while (emexpr && IsA(emexpr, RelabelType))
- emexpr = ((RelabelType *) emexpr)->arg;
-
- if (equal(emexpr, tlexpr))
- return em;
- }
-
- return NULL;
-}
-
/*
* make_sort_from_pathkeys
* Create sort plan to sort according to given pathkeys
@@ -6753,7 +6653,7 @@ make_unique_from_pathkeys(Plan *lefttree, List *pathkeys, int numCols)
foreach(j, plan->targetlist)
{
tle = (TargetEntry *) lfirst(j);
- em = find_ec_member_for_tle(ec, tle, NULL);
+ em = find_ec_member_matching_expr(ec, tle->expr, NULL);
if (em)
{
/* found expr already in tlist */
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 8a26288070..311579d059 100644
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/tlist.c
@@ -79,34 +79,6 @@ tlist_member(Expr *node, List *targetlist)
return NULL;
}
-/*
- * tlist_member_ignore_relabel
- * Same as above, except that we ignore top-level RelabelType nodes
- * while checking for a match. This is needed for some scenarios
- * involving binary-compatible sort operations.
- */
-TargetEntry *
-tlist_member_ignore_relabel(Expr *node, List *targetlist)
-{
- ListCell *temp;
-
- while (node && IsA(node, RelabelType))
- node = ((RelabelType *) node)->arg;
-
- foreach(temp, targetlist)
- {
- TargetEntry *tlentry = (TargetEntry *) lfirst(temp);
- Expr *tlexpr = tlentry->expr;
-
- while (tlexpr && IsA(tlexpr, RelabelType))
- tlexpr = ((RelabelType *) tlexpr)->arg;
-
- if (equal(node, tlexpr))
- return tlentry;
- }
- return NULL;
-}
-
/*
* tlist_member_match_var
* Same as above, except that we match the provided Var on the basis
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 035d3e1206..888e85ff5b 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -135,6 +135,14 @@ extern EquivalenceClass *get_eclass_for_sort_expr(PlannerInfo *root,
Index sortref,
Relids rel,
bool create_it);
+extern EquivalenceMember *find_ec_member_matching_expr(EquivalenceClass *ec,
+ Expr *expr,
+ Relids relids);
+extern EquivalenceMember *find_computable_ec_member(PlannerInfo *root,
+ EquivalenceClass *ec,
+ List *exprs,
+ Relids relids,
+ bool require_parallel_safe);
extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
extern Expr *find_em_expr_usable_for_sorting_rel(PlannerInfo *root,
EquivalenceClass *ec,
diff --git a/src/include/optimizer/tlist.h b/src/include/optimizer/tlist.h
index e081ef2d5e..d62a09665a 100644
--- a/src/include/optimizer/tlist.h
+++ b/src/include/optimizer/tlist.h
@@ -18,7 +18,6 @@
extern TargetEntry *tlist_member(Expr *node, List *targetlist);
-extern TargetEntry *tlist_member_ignore_relabel(Expr *node, List *targetlist);
extern List *add_to_flat_tlist(List *tlist, List *exprs);
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index a417b566d9..545e301e48 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -1579,6 +1579,32 @@ order by 1, 2;
-> Function Scan on generate_series
(7 rows)
+-- Parallel sort with an aggregate that can be safely generated in parallel,
+-- but we can't sort by partial aggregate values.
+explain (costs off) select count(*)
+from tenk1 t1
+join tenk1 t2 on t1.unique1 = t2.unique2
+join tenk1 t3 on t2.unique1 = t3.unique1
+order by count(*);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------
+ Sort
+ Sort Key: (count(*))
+ -> Finalize Aggregate
+ -> Gather
+ Workers Planned: 2
+ -> Partial Aggregate
+ -> Parallel Hash Join
+ Hash Cond: (t2.unique1 = t3.unique1)
+ -> Parallel Hash Join
+ Hash Cond: (t1.unique1 = t2.unique2)
+ -> Parallel Index Only Scan using tenk1_unique1 on tenk1 t1
+ -> Parallel Hash
+ -> Parallel Index Scan using tenk1_unique2 on tenk1 t2
+ -> Parallel Hash
+ -> Parallel Index Only Scan using tenk1_unique1 on tenk1 t3
+(15 rows)
+
-- Parallel sort but with expression (correlated subquery) that
-- is prohibited in parallel plans.
explain (costs off) select distinct
diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql
index 81429164d4..d8768a6b54 100644
--- a/src/test/regress/sql/incremental_sort.sql
+++ b/src/test/regress/sql/incremental_sort.sql
@@ -257,6 +257,13 @@ from tenk1, lateral (select tenk1.unique1 from generate_series(1, 1000)) as sub;
explain (costs off) select sub.unique1, md5(stringu1)
from tenk1, lateral (select tenk1.unique1 from generate_series(1, 1000)) as sub
order by 1, 2;
+-- Parallel sort with an aggregate that can be safely generated in parallel,
+-- but we can't sort by partial aggregate values.
+explain (costs off) select count(*)
+from tenk1 t1
+join tenk1 t2 on t1.unique1 = t2.unique2
+join tenk1 t3 on t2.unique1 = t3.unique1
+order by count(*);
-- Parallel sort but with expression (correlated subquery) that
-- is prohibited in parallel plans.
explain (costs off) select distinct
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index edba5e49a8..286052e3d6 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2697,20 +2697,19 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel,
EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
/*
- * We can only build a sort for pathkeys which contain an EC
- * member in the current relation's target, so ignore any suffix
- * of the list as soon as we find a pathkey without an EC member
- * in the relation.
+ * We can only build a sort for pathkeys that contain a safe EC
+ * member computable from the current relation's reltarget, so
+ * ignore the remainder of the list as soon as we find a pathkey
+ * without such a member.
*
- * By still returning the prefix of the pathkeys list that does
- * meet criteria of EC membership in the current relation, we
- * enable not just an incremental sort on the entirety of
- * query_pathkeys but also incremental sort below a JOIN.
+ * It's still worthwhile to return any prefix of the pathkeys list
+ * that meets this requirement, as we may be able to do an
+ * incremental sort.
*
* If requested, ensure the expression is parallel safe too.
*/
- if (!find_em_expr_usable_for_sorting_rel(root, pathkey_ec, rel,
- require_parallel_safe))
+ if (!relation_has_safe_ec_member(root, rel, pathkey_ec,
+ require_parallel_safe))
break;
npathkeys++;
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 6627491519..7fdcd9342c 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -963,18 +963,21 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
}
/*
- * Find an equivalence class member expression that can be used to build
- * a sort node using the provided relation; return NULL if no candidate.
+ * relation_has_safe_ec_member
+ * Can this relation be sorted on a "safe" member of this EC?
*
* To succeed, we must find an EC member that prepare_sort_from_pathkeys knows
* how to sort on, given the rel's reltarget as input. There are also a few
* additional constraints based on the fact that the desired sort will be done
* within the scan/join part of the plan. Also, non-parallel-safe expressions
* are ignored if 'require_parallel_safe'.
+ *
+ * At some point we might want to return the identified EquivalenceMember,
+ * but for now, callers only want to know if there is one.
*/
-Expr *
-find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
- RelOptInfo *rel, bool require_parallel_safe)
+bool
+relation_has_safe_ec_member(PlannerInfo *root, RelOptInfo *rel,
+ EquivalenceClass *ec, bool require_parallel_safe)
{
PathTarget *target = rel->reltarget;
EquivalenceMember *em;
@@ -984,7 +987,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
* Reject volatile ECs immediately; such sorts must always be postponed.
*/
if (ec->ec_has_volatile)
- return NULL;
+ return false;
/*
* Try to find an EM directly matching some reltarget member.
@@ -1014,7 +1017,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
!is_parallel_safe(root, (Node *) em->em_expr))
continue;
- return em->em_expr;
+ return true;
}
/*
@@ -1023,7 +1026,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
em = find_computable_ec_member(root, ec, target->exprs, rel->relids,
require_parallel_safe);
if (!em)
- return NULL;
+ return false;
/*
* Reject expressions involving set-returning functions, as those can't be
@@ -1032,9 +1035,9 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
* belong to multi-member ECs.)
*/
if (IS_SRF_CALL((Node *) em->em_expr))
- return NULL;
+ return false;
- return em->em_expr;
+ return true;
}
/*
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 888e85ff5b..4ced55933d 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -144,10 +144,9 @@ extern EquivalenceMember *find_computable_ec_member(PlannerInfo *root,
Relids relids,
bool require_parallel_safe);
extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
-extern Expr *find_em_expr_usable_for_sorting_rel(PlannerInfo *root,
- EquivalenceClass *ec,
- RelOptInfo *rel,
- bool require_parallel_safe);
+extern bool relation_has_safe_ec_member(PlannerInfo *root, RelOptInfo *rel,
+ EquivalenceClass *ec,
+ bool require_parallel_safe);
extern void generate_base_implied_equalities(PlannerInfo *root);
extern List *generate_join_implied_equalities(PlannerInfo *root,
Relids join_relids,
On Sun, Apr 18, 2021 at 1:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wrote: > > I think it's time for some refactoring of this code so that we can > > actually share the logic. Accordingly, I propose the attached. > > After sleeping on it, here's an improved version that gets rid of > an unnecessary assumption about ECs usually not containing both > parallel-safe and parallel-unsafe members. I'd tried to do this > yesterday but didn't like the amount of side-effects on createplan.c > (caused by the direct call sites not being passed the "root" pointer). > However, we can avoid refactoring createplan.c APIs by saying that it's > okay to pass root = NULL to find_computable_ec_member if you're not > asking it to check parallel safety. And there's not really a need to > put a parallel-safety check into find_ec_member_matching_expr at all; > that task can be left with the one caller that cares. I like the refactoring here. Two things I wonder: 1. Should we add tests for the relabel code path? 2. It'd be nice not to have the IS_SRF_CALL duplicated, but that might add enough complexity that it's not worth it. Thanks, James Coleman
On Sat, Apr 17, 2021 at 3:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > ... > Also, I don't much care for either the name or API of > find_em_expr_usable_for_sorting_rel. The sole current caller only > really needs a boolean result, and if it did need more than that > it'd likely need the whole EquivalenceMember not just the em_expr > (certainly createplan.c does). So 0002 attached is some bikeshedding > on that. I kept that separate because it might be wise to do it only > in HEAD, just in case somebody out there is calling the function from > an extension. I forgot to comment on this in my previous email, but it seems to me that relation_has_safe_ec_member, while less wordy, isn't quite descriptive enough. Perhaps something like relation_has_sort_safe_ec_member? James Coleman
James Coleman <jtc331@gmail.com> writes:
> I forgot to comment on this in my previous email, but it seems to me
> that relation_has_safe_ec_member, while less wordy, isn't quite
> descriptive enough. Perhaps something like
> relation_has_sort_safe_ec_member?
I'm not wedded to that name, certainly, but it seems like neither
of these is quite getting at the issue. An EC can be sorted on,
by definition, but there are some things we don't want to sort
on till the final output step. I was trying to think of something
using the terminology "early sort", but didn't much like
"relation_has_early_sortable_ec_member" or obvious variants of that.
regards, tom lane
I wrote:
> I'm not wedded to that name, certainly, but it seems like neither
> of these is quite getting at the issue. An EC can be sorted on,
> by definition, but there are some things we don't want to sort
> on till the final output step. I was trying to think of something
> using the terminology "early sort", but didn't much like
> "relation_has_early_sortable_ec_member" or obvious variants of that.
... or, as long as it's returning a boolean, maybe it could be
"relation_can_be_sorted_early" ?
regards, tom lane
James Coleman <jtc331@gmail.com> writes:
> Two things I wonder:
> 1. Should we add tests for the relabel code path?
As far as that goes, the Relabel-stripping loops in
find_ec_member_matching_expr are already exercised in the core
regression tests (I didn't bother to discover exactly where, but
a quick coverage test run says that they're hit). The ones in
exprlist_member_ignore_relabel are not iterated though. On
reflection, the first loop stripping the input node is visibly
unreachable by the sole caller, since everything in the exprvars
list will be a Var, Aggref, WindowFunc, or PlaceHolderVar. I'm
less sure about what is possible in the targetlist that we're
referencing, but it strikes me that ignoring relabel on that
side is probably functionally wrong: if we have say "f(textcol)"
as an expression to be sorted on, but what is in the tlist is
textcol::varchar or the like, I do not think setrefs.c will
consider that an acceptable match. So now that's seeming like
an actual bug --- although the lack of field reports suggests
that it's unreachable, most likely because if we do have
"f(textcol)" as a sort candidate, we'll have made sure to emit
plain "textcol" from the source relation, regardless of whether
there might also be a reason to emit textcol::varchar.
Anyway I'm now inclined to remove that behavior from
find_computable_ec_member, and adjust comments accordingly.
> 2. It'd be nice not to have the IS_SRF_CALL duplicated, but that might
> add enough complexity that it's not worth it.
Yeah, I'd messed around with variants that put more smarts
into the bottom-level functions, and decided that it wasn't
a net improvement.
regards, tom lane
I wrote:
> Anyway I'm now inclined to remove that behavior from
> find_computable_ec_member, and adjust comments accordingly.
After some more testing, that seems like a good thing to do,
so here's a v4.
regards, tom lane
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 0188c1e9a1..6e87fba2aa 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -35,6 +35,7 @@
static EquivalenceMember *add_eq_member(EquivalenceClass *ec,
Expr *expr, Relids relids, Relids nullable_relids,
bool is_child, Oid datatype);
+static bool is_exprlist_member(Expr *node, List *exprs);
static void generate_base_implied_equalities_const(PlannerInfo *root,
EquivalenceClass *ec);
static void generate_base_implied_equalities_no_const(PlannerInfo *root,
@@ -769,6 +770,167 @@ get_eclass_for_sort_expr(PlannerInfo *root,
return newec;
}
+/*
+ * find_ec_member_matching_expr
+ * Locate an EquivalenceClass member matching the given expr, if any;
+ * return NULL if no match.
+ *
+ * "Matching" is defined as "equal after stripping RelabelTypes".
+ * This is used for identifying sort expressions, and we need to allow
+ * binary-compatible relabeling for some cases involving binary-compatible
+ * sort operators.
+ *
+ * Child EC members are ignored unless they belong to given 'relids'.
+ */
+EquivalenceMember *
+find_ec_member_matching_expr(EquivalenceClass *ec,
+ Expr *expr,
+ Relids relids)
+{
+ ListCell *lc;
+
+ /* We ignore binary-compatible relabeling on both ends */
+ while (expr && IsA(expr, RelabelType))
+ expr = ((RelabelType *) expr)->arg;
+
+ foreach(lc, ec->ec_members)
+ {
+ EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
+ Expr *emexpr;
+
+ /*
+ * We shouldn't be trying to sort by an equivalence class that
+ * contains a constant, so no need to consider such cases any further.
+ */
+ if (em->em_is_const)
+ continue;
+
+ /*
+ * Ignore child members unless they belong to the requested rel.
+ */
+ if (em->em_is_child &&
+ !bms_is_subset(em->em_relids, relids))
+ continue;
+
+ /*
+ * Match if same expression (after stripping relabel).
+ */
+ emexpr = em->em_expr;
+ while (emexpr && IsA(emexpr, RelabelType))
+ emexpr = ((RelabelType *) emexpr)->arg;
+
+ if (equal(emexpr, expr))
+ return em;
+ }
+
+ return NULL;
+}
+
+/*
+ * find_computable_ec_member
+ * Locate an EquivalenceClass member that can be computed from the
+ * expressions appearing in "exprs"; return NULL if no match.
+ *
+ * "exprs" can be either a list of bare expression trees, or a list of
+ * TargetEntry nodes. Either way, it should contain Vars and possibly
+ * Aggrefs and WindowFuncs, which are matched to the corresponding elements
+ * of the EquivalenceClass's expressions.
+ *
+ * Unlike find_ec_member_matching_expr, there's no special provision here
+ * for binary-compatible relabeling. This is intentional: if we have to
+ * compute an expression in this way, setrefs.c is going to insist on exact
+ * matches of Vars to the source tlist.
+ *
+ * Child EC members are ignored unless they belong to given 'relids'.
+ * Also, non-parallel-safe expressions are ignored if 'require_parallel_safe'.
+ *
+ * Note: some callers pass root == NULL for notational reasons. This is OK
+ * when require_parallel_safe is false.
+ */
+EquivalenceMember *
+find_computable_ec_member(PlannerInfo *root,
+ EquivalenceClass *ec,
+ List *exprs,
+ Relids relids,
+ bool require_parallel_safe)
+{
+ ListCell *lc;
+
+ foreach(lc, ec->ec_members)
+ {
+ EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
+ List *exprvars;
+ ListCell *lc2;
+
+ /*
+ * We shouldn't be trying to sort by an equivalence class that
+ * contains a constant, so no need to consider such cases any further.
+ */
+ if (em->em_is_const)
+ continue;
+
+ /*
+ * Ignore child members unless they belong to the requested rel.
+ */
+ if (em->em_is_child &&
+ !bms_is_subset(em->em_relids, relids))
+ continue;
+
+ /*
+ * Match if all Vars and quasi-Vars are available in "exprs".
+ */
+ exprvars = pull_var_clause((Node *) em->em_expr,
+ PVC_INCLUDE_AGGREGATES |
+ PVC_INCLUDE_WINDOWFUNCS |
+ PVC_INCLUDE_PLACEHOLDERS);
+ foreach(lc2, exprvars)
+ {
+ if (!is_exprlist_member(lfirst(lc2), exprs))
+ break;
+ }
+ list_free(exprvars);
+ if (lc2)
+ continue; /* we hit a non-available Var */
+
+ /*
+ * If requested, reject expressions that are not parallel-safe. We
+ * check this last because it's a rather expensive test.
+ */
+ if (require_parallel_safe &&
+ !is_parallel_safe(root, (Node *) em->em_expr))
+ continue;
+
+ return em; /* found usable expression */
+ }
+
+ return NULL;
+}
+
+/*
+ * is_exprlist_member
+ * Subroutine for find_computable_ec_member: is "node" in "exprs"?
+ *
+ * Per the requirements of that function, "exprs" might or might not have
+ * TargetEntry superstructure.
+ */
+static bool
+is_exprlist_member(Expr *node, List *exprs)
+{
+ ListCell *lc;
+
+ foreach(lc, exprs)
+ {
+ Expr *expr = (Expr *) lfirst(lc);
+
+ if (expr && IsA(expr, TargetEntry))
+ expr = ((TargetEntry *) expr)->expr;
+
+ if (equal(node, expr))
+ return true;
+ }
+ return false;
+}
+
/*
* Find an equivalence class member expression, all of whose Vars, come from
* the indicated relation.
@@ -799,71 +961,78 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
}
/*
- * Find an equivalence class member expression that can be safely used to build
- * a sort node using the provided relation. The rules are a subset of those
- * applied in prepare_sort_from_pathkeys since that function deals with sorts
- * that must be delayed until the last stages of query execution, while here
- * we only care about proactive sorts.
+ * Find an equivalence class member expression that can be used to build
+ * a sort node using the provided relation; return NULL if no candidate.
+ *
+ * To succeed, we must find an EC member that prepare_sort_from_pathkeys knows
+ * how to sort on, given the rel's reltarget as input. There are also a few
+ * additional constraints based on the fact that the desired sort will be done
+ * within the scan/join part of the plan. Also, non-parallel-safe expressions
+ * are ignored if 'require_parallel_safe'.
*/
Expr *
find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
RelOptInfo *rel, bool require_parallel_safe)
{
- ListCell *lc_em;
+ PathTarget *target = rel->reltarget;
+ EquivalenceMember *em;
+ ListCell *lc;
/*
- * If there is more than one equivalence member matching these
- * requirements we'll be content to choose any one of them.
+ * Reject volatile ECs immediately; such sorts must always be postponed.
*/
- foreach(lc_em, ec->ec_members)
- {
- EquivalenceMember *em = lfirst(lc_em);
- Expr *em_expr = em->em_expr;
+ if (ec->ec_has_volatile)
+ return NULL;
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any further.
- */
- if (em->em_is_const)
- continue;
+ /*
+ * Try to find an EM directly matching some reltarget member.
+ */
+ foreach(lc, target->exprs)
+ {
+ Expr *targetexpr = (Expr *) lfirst(lc);
- /*
- * Any Vars in the equivalence class member need to come from this
- * relation. This is a superset of prepare_sort_from_pathkeys ignoring
- * child members unless they belong to the rel being sorted.
- */
- if (!bms_is_subset(em->em_relids, rel->relids))
+ em = find_ec_member_matching_expr(ec, targetexpr, rel->relids);
+ if (!em)
continue;
/*
- * If requested, reject expressions that are not parallel-safe.
+ * Reject expressions involving set-returning functions, as those
+ * can't be computed early either. (Note: this test and the following
+ * one are effectively checking properties of targetexpr, so there's
+ * no point in asking whether some other EC member would be better.)
*/
- if (require_parallel_safe && !is_parallel_safe(root, (Node *) em_expr))
+ if (IS_SRF_CALL((Node *) em->em_expr))
continue;
/*
- * Disallow SRFs so that all of them can be evaluated at the correct
- * time as determined by make_sort_input_target.
+ * If requested, reject expressions that are not parallel-safe. We
+ * check this last because it's a rather expensive test.
*/
- if (IS_SRF_CALL((Node *) em_expr))
+ if (require_parallel_safe &&
+ !is_parallel_safe(root, (Node *) em->em_expr))
continue;
- /*
- * As long as the expression isn't volatile then
- * prepare_sort_from_pathkeys is able to generate a new target entry,
- * so there's no need to verify that one already exists.
- *
- * While prepare_sort_from_pathkeys has to be concerned about matching
- * up a volatile expression to the proper tlist entry, it doesn't seem
- * valuable here to expend the work trying to find a match in the
- * target's exprs since such a sort will have to be postponed anyway.
- */
- if (!ec->ec_has_volatile)
- return em->em_expr;
+ return em->em_expr;
}
- /* We didn't find any suitable equivalence class expression */
- return NULL;
+ /*
+ * Try to find a expression computable from the reltarget.
+ */
+ em = find_computable_ec_member(root, ec, target->exprs, rel->relids,
+ require_parallel_safe);
+ if (!em)
+ return NULL;
+
+ /*
+ * Reject expressions involving set-returning functions, as those can't be
+ * computed early either. (There's no point in looking for another EC
+ * member in this case; since SRFs can't appear in WHERE, they cannot
+ * belong to multi-member ECs.)
+ */
+ if (IS_SRF_CALL((Node *) em->em_expr))
+ return NULL;
+
+ return em->em_expr;
}
/*
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 22f10fa339..a9aff24831 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -269,9 +269,6 @@ static Plan *prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
Oid **p_sortOperators,
Oid **p_collations,
bool **p_nullsFirst);
-static EquivalenceMember *find_ec_member_for_tle(EquivalenceClass *ec,
- TargetEntry *tle,
- Relids relids);
static Sort *make_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
Relids relids);
static IncrementalSort *make_incrementalsort_from_pathkeys(Plan *lefttree,
@@ -2110,7 +2107,7 @@ create_sort_plan(PlannerInfo *root, SortPath *best_path, int flags)
flags | CP_SMALL_TLIST);
/*
- * make_sort_from_pathkeys() indirectly calls find_ec_member_for_tle(),
+ * make_sort_from_pathkeys indirectly calls find_ec_member_matching_expr,
* which will ignore any child EC members that don't belong to the given
* relids. Thus, if this sort path is based on a child relation, we must
* pass its relids.
@@ -6017,9 +6014,6 @@ make_incrementalsort(Plan *lefttree, int numCols, int nPresortedCols,
*
* Returns the node which is to be the input to the Sort (either lefttree,
* or a Result stacked atop lefttree).
- *
- * Note: Restrictions on what expressions are safely sortable may also need to
- * be added to find_em_expr_usable_for_sorting_rel.
*/
static Plan *
prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
@@ -6089,7 +6083,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
tle = get_tle_by_resno(tlist, reqColIdx[numsortkeys]);
if (tle)
{
- em = find_ec_member_for_tle(ec, tle, relids);
+ em = find_ec_member_matching_expr(ec, tle->expr, relids);
if (em)
{
/* found expr at right place in tlist */
@@ -6120,7 +6114,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
foreach(j, tlist)
{
tle = (TargetEntry *) lfirst(j);
- em = find_ec_member_for_tle(ec, tle, relids);
+ em = find_ec_member_matching_expr(ec, tle->expr, relids);
if (em)
{
/* found expr already in tlist */
@@ -6134,56 +6128,12 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
if (!tle)
{
/*
- * No matching tlist item; look for a computable expression. Note
- * that we treat Aggrefs as if they were variables; this is
- * necessary when attempting to sort the output from an Agg node
- * for use in a WindowFunc (since grouping_planner will have
- * treated the Aggrefs as variables, too). Likewise, if we find a
- * WindowFunc in a sort expression, treat it as a variable.
+ * No matching tlist item; look for a computable expression.
*/
- Expr *sortexpr = NULL;
-
- foreach(j, ec->ec_members)
- {
- EquivalenceMember *em = (EquivalenceMember *) lfirst(j);
- List *exprvars;
- ListCell *k;
-
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any
- * further.
- */
- if (em->em_is_const)
- continue;
-
- /*
- * Ignore child members unless they belong to the rel being
- * sorted.
- */
- if (em->em_is_child &&
- !bms_is_subset(em->em_relids, relids))
- continue;
-
- sortexpr = em->em_expr;
- exprvars = pull_var_clause((Node *) sortexpr,
- PVC_INCLUDE_AGGREGATES |
- PVC_INCLUDE_WINDOWFUNCS |
- PVC_INCLUDE_PLACEHOLDERS);
- foreach(k, exprvars)
- {
- if (!tlist_member_ignore_relabel(lfirst(k), tlist))
- break;
- }
- list_free(exprvars);
- if (!k)
- {
- pk_datatype = em->em_datatype;
- break; /* found usable expression */
- }
- }
- if (!j)
+ em = find_computable_ec_member(NULL, ec, tlist, relids, false);
+ if (!em)
elog(ERROR, "could not find pathkey item to sort");
+ pk_datatype = em->em_datatype;
/*
* Do we need to insert a Result node?
@@ -6203,7 +6153,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
/*
* Add resjunk entry to input's tlist
*/
- tle = makeTargetEntry(sortexpr,
+ tle = makeTargetEntry(copyObject(em->em_expr),
list_length(tlist) + 1,
NULL,
true);
@@ -6242,56 +6192,6 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
return lefttree;
}
-/*
- * find_ec_member_for_tle
- * Locate an EquivalenceClass member matching the given TLE, if any
- *
- * Child EC members are ignored unless they belong to given 'relids'.
- */
-static EquivalenceMember *
-find_ec_member_for_tle(EquivalenceClass *ec,
- TargetEntry *tle,
- Relids relids)
-{
- Expr *tlexpr;
- ListCell *lc;
-
- /* We ignore binary-compatible relabeling on both ends */
- tlexpr = tle->expr;
- while (tlexpr && IsA(tlexpr, RelabelType))
- tlexpr = ((RelabelType *) tlexpr)->arg;
-
- foreach(lc, ec->ec_members)
- {
- EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
- Expr *emexpr;
-
- /*
- * We shouldn't be trying to sort by an equivalence class that
- * contains a constant, so no need to consider such cases any further.
- */
- if (em->em_is_const)
- continue;
-
- /*
- * Ignore child members unless they belong to the rel being sorted.
- */
- if (em->em_is_child &&
- !bms_is_subset(em->em_relids, relids))
- continue;
-
- /* Match if same expression (after stripping relabel) */
- emexpr = em->em_expr;
- while (emexpr && IsA(emexpr, RelabelType))
- emexpr = ((RelabelType *) emexpr)->arg;
-
- if (equal(emexpr, tlexpr))
- return em;
- }
-
- return NULL;
-}
-
/*
* make_sort_from_pathkeys
* Create sort plan to sort according to given pathkeys
@@ -6753,7 +6653,7 @@ make_unique_from_pathkeys(Plan *lefttree, List *pathkeys, int numCols)
foreach(j, plan->targetlist)
{
tle = (TargetEntry *) lfirst(j);
- em = find_ec_member_for_tle(ec, tle, NULL);
+ em = find_ec_member_matching_expr(ec, tle->expr, NULL);
if (em)
{
/* found expr already in tlist */
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 8a26288070..311579d059 100644
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/tlist.c
@@ -79,34 +79,6 @@ tlist_member(Expr *node, List *targetlist)
return NULL;
}
-/*
- * tlist_member_ignore_relabel
- * Same as above, except that we ignore top-level RelabelType nodes
- * while checking for a match. This is needed for some scenarios
- * involving binary-compatible sort operations.
- */
-TargetEntry *
-tlist_member_ignore_relabel(Expr *node, List *targetlist)
-{
- ListCell *temp;
-
- while (node && IsA(node, RelabelType))
- node = ((RelabelType *) node)->arg;
-
- foreach(temp, targetlist)
- {
- TargetEntry *tlentry = (TargetEntry *) lfirst(temp);
- Expr *tlexpr = tlentry->expr;
-
- while (tlexpr && IsA(tlexpr, RelabelType))
- tlexpr = ((RelabelType *) tlexpr)->arg;
-
- if (equal(node, tlexpr))
- return tlentry;
- }
- return NULL;
-}
-
/*
* tlist_member_match_var
* Same as above, except that we match the provided Var on the basis
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 035d3e1206..888e85ff5b 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -135,6 +135,14 @@ extern EquivalenceClass *get_eclass_for_sort_expr(PlannerInfo *root,
Index sortref,
Relids rel,
bool create_it);
+extern EquivalenceMember *find_ec_member_matching_expr(EquivalenceClass *ec,
+ Expr *expr,
+ Relids relids);
+extern EquivalenceMember *find_computable_ec_member(PlannerInfo *root,
+ EquivalenceClass *ec,
+ List *exprs,
+ Relids relids,
+ bool require_parallel_safe);
extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
extern Expr *find_em_expr_usable_for_sorting_rel(PlannerInfo *root,
EquivalenceClass *ec,
diff --git a/src/include/optimizer/tlist.h b/src/include/optimizer/tlist.h
index e081ef2d5e..d62a09665a 100644
--- a/src/include/optimizer/tlist.h
+++ b/src/include/optimizer/tlist.h
@@ -18,7 +18,6 @@
extern TargetEntry *tlist_member(Expr *node, List *targetlist);
-extern TargetEntry *tlist_member_ignore_relabel(Expr *node, List *targetlist);
extern List *add_to_flat_tlist(List *tlist, List *exprs);
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index a417b566d9..545e301e48 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -1579,6 +1579,32 @@ order by 1, 2;
-> Function Scan on generate_series
(7 rows)
+-- Parallel sort with an aggregate that can be safely generated in parallel,
+-- but we can't sort by partial aggregate values.
+explain (costs off) select count(*)
+from tenk1 t1
+join tenk1 t2 on t1.unique1 = t2.unique2
+join tenk1 t3 on t2.unique1 = t3.unique1
+order by count(*);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------
+ Sort
+ Sort Key: (count(*))
+ -> Finalize Aggregate
+ -> Gather
+ Workers Planned: 2
+ -> Partial Aggregate
+ -> Parallel Hash Join
+ Hash Cond: (t2.unique1 = t3.unique1)
+ -> Parallel Hash Join
+ Hash Cond: (t1.unique1 = t2.unique2)
+ -> Parallel Index Only Scan using tenk1_unique1 on tenk1 t1
+ -> Parallel Hash
+ -> Parallel Index Scan using tenk1_unique2 on tenk1 t2
+ -> Parallel Hash
+ -> Parallel Index Only Scan using tenk1_unique1 on tenk1 t3
+(15 rows)
+
-- Parallel sort but with expression (correlated subquery) that
-- is prohibited in parallel plans.
explain (costs off) select distinct
diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql
index 81429164d4..d8768a6b54 100644
--- a/src/test/regress/sql/incremental_sort.sql
+++ b/src/test/regress/sql/incremental_sort.sql
@@ -257,6 +257,13 @@ from tenk1, lateral (select tenk1.unique1 from generate_series(1, 1000)) as sub;
explain (costs off) select sub.unique1, md5(stringu1)
from tenk1, lateral (select tenk1.unique1 from generate_series(1, 1000)) as sub
order by 1, 2;
+-- Parallel sort with an aggregate that can be safely generated in parallel,
+-- but we can't sort by partial aggregate values.
+explain (costs off) select count(*)
+from tenk1 t1
+join tenk1 t2 on t1.unique1 = t2.unique2
+join tenk1 t3 on t2.unique1 = t3.unique1
+order by count(*);
-- Parallel sort but with expression (correlated subquery) that
-- is prohibited in parallel plans.
explain (costs off) select distinct
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index edba5e49a8..30728be85a 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2697,20 +2697,19 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel,
EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
/*
- * We can only build a sort for pathkeys which contain an EC
- * member in the current relation's target, so ignore any suffix
- * of the list as soon as we find a pathkey without an EC member
- * in the relation.
+ * We can only build a sort for pathkeys that contain a
+ * safe-to-compute-early EC member computable from the current
+ * relation's reltarget, so ignore the remainder of the list as
+ * soon as we find a pathkey without such a member.
*
- * By still returning the prefix of the pathkeys list that does
- * meet criteria of EC membership in the current relation, we
- * enable not just an incremental sort on the entirety of
- * query_pathkeys but also incremental sort below a JOIN.
+ * It's still worthwhile to return any prefix of the pathkeys list
+ * that meets this requirement, as we may be able to do an
+ * incremental sort.
*
- * If requested, ensure the expression is parallel safe too.
+ * If requested, ensure the sort expression is parallel-safe too.
*/
- if (!find_em_expr_usable_for_sorting_rel(root, pathkey_ec, rel,
- require_parallel_safe))
+ if (!relation_can_be_sorted_early(root, rel, pathkey_ec,
+ require_parallel_safe))
break;
npathkeys++;
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 6e87fba2aa..6f1abbe47d 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -961,18 +961,21 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
}
/*
- * Find an equivalence class member expression that can be used to build
- * a sort node using the provided relation; return NULL if no candidate.
+ * relation_can_be_sorted_early
+ * Can this relation be sorted on this EC before the final output step?
*
* To succeed, we must find an EC member that prepare_sort_from_pathkeys knows
* how to sort on, given the rel's reltarget as input. There are also a few
* additional constraints based on the fact that the desired sort will be done
- * within the scan/join part of the plan. Also, non-parallel-safe expressions
- * are ignored if 'require_parallel_safe'.
+ * "early", within the scan/join part of the plan. Also, non-parallel-safe
+ * expressions are ignored if 'require_parallel_safe'.
+ *
+ * At some point we might want to return the identified EquivalenceMember,
+ * but for now, callers only want to know if there is one.
*/
-Expr *
-find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
- RelOptInfo *rel, bool require_parallel_safe)
+bool
+relation_can_be_sorted_early(PlannerInfo *root, RelOptInfo *rel,
+ EquivalenceClass *ec, bool require_parallel_safe)
{
PathTarget *target = rel->reltarget;
EquivalenceMember *em;
@@ -982,7 +985,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
* Reject volatile ECs immediately; such sorts must always be postponed.
*/
if (ec->ec_has_volatile)
- return NULL;
+ return false;
/*
* Try to find an EM directly matching some reltarget member.
@@ -1012,7 +1015,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
!is_parallel_safe(root, (Node *) em->em_expr))
continue;
- return em->em_expr;
+ return true;
}
/*
@@ -1021,7 +1024,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
em = find_computable_ec_member(root, ec, target->exprs, rel->relids,
require_parallel_safe);
if (!em)
- return NULL;
+ return false;
/*
* Reject expressions involving set-returning functions, as those can't be
@@ -1030,9 +1033,9 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
* belong to multi-member ECs.)
*/
if (IS_SRF_CALL((Node *) em->em_expr))
- return NULL;
+ return false;
- return em->em_expr;
+ return true;
}
/*
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 888e85ff5b..f1d111063c 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -144,10 +144,9 @@ extern EquivalenceMember *find_computable_ec_member(PlannerInfo *root,
Relids relids,
bool require_parallel_safe);
extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
-extern Expr *find_em_expr_usable_for_sorting_rel(PlannerInfo *root,
- EquivalenceClass *ec,
- RelOptInfo *rel,
- bool require_parallel_safe);
+extern bool relation_can_be_sorted_early(PlannerInfo *root, RelOptInfo *rel,
+ EquivalenceClass *ec,
+ bool require_parallel_safe);
extern void generate_base_implied_equalities(PlannerInfo *root);
extern List *generate_join_implied_equalities(PlannerInfo *root,
Relids join_relids,
On Mon, Apr 19, 2021 at 7:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wrote: > > Anyway I'm now inclined to remove that behavior from > > find_computable_ec_member, and adjust comments accordingly. > > After some more testing, that seems like a good thing to do, > so here's a v4. This all looks good to me. James Coleman
Re: "could not find pathkey item to sort" for TPC-DS queries 94-96
От
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Дата:
Tom Lane <tgl@sss.pgh.pa.us> writes: > + /* We ignore binary-compatible relabeling on both ends */ > + while (expr && IsA(expr, RelabelType)) > + expr = ((RelabelType *) expr)->arg; There are 10 instances of this exact loop scattered around the codebase. Is it worth it turning it into a static inline function? - ilmari -- - Twitter seems more influential [than blogs] in the 'gets reported in the mainstream press' sense at least. - Matt McLeod - That'd be because the content of a tweet is easier to condense down to a mainstream media article. - Calle Dybedahl
Re: "could not find pathkey item to sort" for TPC-DS queries 94-96
От
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Дата:
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
>> + /* We ignore binary-compatible relabeling on both ends */
>> + while (expr && IsA(expr, RelabelType))
>> + expr = ((RelabelType *) expr)->arg;
>
> There are 10 instances of this exact loop scattered around the codebase.
> Is it worth it turning it into a static inline function?
Something like the attached, maybe?
- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen
From b481a41e494f169765ee204aa17ba60c16950455 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 20 Apr 2021 12:08:46 +0100
Subject: [PATCH] Create a function for stripping RelabelType nodes off an
expression
The exact same while loop was repeated 10 times across the codebase,
which makes it smell like time to refactor.
---
contrib/postgres_fdw/postgres_fdw.c | 7 ++-----
src/backend/nodes/nodeFuncs.c | 3 +--
src/backend/optimizer/path/equivclass.c | 4 +---
src/backend/optimizer/plan/createplan.c | 8 ++------
src/backend/optimizer/plan/initsplan.c | 12 +++++-------
src/backend/optimizer/util/tlist.c | 8 ++------
src/include/nodes/nodeFuncs.h | 9 +++++++++
7 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index c590f374c6..5c45b2b087 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -7203,8 +7203,7 @@ find_em_expr_for_input_target(PlannerInfo *root,
}
/* We ignore binary-compatible relabeling on both ends */
- while (expr && IsA(expr, RelabelType))
- expr = ((RelabelType *) expr)->arg;
+ expr = (Expr *) strip_relabeltype(expr);
/* Locate an EquivalenceClass member matching this expr, if any */
foreach(lc2, ec->ec_members)
@@ -7221,9 +7220,7 @@ find_em_expr_for_input_target(PlannerInfo *root,
continue;
/* Match if same expression (after stripping relabel) */
- em_expr = em->em_expr;
- while (em_expr && IsA(em_expr, RelabelType))
- em_expr = ((RelabelType *) em_expr)->arg;
+ em_expr = (Expr *) strip_relabeltype(em->em_expr);
if (equal(em_expr, expr))
return em->em_expr;
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index ff3dcc7b18..9918a4c12e 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -587,8 +587,7 @@ applyRelabelType(Node *arg, Oid rtype, int32 rtypmod, Oid rcollid,
* all but the top one, and must do so to ensure that semantically
* equivalent expressions are equal().
*/
- while (arg && IsA(arg, RelabelType))
- arg = (Node *) ((RelabelType *) arg)->arg;
+ arg = strip_relabeltype(arg);
if (arg && IsA(arg, Const))
{
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 0188c1e9a1..2f78998a82 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -2307,9 +2307,7 @@ match_eclasses_to_foreign_key_col(PlannerInfo *root,
continue; /* ignore children here */
/* EM must be a Var, possibly with RelabelType */
- var = (Var *) em->em_expr;
- while (var && IsA(var, RelabelType))
- var = (Var *) ((RelabelType *) var)->arg;
+ var = (Var *) strip_relabeltype(em->em_expr);
if (!(var && IsA(var, Var)))
continue;
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 22f10fa339..b13aa65244 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -6257,9 +6257,7 @@ find_ec_member_for_tle(EquivalenceClass *ec,
ListCell *lc;
/* We ignore binary-compatible relabeling on both ends */
- tlexpr = tle->expr;
- while (tlexpr && IsA(tlexpr, RelabelType))
- tlexpr = ((RelabelType *) tlexpr)->arg;
+ tlexpr = (Expr *) strip_relabeltype(tle->expr);
foreach(lc, ec->ec_members)
{
@@ -6281,9 +6279,7 @@ find_ec_member_for_tle(EquivalenceClass *ec,
continue;
/* Match if same expression (after stripping relabel) */
- emexpr = em->em_expr;
- while (emexpr && IsA(emexpr, RelabelType))
- emexpr = ((RelabelType *) emexpr)->arg;
+ emexpr = (Expr *) strip_relabeltype(em->em_expr);
if (equal(emexpr, tlexpr))
return em;
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index 3ac853d9ef..84805906e4 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2566,16 +2566,14 @@ match_foreign_keys_to_quals(PlannerInfo *root)
if (!IsA(clause, OpExpr) ||
list_length(clause->args) != 2)
continue;
- leftvar = (Var *) get_leftop((Expr *) clause);
- rightvar = (Var *) get_rightop((Expr *) clause);
- /* Operands must be Vars, possibly with RelabelType */
- while (leftvar && IsA(leftvar, RelabelType))
- leftvar = (Var *) ((RelabelType *) leftvar)->arg;
+ /* Type relabeling doesn't matter */
+ leftvar = (Var *) strip_relabeltype(get_leftop((Expr *) clause));
+ rightvar = (Var *) strip_relabeltype(get_rightop((Expr *) clause));
+
+ /* Operands must be Vars */
if (!(leftvar && IsA(leftvar, Var)))
continue;
- while (rightvar && IsA(rightvar, RelabelType))
- rightvar = (Var *) ((RelabelType *) rightvar)->arg;
if (!(rightvar && IsA(rightvar, Var)))
continue;
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 8a26288070..6ea3e1da03 100644
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/tlist.c
@@ -90,16 +90,12 @@ tlist_member_ignore_relabel(Expr *node, List *targetlist)
{
ListCell *temp;
- while (node && IsA(node, RelabelType))
- node = ((RelabelType *) node)->arg;
+ node = (Expr *) strip_relabeltype(node);
foreach(temp, targetlist)
{
TargetEntry *tlentry = (TargetEntry *) lfirst(temp);
- Expr *tlexpr = tlentry->expr;
-
- while (tlexpr && IsA(tlexpr, RelabelType))
- tlexpr = ((RelabelType *) tlexpr)->arg;
+ Expr *tlexpr = (Expr *) strip_relabeltype(tlentry->expr);
if (equal(node, tlexpr))
return tlentry;
diff --git a/src/include/nodes/nodeFuncs.h b/src/include/nodes/nodeFuncs.h
index 03a346c01d..b11649d283 100644
--- a/src/include/nodes/nodeFuncs.h
+++ b/src/include/nodes/nodeFuncs.h
@@ -126,6 +126,15 @@ get_notclausearg(const void *notclause)
return (Expr *) linitial(((const BoolExpr *) notclause)->args);
}
+/* Strip off any RelabelType nodes */
+static inline Node *
+strip_relabeltype(const void *node)
+{
+ while (node && IsA(node, RelabelType))
+ node = ((RelabelType *) node)->arg;
+ return (Node *) node;
+}
+
extern bool check_functions_in_node(Node *node, check_function_callback checker,
void *context);
--
2.29.2
On Tue, Apr 20, 2021 at 7:11 AM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > > ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: > > > Tom Lane <tgl@sss.pgh.pa.us> writes: > > > >> + /* We ignore binary-compatible relabeling on both ends */ > >> + while (expr && IsA(expr, RelabelType)) > >> + expr = ((RelabelType *) expr)->arg; > > > > There are 10 instances of this exact loop scattered around the codebase. > > Is it worth it turning it into a static inline function? > > Something like the attached, maybe? I'm not opposed to this, but I think it should go in a separate thread since it's orthogonal to the bugfix there and also will confuse cfbot. James
ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
>> There are 10 instances of this exact loop scattered around the codebase.
>> Is it worth it turning it into a static inline function?
> Something like the attached, maybe?
Meh. The trouble with this is that the call sites don't all declare
the pointer variable the same way. While the written-out loops can
look the same regardless, a function can only accommodate one choice
without messy casts. For my money, the notational savings here is
small enough that the casts really discourage doing anything.
So if we wanted to do this, I'd think about using a macro:
#define strip_relabeltype(nodeptr) \
while (nodeptr && IsA(nodeptr, RelabelType))
nodeptr = ((RelabelType *) nodeptr)->arg
...
strip_relabeltype(em_expr);
...
Since the argument would have to be a variable, the usual
multiple-reference hazards of using a macro don't seem to apply.
(Probably the macro could do with "do ... while" decoration
to discourage any syntactic oddities, but you get the idea.)
regards, tom lane
James Coleman <jtc331@gmail.com> writes:
> On Mon, Apr 19, 2021 at 7:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> After some more testing, that seems like a good thing to do,
>> so here's a v4.
> This all looks good to me.
Pushed, thanks for reviewing!
regards, tom lane