[sqlsmith] Crash in apply_projection_to_path

Started by Andreas Seltenreichover 9 years ago7 messages
#1Andreas Seltenreich
seltenreich@gmx.de

Hi,

the following query against the regression database crashes master as of
23b09e15.

select 1 from depth0 inner join depth1 on (depth0.c = depth1.c)
where depth0.c @@ depth1.c limit 1;

regards,
Andreas

Program terminated with signal SIGSEGV, Segmentation fault.
#0 create_projection_path (root=root@entry=0x6918e60,
rel=0x7f7f7f7f7f7f7f7f, subpath=0x69d6de8, target=target@entry=0x69d7428)
at pathnode.c:2160
2160 pathnode->path.parallel_safe = rel->consider_parallel &&
#0 create_projection_path (root=root@entry=0x6918e60, rel=0x7f7f7f7f7f7f7f7f, subpath=0x69d6de8, target=target@entry=0x69d7428) at pathnode.c:2160
#1 0x000000000067841e in apply_projection_to_path (root=0x6918e60, rel=0x69d5e18, path=0x69d6ee0, target=0x69d7428) at pathnode.c:2251
#2 0x000000000065ff20 in grouping_planner (root=0x22e1850, root@entry=0x6918e60, inheritance_update=56 '8', inheritance_update@entry=0 '\000', tuple_fraction=2615.2579411764709, tuple_fraction@entry=0) at planner.c:1737
#3 0x0000000000661f44 in subquery_planner (glob=glob@entry=0x6918dc8, parse=parse@entry=0x235e3b0, parent_root=parent_root@entry=0x0, hasRecursion=hasRecursion@entry=0 '\000', tuple_fraction=tuple_fraction@entry=0) at planner.c:758
#4 0x0000000000662e2b in standard_planner (parse=0x235e3b0, cursorOptions=256, boundParams=0x0) at planner.c:307
#5 0x00000000006f40ed in pg_plan_query (querytree=0x235e3b0, cursorOptions=256, boundParams=0x0) at postgres.c:798
#6 0x00000000006f41e4 in pg_plan_queries (querytrees=<optimized out>, cursorOptions=256, boundParams=0x0) at postgres.c:857
#7 0x00000000006f5c93 in exec_simple_query (query_string=<optimized out>) at postgres.c:1022

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andreas Seltenreich (#1)
Re: [sqlsmith] Crash in apply_projection_to_path

Andreas Seltenreich <seltenreich@gmx.de> writes:

the following query against the regression database crashes master as of
23b09e15.

select 1 from depth0 inner join depth1 on (depth0.c = depth1.c)
where depth0.c @@ depth1.c limit 1;

What's going on here is that add_partial_path is recycling a now-dominated
partial path without regard for the fact that there's already a GatherPath
pointing at that old partial path. Later use of the GatherPath tries to
make use of its now-dangling subpath pointer.

I'd be inclined to think that it's silly to build GatherPaths in advance
of having finalized the list of partial paths for a rel.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#2)
Re: [sqlsmith] Crash in apply_projection_to_path

On Thu, Apr 28, 2016 at 10:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andreas Seltenreich <seltenreich@gmx.de> writes:

the following query against the regression database crashes master as of
23b09e15.

select 1 from depth0 inner join depth1 on (depth0.c = depth1.c)
where depth0.c @@ depth1.c limit 1;

What's going on here is that add_partial_path is recycling a now-dominated
partial path without regard for the fact that there's already a GatherPath
pointing at that old partial path. Later use of the GatherPath tries to
make use of its now-dangling subpath pointer.

I'd be inclined to think that it's silly to build GatherPaths in advance
of having finalized the list of partial paths for a rel.

Uh ... yeah, that shouldn't be happening. I obviously screwed something up.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Amit Kapila
amit.kapila16@gmail.com
In reply to: Robert Haas (#3)
1 attachment(s)
Re: [sqlsmith] Crash in apply_projection_to_path

On Fri, Apr 29, 2016 at 8:07 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Apr 28, 2016 at 10:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andreas Seltenreich <seltenreich@gmx.de> writes:

the following query against the regression database crashes master as

of

23b09e15.

select 1 from depth0 inner join depth1 on (depth0.c = depth1.c)
where depth0.c @@ depth1.c limit 1;

What's going on here is that add_partial_path is recycling a

now-dominated

partial path without regard for the fact that there's already a

GatherPath

pointing at that old partial path. Later use of the GatherPath tries to
make use of its now-dangling subpath pointer.

I'd be inclined to think that it's silly to build GatherPaths in advance
of having finalized the list of partial paths for a rel.

Uh ... yeah, that shouldn't be happening. I obviously screwed something

up.

What's happening here is that to form joinrel, we need to call
add_paths_to_joinrel() with both outer and inner relation twice, once with
rel1 as outer relation and rel1 as inner relation and vice versa. So now
the second call to add_paths_to_joinrel() can replace a partial path which
is being referenced by GatherPath generated in first call. I think we
should generate gather paths for join rel after both the calls
to add_paths_to_joinrel() aka in make_join_rel(). Attached patch on above
lines fixes the problem for me.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Attachments:

fix_parallel_join_path_v1.patchapplication/octet-stream; name=fix_parallel_join_path_v1.patchDownload
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index f8e02b9..9d06fb2 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -223,12 +223,7 @@ add_paths_to_joinrel(PlannerInfo *root,
 												 jointype, &extra);
 
 	/*
-	 * 6. Consider gathering partial paths.
-	 */
-	generate_gather_paths(root, joinrel);
-
-	/*
-	 * 7. Finally, give extensions a chance to manipulate the path list.
+	 * 6. Finally, give extensions a chance to manipulate the path list.
 	 */
 	if (set_join_pathlist_hook)
 		set_join_pathlist_hook(root, joinrel, outerrel, innerrel,
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 01d4fea..1b72b46 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -869,6 +869,12 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2)
 			break;
 	}
 
+	/*
+	 * Consider gathering partial paths.  This has to be done after
+	 * generating all the partial paths for a joinrel.
+	 */
+	generate_gather_paths(root, joinrel);
+
 	bms_free(joinrelids);
 
 	return joinrel;
#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Amit Kapila (#4)
Re: [sqlsmith] Crash in apply_projection_to_path

Amit Kapila <amit.kapila16@gmail.com> writes:

On Thu, Apr 28, 2016 at 10:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd be inclined to think that it's silly to build GatherPaths in advance
of having finalized the list of partial paths for a rel.

What's happening here is that to form joinrel, we need to call
add_paths_to_joinrel() with both outer and inner relation twice, once with
rel1 as outer relation and rel1 as inner relation and vice versa. So now
the second call to add_paths_to_joinrel() can replace a partial path which
is being referenced by GatherPath generated in first call. I think we
should generate gather paths for join rel after both the calls
to add_paths_to_joinrel() aka in make_join_rel(). Attached patch on above
lines fixes the problem for me.

make_join_rel is certainly not far enough down the call stack to solve
this problem. It can, and typically will, be invoked multiple times
for the same target join relation.

One possible answer is to do it in standard_join_search, just before
the set_cheapest call for each join relation. You'd need to account
for the issue in GEQO search as well.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Amit Kapila
amit.kapila16@gmail.com
In reply to: Tom Lane (#5)
Re: [sqlsmith] Crash in apply_projection_to_path

On Fri, Apr 29, 2016 at 7:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Kapila <amit.kapila16@gmail.com> writes:

On Thu, Apr 28, 2016 at 10:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd be inclined to think that it's silly to build GatherPaths in

advance

of having finalized the list of partial paths for a rel.

What's happening here is that to form joinrel, we need to call
add_paths_to_joinrel() with both outer and inner relation twice, once

with

rel1 as outer relation and rel1 as inner relation and vice versa. So

now

the second call to add_paths_to_joinrel() can replace a partial path

which

is being referenced by GatherPath generated in first call. I think we
should generate gather paths for join rel after both the calls
to add_paths_to_joinrel() aka in make_join_rel(). Attached patch on

above

lines fixes the problem for me.

make_join_rel is certainly not far enough down the call stack to solve
this problem. It can, and typically will, be invoked multiple times
for the same target join relation.

One possible answer is to do it in standard_join_search, just before
the set_cheapest call for each join relation.

Yes, that makes sense to me.

You'd need to account
for the issue in GEQO search as well.

How about doing it in merge_clump() before calling set_cheapest()?

Yet, another idea could be to create a copy of partial path before passing
it to create_gather_path()?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Amit Kapila (#6)
Re: [sqlsmith] Crash in apply_projection_to_path

Amit Kapila <amit.kapila16@gmail.com> writes:

On Fri, Apr 29, 2016 at 7:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

make_join_rel is certainly not far enough down the call stack to solve
this problem. It can, and typically will, be invoked multiple times
for the same target join relation.

One possible answer is to do it in standard_join_search, just before
the set_cheapest call for each join relation.

Yes, that makes sense to me.

Done that way.

Yet, another idea could be to create a copy of partial path before passing
it to create_gather_path()?

That's just a kluge, and I'm not exactly convinced it'd solve the problem
anyway, unless you wanted to recursively copy the entire infrastructure
of the partial path. What's needed is to set rules for how we build paths
up in an orderly fashion. Such rules already existed for regular paths
(see the comments I added in c45bf5751), and now they exist for parallel
paths as well.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers