On Thu, 23 Apr 2020 at 02:37, Amit Langote <amitlangot...@gmail.com> wrote: > Regarding the patch, I had been assuming that the "pa" in > pa_subpaths_valid stands for "parallel append", so it using the > variable as is in the new code structure would be misleading. Maybe, > parallel_subpaths_valid?
I started making another pass over this patch again. I did the renaming you've mentioned plus the other two List variables for the subpaths. I did notice that I had forgotten to properly disable parallel append when it was disallowed by the rel's consider_parallel flag. For us to have done something wrong, we'd have needed a parallel safe path in the pathlist and also have needed the rel to be consider_parallel == false. It was easy enough to make up a case for that by sticking a parallel restrict function in the target list. I've fixed that issue in the attached and also polished up the comments a little and removed the unused variable that I had forgotten about. For now. I'd still like to get a bit more confidence that the only noticeable change in the outcome here is that we're now pulling up sub-Appends in all cases. I've read the code a number of times and just can't quite see any room for anything changing. My tests per Robert's case all matched what the previous version did, but I'm still only about 93% on this. Given that I'm aiming to fix a bug in master, v11 and v12 here, I need to get that confidence level up to about the 100% mark. I've attached the v2 patch. David
always_pullup_child_appends_to_top_level_append_v2.patch
Description: Binary data
