On Mon, 30 Mar 2026 at 06:16, Melanie Plageman <[email protected]> wrote: > Attached v48 does a bit more cleanup. No functional changes. I'm > planning to push this soon. I think my remaining question is whether I > should move the row marks and result relation bitmaps into the estate. > I'm leaning toward not doing that and leaving them in the PlannedStmt. > Anyway, If I want to replace the list of result relation RTIs in the > PlannedStmt, I have to leave the bitmapset version there.
I looked at v48-0001 and it looks fine to me. I've only minor quibbles about you using foreach() instead of foreach_int() and foreach_node() for populating the new Bitmapsets in standard_planner(). I don't see any advantage to adding the fields to EState. There might be if there was some performance reason, but it looks like you're only accessing the fields when scans are initialised. It's hard to imagine an extra pointer deference would matter there. I didn't find any guidance in any comments to understand if there's a best practise here, so I assume what's there today is down to people's taste. For me, I'd say if it's not performance critical and the executor does not modify the field for any purpose, then keeping it in PlannedStmt is fine. If someone thinks I'm wrong on that, then a comment at the top of EState would be helpful. David
