Обсуждение: set_cheapest without checking pathlist

Hello,

Robert: I've taken the liberty of cc'ing you since you worked on most
of this code. My apologies if that wasn't appropriate.

While working on "Parallelize correlated subqueries that execute
within each worker" [1] I noticed that while in the other call to
set_cheapest (for partially_grouped_rel) in the same function the call
after gather_grouping_paths(root, partially_grouped_rel) is not
similarly guarded with a check for a NIL pathlist on
partially_grouped_rel.

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 could be convinced that this should be simply part of the patch in
the other thread, but it seemed to me it'd be worth considering
independently because as noted above I don't see any reason why this
couldn't happen separately. That being said, on master I don't have a
case showing this is necessary.

Thanks,
James Coleman

1:
https://www.postgresql.org/message-id/flat/CAAaqYe-Aq6oNf9NPZnpPE7SgRLomXXWJA1Gz9L9ndi_Nv%3D94Yw%40mail.gmail.com#e0b1a803d0fdb97189ce493f15f99c14

On Thu, 1 Feb 2024 at 16:29, Richard Guo <guofenglinux@gmail.com> wrote:
>
>
> On Thu, Feb 1, 2024 at 10:04 AM James Coleman <jtc331@gmail.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)"

David

On Thu, 1 Feb 2024 at 19:36, Richard Guo <guofenglinux@gmail.com> wrote:
> maybe we can add an Assert or a comment clarifying that the pathlist of
> partially_grouped_rel cannot be empty here.

There'd be no need to Assert that as set_cheapest() will raise an
error when given a rel with an empty pathlist.

I don't think putting an Assert that would fail in the same situation
that we'd later ERROR out on would be a good idea. It'd make it harder
to debug the issue.

David

On Thu, Feb 1, 2024 at 1:36 AM Richard Guo <guofenglinux@gmail.com> wrote:
>
>
> On Thu, Feb 1, 2024 at 11:37 AM David Rowley <dgrowleyml@gmail.com> wrote:
>>
>> On Thu, 1 Feb 2024 at 16:29, Richard Guo <guofenglinux@gmail.com> wrote:
>> > On Thu, Feb 1, 2024 at 10:04 AM James Coleman <jtc331@gmail.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