On Sun, Aug 20, 2023 at 11:48 PM Tom Lane <tgl@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.