Horton

Thread

Commits

  1. Fix possible crash in add_paths_to_append_rel()

  2. Consider cheap startup paths in add_paths_to_append_rel

  1. Crash in add_paths_to_append_rel

    Richard Guo <guofenglinux@gmail.com> — 2023-10-09T09:45:41Z

    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

  2. Re: Crash in add_paths_to_append_rel

    David Rowley <dgrowleyml@gmail.com> — 2023-10-09T11:35:38Z

    On Mon, 9 Oct 2023 at 22:49, Richard Guo <guofenglinux@gmail.com> 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

  3. Re: Crash in add_paths_to_append_rel

    David Rowley <dgrowleyml@gmail.com> — 2023-10-10T03:52:05Z

    On Mon, 9 Oct 2023 at 22:49, Richard Guo <guofenglinux@gmail.com> 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

  4. Re: Crash in add_paths_to_append_rel

    Andy Fan <zhihui.fan1213@gmail.com> — 2023-10-10T04:48:17Z

    On Tue, Oct 10, 2023 at 11:52 AM David Rowley <dgrowleyml@gmail.com> wrote:

> On Mon, 9 Oct 2023 at 22:49, Richard Guo <guofenglinux@gmail.com> 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

  5. Re: Crash in add_paths_to_append_rel

    Richard Guo <guofenglinux@gmail.com> — 2023-10-10T05:49:31Z

    On Mon, Oct 9, 2023 at 7:35 PM David Rowley <dgrowleyml@gmail.com> 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

  6. Re: Crash in add_paths_to_append_rel

    Richard Guo <guofenglinux@gmail.com> — 2023-10-10T05:50:15Z

    On Tue, Oct 10, 2023 at 11:52 AM David Rowley <dgrowleyml@gmail.com> wrote:

> On Mon, 9 Oct 2023 at 22:49, Richard Guo <guofenglinux@gmail.com> 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