Re: set_cheapest without checking pathlist
On Thu, Feb 1, 2024 at 1:36 AM Richard Guo wrote: > > > On Thu, Feb 1, 2024 at 11:37 AM David Rowley wrote: >> >> On Thu, 1 Feb 2024 at 16:29, Richard Guo wrote: >> > On Thu, Feb 1, 2024 at 10:04 AM James Coleman 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
Re: set_cheapest without checking pathlist
On Thu, Feb 1, 2024 at 5:03 PM David Rowley wrote: > On Thu, 1 Feb 2024 at 19:36, Richard Guo 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. Fair point. Thanks Richard
Re: set_cheapest without checking pathlist
On Thu, 1 Feb 2024 at 19:36, Richard Guo 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
Re: set_cheapest without checking pathlist
On Thu, Feb 1, 2024 at 11:37 AM David Rowley wrote: > On Thu, 1 Feb 2024 at 16:29, Richard Guo wrote: > > On Thu, Feb 1, 2024 at 10:04 AM James Coleman 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. Thanks Richard
Re: set_cheapest without checking pathlist
On Thu, 1 Feb 2024 at 16:29, Richard Guo wrote: > > > On Thu, Feb 1, 2024 at 10:04 AM James Coleman 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
Re: set_cheapest without checking pathlist
On Thu, Feb 1, 2024 at 10:04 AM James Coleman 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. Thanks Richard
