On 11/29/20 3:26 PM, James Coleman wrote: > On Mon, Nov 23, 2020 at 8:35 AM James Coleman <jtc...@gmail.com> wrote: >> >> On Sun, Nov 22, 2020 at 4:59 PM Tomas Vondra >> <tomas.von...@enterprisedb.com> wrote: >>> ... >>> Thanks for the patches, I'll take a closer look next week. The 0002 >>> patch is clearly something we need to backpatch, not sure about 0001 as >>> it essentially enables new behavior in some cases (Sort on unsorted >>> paths below Gather Merge). >> >> Thanks for taking a look. >> >> I had the same question re: backporting. On the one hand it will >> change behavior (this is always a bit of a gray area since in one >> sense bugs change behavior), but IMO it's not a new feature, because >> the code clearly intended to have that feature in the first place (it >> creates gather merges on top of a full sort). So I'd lean towards >> considering it a bug fix, but I'm also not going to make that a hill >> to die on. >> >> One semi-related question: do you think we should add a comment to >> prepare_sort_from_pathkeys explaining that modifying it may mean >> find_em_expr_for_rel needs to be updated also? > > Here's an updated patch series. > > 0001 and 0002 as before, but with some minor cleanup. >
0001 - seems fine 0002 - Should we rename the "parallel" parameter to something more descriptive, like "require_parallel_safe"? > 0003 disallows SRFs in these sort expressions (per discussion over at [1]). > OK, but I'd move the define from tlist.c to tlist.h, not optimizer.h. > 0004 removes the search through the target for matching volatile > expressions (per discussion at [2]). > OK > 0005 adds the comment I mentioned above clarifying these two functions > are linked. > Makes sense. I wonder if we need to be more specific about how consistent those two places need to be. Do they need to match 1:1, or how do people in the future decide which restrictions need to be in both places? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
