Hi, I've looked at the rebased patch you sent yesterday, and I do have a couple of comments.
1) There seems to be forgotten declaration of initArrayResultInternal in arrayfuncs.c. I suppose you've renamed it to initArrayResultWithSize and moved it to a header file, and forgotten to remove this bit. 2) bytea_string_agg_deserialize has this comment: /* * data: technically we could reuse the buf.data buffer, but that is * slightly larger than we need due to the extra 4 bytes for the cursor */ I find the argument "it has 4 extra bytes, so we'll allocate new chunk" somewhat weak. We do allocate the memory in 2^n chunks anyway, so the new chunk is likely to be much larger anyway. I'm not saying we need to reuse the buffer, IMHO the savings will be non-measurable. 3) string_agg_transfn and bytea_string_agg_transfn say this" /* * We always append the delimiter text before the value text, even * with the first aggregated item. The reason for this is that if * this state needs to be combined with another state using the * aggregate's combine function, then we need to have the delimiter * for the first aggregated item. The first delimiter will be * stripped off in the final function anyway. We use a little cheat * here and store the position of the actual first value (after the * delimiter) using the StringInfo's cursor variable. This relies on * the cursor not being used for any other purpose. */ How does this make the code any simpler, compared to not adding the delimiter (as before) and adding it when combining the values (at which point you should know when it's actually needed)? regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
