Re: set_cheapest without checking pathlist

On Thu, Feb 1, 2024 at 1:36 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
>
> On Thu, Feb 1, 2024 at 11:37 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>>
>> On Thu, 1 Feb 2024 at 16:29, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>> > On Thu, Feb 1, 2024 at 10:04 AM James Coleman <jtc331(at)gmail(dot)com> wrote:
>> >> I don't see any inherent reason why we must always assume that
>> >> gather_grouping_paths will always result in having at least one entry
>> >> in pathlist. If, for example, we've disabled incremental sort and the
>> >> cheapest partial path happens to already be sorted, then I don't
>> >> believe we'll add any paths. And if that happens then set_cheapest
>> >> will error with the message "could not devise a query plan for the
>> >> given query". So I propose we add a similar guard to this call point.
>> >
>> >
>> > I don't believe that would happen. It seems to me that we should, at
>> > the very least, have a path which is Gather on top of the cheapest
>> > partial path (see generate_gather_paths), as long as the
>> > partially_grouped_rel has partial paths.
>>
>> It will have partial paths because it's nested inside "if
>> (partially_grouped_rel && partially_grouped_rel->partial_pathlist)"
>
>
> Right. And that leads to the conclusion that gather_grouping_paths will
> always generate at least one path for partially_grouped_rel. So it
> seems to me that the check added in the patch is not necessary. But
> maybe we can add an Assert or a comment clarifying that the pathlist of
> partially_grouped_rel cannot be empty here.

Yes, that's true currently. I don't particularly love that we have the
knowledge of that baked in at this level, but it won't break anything
currently.

I'll put this as a patch in the parallelization patch series
referenced earlier since in that series my changes result in
generate_gather_paths() not necessarily always being able to build a
path.

Regards,
James Coleman