2015-03-10 15:30 GMT+01:00 Robert Haas <[email protected]>:
> On Sat, Mar 7, 2015 at 1:06 PM, Petr Jelinek <[email protected]> wrote:
> > You still duplicate the type cache code, but many other array functions
> do
> > that too so I will not hold that against you. (Maybe somebody should
> write
> > separate patch that would put all that duplicate code into common
> function?)
> >
> > I think this patch is ready for committer so I am going to mark it as
> such.
>
> The documentation in this patch needs some improvements to the
> English. Can anyone help with that?
>
> The documentation should discuss what happens if the array is
> multi-dimensional.
>
> The code for array_offset and for array_offset_start appear to be
> byte-for-byte identical. There's no comment explaining why, but I bet
> it's to make the opr_sanity test pass. How about adding a comment?
>
> The comment for array_offset_common refers to array_offset_start as
> array_offset_startpos.
>
yes, it is a reason. I'll comment it.
>
> I am sure in agreement with the idea that it would be good to factor
> out the common typecache code (for setting up my_extra). Any chance
> we get a preliminary patch that does that refactoring, and then rebase
> the main patch on top of it? I agree that it's not really this
> patch's job to solve that problem, but it would be nice.
>
The common part is following code:
my_extra = (ArrayMetaState *) fcinfo->flinfo->fn_extra;
if (my_extra == NULL)
{
fcinfo->flinfo->fn_extra =
MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
sizeof(ArrayMetaState));
my_extra = (ArrayMetaState *) fcinfo->flinfo->fn_extra;
my_extra->element_type = ~element_type;
}
and
if (my_extra->element_type != element_type)
{
get_typlenbyvalalign(element_type,
&my_extra->typlen,
&my_extra->typbyval,
&my_extra->typalign);
my_extra->element_type = element_type;
}
so we can design function like
(ArrayMetaState *)
GetCachedArrayMetaState(FunctionCallInfo fcinfo, Oid element_type, bool
*reused)
{
ArrayMetaState *state = (ArrayMetaState *) fcinfo->flinfo->fn_extra;
if (state == NULL)
{
fcinfo->flinfo->fn_extra =
MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
sizeof(ArrayMetaState));
state = (ArrayMetaState *) fcinfo->flinfo->fn_extra;
state->element_type = ~element_type;
}
if (state->element_type != element_type)
{
get_typlenbyvalalign(element_type,
&my_extra->typlen,
&my_extra->typbyval,
&my_extra->typalign);
state->element_type = element_type;
*resused = false;
}
else
*reused = true;
}
Usage in code:
array_state = GetCachedArrayMetaState(fceinfo, element_type, &reused);
if (!reused)
{
// some other initialization
}
The content is relatively clean, but the most big problem is naming (as
usual)
Comments?
Regards
Pavel
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
