On Wed, 17 Jul 2024 at 01:45, Alexander Korotkov <aekorot...@gmail.com> wrote: > > Jian He gave a try to ChangeVarNodes() [1]. That gives some > improvement, but the vast majority of complexity is still here. I > think the reason for complexity of SJE is that it's the first time we > remove relation, which is actually *used* and therefore might has > references in awful a lot of places. In previous cases we removed > relations, which were actually unused. >
I had a quick look at this, and I have a couple of comments on the rewriter changes. The new function replace_relid() looks to be the same as adjust_relid_set(). The changes to ChangeVarNodes() look a little messy. There's a lot of code duplicated between ChangeVarNodesExtended() and ChangeVarNodes(), which could be avoided by having one call the other. Also, it would be better for ChangeVarNodesExtended() to have a "flags" parameter instead of an extra boolean parameter, to make it more extensible in the future. However,... I question whether ChangeVarNodesExtended() and the changes to ChangeVarNodes() are really the right way to go about this. ChangeVarNodes() in particular gains a lot more logic to handle RestrictInfo nodes that doesn't really feel like it belongs there -- e.g., building NullTest nodes is really specific to SJE, and doesn't seem like it's something ChangeVarNodes() should be doing. A better solution might be to add a new walker function to analyzejoins.c that does just what SJE needs, which is different from ChangeVarNodes() in a number of ways. For Var nodes, it might ultimately be necessary to do more than just change the varno, to solve the RETURNING/EPQ problems. For RestrictInfo nodes, there's a lot of SJE-specific logic. The SJE code wants to ignore RangeTblRef nodes, but it could delegate to ChangeVarNodes() for various other node types to avoid code duplication. At the top level, the stuff that ChangeVarNodes() does to fields of the Query struct would be different for SJE, I think. Regards, Dean
