Richard Guo <[email protected]> writes: > I ran into a wrong-result issue on master (also reproducible on v16 > and later, I believe).
FWIW, this example reproduces for me on v17 and up, but not v16. It might be worth determining why. A quick bisect run says that it broke at 9f133763961e280d8ba692bcad0b061b861e9138 is the first bad commit commit 9f133763961e280d8ba692bcad0b061b861e9138 Author: Alexander Korotkov <[email protected]> Date: Thu Feb 15 12:05:52 2024 +0200 Pull up ANY-SUBLINK with the necessary lateral support. but I've not dug further than that. > I believe the same happens with any path that inherits its subpath's > ParamPathInfo: Memoize, Projection, Sort, IncrementalSort, Unique, Agg > and GroupingSets, although I don't have proof. Attached is a patch > that makes get_param_path_clause_serials() look through those paths, > and uses it in get_memoize_path() too, which reads ppi_serials > directly. I think you're on to something, but this patch has a hasty look to it. AFAICS, the root of the problem is that the wrapper-path-making functions in pathnode.c just do pathnode->path.param_info = subpath->param_info; and we now see that that isn't good enough. I wonder if we should try harder. At the very least I think it'd be advisable to make all of those look like, say, pathnode->path.param_info = get_wrapper_parampathinfo(subpath); Even if get_wrapper_parampathinfo doesn't do anything beyond handing back the subpath's param_info, it would provide a place for a comment discussing what it's doing and why. However, I feel like we should consider a redesign rather than just a minimal fix. In particular, it took me a bit to reconstruct why the code looks like it does now. I think it's this: (1) Joins need special handling in get_param_path_clause_serials because different join input pairs for the same result join may have different joinrestrictinfo lists and thus enforce different sets of rinfo serials; so we can't compute a ppi_serials value in advance. (2) AppendPath needs special handling in case there are joins under it, which would only occur for partitionwise joining. Otherwise we could have precomputed the correct ppi_serials to begin with. (I didn't check the code history, but I wonder if somebody rewrote this for partitionwise joining and did not bother to add a comment explaining that.) But if that is the situation, then what about a wrapper path that wraps a join? Seems like we need to be able to recurse and compute a context-dependent set of ppi_serials for that too. Now we could just recurse always, which is what your patch does, but that feels annoyingly inefficient. I am wondering about a redesign that would allow marking ParamPathInfos as context-dependent (if they are for a join or something with a join under it) or not (everything else). Then we only need to recurse when the hard cases occur, and otherwise we can just believe the ppi_serials field. Also, as a more nitpicky objection, I'd be inclined to refactor get_param_path_clause_serials so that the whole thing is one big switch over nodeTag(path), rather than the messy structure your v1 patch would leave behind. regards, tom lane
