David Rowley <dgrowle...@gmail.com> writes: > I think fixing it your way makes sense. I don't really see any reason > why we should have two. However...
> Another way it *could* be fixed would be to get rid of pull_paramids() > and change create_memoize_plan() to set keyparamids to all the param > IDs that match are equal() to each param_exprs. That way nodeMemoize > wouldn't purge the cache as we'd know the changing param is accounted > for in the cache. For the record, I don't think that's better, but it > scares me a bit less as I don't know what other repercussions there > are of applying your patch to get rid of the duplicate > NestLoopParam.paramval. > I'd feel better about doing it your way if Tom could comment on if > there was a reason he put the function calls that way around in > 5ebaaa494. I'm fairly sure I thought it wouldn't matter because of the Param de-duplication done in paramassign.c. However, Richard's example shows that's not so, because process_subquery_nestloop_params is picky about the param ID assigned to a particular Var while replace_nestloop_params is not. So flipping the order makes sense. I'd change the comment though, maybe to /* * Replace any outer-relation variables with nestloop params. * * We must provide nestloop params for both lateral references of * the subquery and outer vars in the scan_clauses. It's better * to assign the former first, because that code path requires * specific param IDs, while replace_nestloop_params can adapt * to the IDs assigned by process_subquery_nestloop_params. * This avoids possibly duplicating nestloop params when the * same Var is needed for both reasons. */ However ... it seems like we're not out of the woods yet. Why is Richard's proposed test case still showing + -> Memoize (actual rows=5000 loops=N) + Cache Key: t1.two, t1.two Seems like there is missing de-duplication logic, or something. > I also feel like we might be getting a bit close to the minor version > releases to be adjusting this stuff in back branches. Yeah, I'm not sure I would change this in the back branches. regards, tom lane