On Thu, Jan 29, 2026 at 1:44 AM Richard Guo <[email protected]> wrote: > I think we should access spath->disabled_nodes until after we have > verified that spath is not NULL.
Good catch. I have included a fix for this in 4020b370f214315b8c10430301898ac21658143f. > Also, it's not quite clear to me why create_material_path (and > cost_material) requires the "enabled" parameter. I couldn't find any > comments on this, so it might be worth adding some comments here. I think cost_material() got an enabled argument because of materialize_finished_plan(). Most of the time, plan nodes are created from paths, and we want to use the parent's pgs_mask to determine whether the chosen strategy (e.g. materialization) is enabled. However, materialize_finished_plan() creates a plan directly, so there's no RelOptInfo or JoinPathExtraData whose pgs_mask we can consult, and so we have to fall back on consulting enable_material directly. Once that parameter got added to cost_material(), it made sense to me to add it to create_material_path() as well. There are probably other ways I could have done this. For instance, we could make create_material_path() pass true to cost_material(), and not have its own "enabled" argument. That would mean that reparameterize_path() would have to unconditionally pass true. I'm not sure if that would be a problem. Another idea is to make cost_material() consult enable_material itself if path->parent is NULL, so that it wholly encapsulates the calculation locally. One thing that is a little different about materialization vs. some other operations is that it's used for multiple purposes. Sometimes we use it because it's required for correctness, and other times we do it to reduce cost. And, it can be used to affect cost in different scenarios. In the Nested Loop case, PGS_NESTLOOP_MATERIALIZE should control whether we are willing to materialize, but that doesn't apply to other cases, like build_subplan() or create_mergejoin_plan(). Moreover, these latter cases have not yet been converted to use Paths -- hopefully they will be converted someday, but maybe not soon. All of this makes things a little more complicated for materialization vs. something like a nested loop or a sequential scan, where the logic to create that thing is all in one place and paths are in use. But that is not to say that I'm deeply invested in cost_material() having an "enabled" argument -- that seems like a detail to me that could go either way. If it seems objectionable, we could change it, and I think we could probably do so in more than one way and without really affecting the behavior. Alternatively, we could leave it the way it is and add a comment clarifying whatever portion of the above you think is worth documenting in the source. It seemed like a sufficiently minor detail to me that I did not bother, but that might have been a mistake. -- Robert Haas EDB: http://www.enterprisedb.com
