2014-10-25 12:20 GMT+02:00 Ali Akbar <the.ap...@gmail.com>: > 2014-10-25 15:43 GMT+07:00 Pavel Stehule <pavel.steh...@gmail.com>: > >> >> >> 2014-10-25 10:16 GMT+02:00 Ali Akbar <the.ap...@gmail.com>: >> >>> makeArrayResult1 - I have no better name now >>>>> >>>>> I found one next minor detail. >>>>> >>>>> you reuse a array_agg_transfn function. Inside is a message >>>>> "array_agg_transfn called in non-aggregate context". It is not correct for >>>>> array_agg_anyarray_transfn >>>>> >>>> >>> Fixed. >>> >>> >>>> probably specification dim and lbs in array_agg_finalfn is useless, >>>>> when you expect a natural result. A automatic similar to >>>>> array_agg_anyarray_finalfn should work there too. >>>>> >>>>> makeMdArrayResultArray and appendArrayDatum should be marked as >>>>> "static" >>>>> >>>> >>> ok, marked all of that as static. >>> >>> >>>> I am thinking so change of makeArrayResult is wrong. It is developed >>>> for 1D array. When we expect somewhere ND result now, then we should to use >>>> makeMdArrayResult there. So makeArrayResult should to return 1D array in >>>> all cases. Else a difference between makeArrayResult and makeMdArrayResult >>>> hasn't big change and we have a problem with naming. >>>> >>> >>> I'm thinking it like this: >>> - if we want to accumulate array normally, use accumArrayResult and >>> makeArrayResult. If we accumulate scalar the result will be 1D array. if we >>> accumulate array, the resulting dimension is incremented by 1. >>> - if we want, somehow to affect the normal behavior, and change the >>> dimensions, use makeMdArrayResult. >>> >>> Searching through the postgres' code, other than internal use in >>> arrayfuncs.c, makeMdArrayResult is used only >>> in src/pl/plperl/plperl.c:plperl_array_to_datum, while converting perl >>> array to postgres array. >>> >>> So if somehow we will accumulate array other than in array_agg, i think >>> the most natural way is using accumArrayResult and then makeArrayResult. >>> >> >> ok, there is more variants and I can't to decide. But I am not satisfied >> with this API. We do some wrong in structure. makeMdArrayResult is now >> ugly. >> > > One approach that i can think is we cleanly separate the structures and > API. We don't touch existing ArrayBuildState, accumArrayResult, > makeArrayResult and makeMdArrayResult, and we create new functions: > ArrayBuildStateArray, accumArrayResultArray, makeArrayResultArray and > makeArrayResultArrayCustom. >
yes, I am thinking about separatate path, that will be joined in constructMdArray In reality, there are two different array builders - with different API and better to respect it. > > But if we do that, the array(subselect) implementation will not be as > simple as current patch. We must also change all the ARRAY_SUBLINK-related > accumArrayResult call. > > Regarding current patch implementation, the specific typedefs are declared > as: > +typedef struct ArrayBuildStateScalar > +{ > + ArrayBuildState astate; > ... > +} ArrayBuildStateArray; > > so it necessities rather ugly code like this: > + result->elemtype = astate->astate.element_type; > > Can we use C11 feature of unnamed struct like this? : > no, what I know a C11 is prohibited > +typedef struct ArrayBuildStateScalar > +{ > + ArrayBuildState; > ... > +} ArrayBuildStateArray; > > so the code can be a little less ugly by doing it like this: > + result->elemtype = astate->element_type; > > I don't know whether all currently supported compilers implements this > feature.. > > > Can you suggest a better structure for this? > > If we can't settle on a better structure, i think i'll reimplement > array_agg without the performance improvement, using deconstruct_array and > unchanged accumArrayResult & makeMdArrayResult. > you can check it? We can test, how performance lost we get. As second benefit we can get numbers for introduction new optimized array builder Regards Pavel > > Thanks, > -- > Ali Akbar >