Re: Crash in add_paths_to_append_rel
On Tue, Oct 10, 2023 at 11:52 AM David Rowley wrote: > On Mon, 9 Oct 2023 at 22:49, Richard Guo wrote: > > I came across a crash in add_paths_to_append_rel() with the query below. > > > It was introduced by commit a8a968a8 where we referenced > > cheapest_startup_path->param_info without first checking that we have a > > valid cheapest_startup_path. > > I pushed this with just minor adjustments to the comments you wrote. I > didn't feel the need to reiterate on the set_cheapest() comments. Thanks for pushing! Thanks Richard
Re: Crash in add_paths_to_append_rel
On Mon, Oct 9, 2023 at 7:35 PM David Rowley wrote: > Since you've managed to attribute this to a specific commit, for the > future, I think instead of opening a new thread, it would be more > useful to communicate the issue on the thread that's linked in the > commit message. Ah, yes, I should have done it that way, sorry. Thanks Richard
Re: Crash in add_paths_to_append_rel
On Tue, Oct 10, 2023 at 11:52 AM David Rowley wrote: > On Mon, 9 Oct 2023 at 22:49, Richard Guo wrote: > > I came across a crash in add_paths_to_append_rel() with the query below. > > > It was introduced by commit a8a968a8 where we referenced > > cheapest_startup_path->param_info without first checking that we have a > > valid cheapest_startup_path. > > I pushed this with just minor adjustments to the comments you wrote. I > didn't feel the need to reiterate on the set_cheapest() comments. > > Thanks Richard for the report and also Thanks you David for the push! -- Best Regards Andy Fan
Re: Crash in add_paths_to_append_rel
On Mon, 9 Oct 2023 at 22:49, Richard Guo wrote: > I came across a crash in add_paths_to_append_rel() with the query below. > It was introduced by commit a8a968a8 where we referenced > cheapest_startup_path->param_info without first checking that we have a > valid cheapest_startup_path. I pushed this with just minor adjustments to the comments you wrote. I didn't feel the need to reiterate on the set_cheapest() comments. David
Re: Crash in add_paths_to_append_rel
On Mon, 9 Oct 2023 at 22:49, Richard Guo wrote: > I came across a crash in add_paths_to_append_rel() with the query below. > It was introduced by commit a8a968a8 where we referenced > cheapest_startup_path->param_info without first checking that we have a > valid cheapest_startup_path. Thank you for the report. Since you've managed to attribute this to a specific commit, for the future, I think instead of opening a new thread, it would be more useful to communicate the issue on the thread that's linked in the commit message. I will look at this tomorrow. David
Crash in add_paths_to_append_rel
I came across a crash in add_paths_to_append_rel() with the query below. create table t (a int); create table inh (b int); create table inh_child() inherits(inh); explain (costs off) select * from t left join lateral (select t.a from inh) on true limit 1; server closed the connection unexpectedly It was introduced by commit a8a968a8 where we referenced cheapest_startup_path->param_info without first checking that we have a valid cheapest_startup_path. We do have checked that the pathlist is not empty, but that does not imply that the cheapest_startup_path is not NULL. Normally only unparameterized paths are considered candidates for cheapest_startup_path, because startup cost does not have too much value for parameterized paths since they are on the inside of a nestloop. So if there are no unparameterized paths, the cheapest_startup_path will be NULL. That is the case in the repro query above. Because of the lateral reference within PHV, the child rels of 'inh' do not have unparameterized paths, so their cheapest_startup_path is NULL. I think we should explicitly check that cheapest_startup_path is not NULL here. Doing that seems to make the check of pathlist not being empty unnecessary. Besides, I think we do not need to check that cheapest_startup_path->param_info is NULL, since cheapest_startup_path must be unparameterized path. Maybe we can use an Assert instead. Attached is my proposed fix. Thanks Richard v1-0001-Don-t-assume-that-cheapest_startup_path-exists.patch Description: Binary data
