On Sun, Aug 20, 2023 at 11:48 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> I looked over the v3 patch. I think it's going in generally > the right direction, but there is a huge oversight: > path_is_reparameterizable_by_child does not come close to modeling > the success/failure conditions of reparameterize_path_by_child. > In all the cases where reparameterize_path_by_child can recurse, > path_is_reparameterizable_by_child has to do so also, to check > whether the child path is reparameterizable. (I'm somewhat > disturbed that apparently we have no test cases that caught that > oversight; can we add one cheaply?) Thanks for pointing this out. It's my oversight. We have to check the child path(s) recursively to tell if a path is reparameterizable or not. I've fixed this in v4 patch, along with a test case. In the test case we have a MemoizePath whose subpath is TidPath, which is not reparameterizable. This test case would trigger Assert in v3 patch. > BTW, I also note from the cfbot that 9e9931d2b broke this patch by > adding more ADJUST_CHILD_ATTRS calls that need to be modified. > I wonder if we could get away with having that macro cast to "void *" > to avoid needing to change all its call sites. I'm not sure whether > pickier compilers might warn about doing it that way. Agreed and have done that in v4 patch. Thanks Richard
v4-0001-Postpone-reparameterization-of-paths-until-when-creating-plans.patch
Description: Binary data
