On Fri, Sep 12, 2025 at 6:34 PM Melanie Plageman <melanieplage...@gmail.com> wrote: > You're missing a word above -- like "modules to store their own" > This path does not reflect where you put the file
Thanks. > Storing a uint32 in a signed int that could be 32-bit stuck out to me. "git grep pg_nextpower2_32" finds examples of assigning the result to both "int" and "uint32", and I see no practical risk here. > Since you don't copy the extension name, it might be worth mentioning > that the caller should provide a literal or at least something that > will be around later. Maybe, but there's no obvious reason for any caller to use anything other than a string literal. > You used a different variable name here than in the implementation for > the RelOptInfo parameter. Oops. > > 0002 removes join_search_private, as before. Whether it makes sense to > > go ahead with this is debatable. Needs review, and needs an opinion on > > whether this should be considered a PoC only (and discarded) or > > something that should go forward to commit. > > Is there a downside to going forward with it? I think it's just a stylistic preference, whether people like it this way better or not. > As for the code itself, I thought assumeReplanning was a bit vague > since it seems like whether or not replanning is allowed could come up > outside of join order search -- but perhaps that's okay. Yeah, there is room for bikeshedding that name. > > For another example of how these patches could be used, see > > http://postgr.es/m/CA+TgmoZ=6jji9tgyzcm33vads46hfkyz6aju_salt6gfs-i...@mail.gmail.com > > and in particular 0001 and 0002. This patch set's planner_setup_hook > > call would go write after those patches compute default_ssa_mask and > > default_jsa_mask, allowing the hook to override those values. > > So, are you saying that you would rewrite the patches in that set to > use the infrastructure in this set -- e.g. remove that set's > PlannerGlobal.default_jsa_mask and instead put it in > PlannerGlobal.extension_state? Or am I misunderstanding? No, that's not what I'm saying. What I'm saying is that with both patches applied, planner_setup_hook() from this patch ends up getting called right after default_jsa_mask is set, so another thing this hook can do is adjust that value. Or, for example, you can write a patch that uses this infrastructure to associate state with each RelOptInfo, and then you can use that state to decide how to set jsa_mask in join_path_setup_hook. In other words, it's easier to make effective use of those patches if you have the infrastructure provided by these patches. -- Robert Haas EDB: http://www.enterprisedb.com
