Re: Some revises in adding sorting path

On Thu, Dec 28, 2023 at 4:01 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Thu, Dec 28, 2023 at 4:00 PM Richard Guo <guofenglinux@gmail.com> wrote:
> >
> >
> > On Wed, Mar 29, 2023 at 4:00 AM David Rowley <dgrowleyml@gmail.com> wrote:
> >>
> >> If you write some tests for this code, it will be useful to prove that
> >> it actually does something, and also that it does not break again in
> >> the future. I don't really want to just blindly copy the pattern used
> >> in 3c6fc5820 for creating incremental sort paths if it's not useful
> >> here. It would be good to see tests that make an Incremental Sort path
> >> using the code you're changing.
> >
> >
> > Thanks for the suggestion.  I've managed to come up with a query that
> > gets the codes being changed here to perform an incremental sort.
> >
> > set min_parallel_index_scan_size to 0;
> > set enable_seqscan to off;
> >
> > Without making those parallel paths:
> >
> > explain (costs off)
> > select * from tenk1 where four = 2 order by four, hundred, parallel_safe_volatile(thousand);
> >                           QUERY PLAN
> > --------------------------------------------------------------
> >  Incremental Sort
> >    Sort Key: hundred, (parallel_safe_volatile(thousand))
> >    Presorted Key: hundred
> >    ->  Gather Merge
> >          Workers Planned: 3
> >          ->  Parallel Index Scan using tenk1_hundred on tenk1
> >                Filter: (four = 2)
> > (7 rows)
> >
> > and with those parallel paths:
> >
> > explain (costs off)
> > select * from tenk1 where four = 2 order by four, hundred, parallel_safe_volatile(thousand);
> >                           QUERY PLAN
> > ---------------------------------------------------------------
> >  Gather Merge
> >    Workers Planned: 3
> >    ->  Incremental Sort
> >          Sort Key: hundred, (parallel_safe_volatile(thousand))
> >          Presorted Key: hundred
> >          ->  Parallel Index Scan using tenk1_hundred on tenk1
> >                Filter: (four = 2)
> > (7 rows)
> >
> > I've added two tests for the code changes in create_ordered_paths in the
> > new patch.
> >
> >>
> >> Same for the 0003 patch.
> >
> >
> > For the code changes in gather_grouping_paths, I've managed to come up
> > with a query that makes an explicit Sort atop cheapest partial path.
> >
> > explain (costs off)
> > select count(*) from tenk1 group by twenty, parallel_safe_volatile(two);
> >                              QUERY PLAN
> > --------------------------------------------------------------------
> >  Finalize GroupAggregate
> >    Group Key: twenty, (parallel_safe_volatile(two))
> >    ->  Gather Merge
> >          Workers Planned: 4
> >          ->  Sort
> >                Sort Key: twenty, (parallel_safe_volatile(two))
> >                ->  Partial HashAggregate
> >                      Group Key: twenty, parallel_safe_volatile(two)
> >                      ->  Parallel Seq Scan on tenk1
> > (9 rows)
> >
> > Without this logic the plan would look like:
> >
> > explain (costs off)
> > select count(*) from tenk1 group by twenty, parallel_safe_volatile(two);
> >                              QUERY PLAN
> > --------------------------------------------------------------------
> >  Finalize GroupAggregate
> >    Group Key: twenty, (parallel_safe_volatile(two))
> >    ->  Sort
> >          Sort Key: twenty, (parallel_safe_volatile(two))
> >          ->  Gather
> >                Workers Planned: 4
> >                ->  Partial HashAggregate
> >                      Group Key: twenty, parallel_safe_volatile(two)
> >                      ->  Parallel Seq Scan on tenk1
> > (9 rows)
> >
> > This test is also added in the new patch.
> >
> > But I did not find a query that makes an incremental sort in this case.
> > After trying for a while it seems to me that we do not need to consider
> > incremental sort in this case, because for a partial path of a grouped
> > or partially grouped relation, it is either unordered (HashAggregate or
> > Append), or it has been ordered by the group_pathkeys (GroupAggregate).
> > It seems there is no case that we'd have a partial path that is
> > partially sorted.
> >
Just for clarity; I am not familiar with the code. And for the review,
I ran 'make check' and 'make check-world' and all the test cases
passed successfully.

Thanks and Regards,
Shubham Khanna.