On 27 July 2015 at 20:11, Heikki Linnakangas <hlinn...@iki.fi> wrote:
> On 07/27/2015 08:34 AM, David Rowley wrote: > >> - * agg_input_types, agg_state_type, agg_result_type identify the input, >> - * transition, and result types of the aggregate. These should all be >> - * resolved to actual types (ie, none should ever be ANYELEMENT etc). >> + * agg_input_types identifies the input types of the aggregate. These >> should >> + * be resolved to actual types (ie, none should ever be ANYELEMENT etc). >> >> I'm not sure I understand why agg_state_type and agg_result_type have >> changed here. >> > > The function no longer has the agg_result_type argument, but the removal > of agg_state_type from the comment was a mistake. I've put agg_state_type back in the attached delta which is again based on your version of the patch. > > > + peraggstate->sortstates = (Tuplesortstate **) >> + palloc0(sizeof(Tuplesortstate *) * numGroupingSets); >> + for (currentsortno = 0; currentsortno < numGroupingSets; >> currentsortno++) >> + peraggstate->sortstates[currentsortno] = NULL; >> >> This was not you, but this NULL setting looks unneeded due to the >> palloc0(). >> > > Yeah, I noticed that too. Ok, let's take it out. > > Removed in attached. > In this function I also wasn't quite sure if it was with comparing both >> non-NULL INITCOND's here. I believe my code comments may slightly >> contradict what the code actually does, as the comments talk about them >> having to match, but the code just bails if any are non-NULL. The reason I >> didn't check them was because it seems inevitable that some duplicate work >> needs to be done when setting up the INITCOND. Perhaps it's worth it? >> > > It would be nice to handle non-NULL initconds. I think you'll have to > check that the input function isn't volatile. Or perhaps just call the > input function, and check that the resulting Datum is byte-per-byte > identical, although that might be awkward to do with the current code > structure. > > I've not done anything with this. I'd not thought of an input function being volatile before, but I guess it's possible, which makes me a bit scared that we could be treading on ground we shouldn't be. I know it's more of an output function thing than an input function thing, but a GUC like extra_float_digits could cause problems here. In summary, I'm much less confident it's safe to enable the optimisation in this case. > BTW, the name of the AggStatePerAggStateData struct is pretty horrible. >>> The repeated "AggState" feels awkward. Now that I've stared at the patch >>> for a some time, it doesn't bother me anymore, but it took me quite a >>> while >>> to over that. I'm sure it will for others too. And it's not just that >>> struct, the comments talk about "aggregate state", which could be >>> confused >>> to mean "AggState", but it actually means AggStatePerAggStateData. I >>> don't >>> have any great suggestions, but can you come up a better naming scheme? >>> >> >> I agree, they're horrible. The thing that's causing the biggest problem is >> the struct named AggState, which carries state for *all* aggregates, and >> has nothing to do with "transition state", so it seems there's two >> different meanings if the word "state" and I've used both meanings in the >> name for AggStatePerAggStateData. >> >> Perhaps just renaming AggStatePerAggStateData to AggStateTransStateData >> would be good enough? >> > > Hmm. I think it should be "AggStatePerTransData" then, to keep the same > pattern as AggStatePerAggData and AggStatePerGroupData. > > Sounds good. I've renamed it to that in the attached delta patch. Regards David Rowley -- David Rowley http://www.2ndQuadrant.com/ <http://www.2ndquadrant.com/> PostgreSQL Development, 24x7 Support, Training & Services
sharing_aggstate-heikki-1_delta2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
