On Wed, Mar 21, 2018 at 8:01 AM, Jeevan Chalke <jeevan.cha...@enterprisedb.com> wrote: >> In the patch as proposed, create_partial_grouping_paths() can get >> called even if GROUPING_CAN_PARTIAL_AGG is not set. I think that's >> wrong. > > I don't think so. For parallel case, we do check that. And for partitionwise > aggregation check, it was checked inside can_partitionwise_grouping() > function and flags were set accordingly. Am I missing something?
Well, one of us is missing something somewhere. If GROUPING_CAN_PARTIAL_AGG means that we're allowed to do partial grouping, and if create_partial_grouping_paths() is where partial grouping happens, then we should only be calling the latter if the former is set. I mean, how can it make sense to create partially-grouped paths if we're not allowed to do partial grouping? > I have tweaked these conditions and posted in a separate patch (0006). > However, I have merged all your three patches in one (0005). OK, thanks. I wasn't sure I had understood what was going on, so thanks for checking it. Thanks also for keeping 0004-0006 separate here, but I think you can flatten them into one patch in the next version. >> I think that if the last test in can_partitionwise_grouping were moved >> before the previous test, it could be simplified to test only >> (extra->flags & GROUPING_CAN_PARTIAL_AGG) == 0 and not >> *perform_partial_partitionwise_aggregation. > > I think we can't do this way. If *perform_partial_partitionwise_aggregation > found to be true then only we need to check whether partial aggregation > itself is possible or not. If we are going to perform a full partitionwise > aggregation then test for can_partial_agg is not needed. Have I misread your > comments? It seems you're correct, because when I change it the tests fail. I don't yet understand why. Basically, the main patch seems to use three Boolean signaling mechanisms: 1. GROUPING_CAN_PARTITIONWISE_AGG 2. is_partial_aggregation 3. perform_partial_partitionwise_aggregation Stuff I don't understand: - Why is one of them a Boolean shoved into "flags", even though it's not static across the whole hierarchy like the other flags, and the other two are separate Booleans? - What do they all do, anyway? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
