Re: proposal: searching in array function - array_position

2015-03-10 15:30 GMT+01:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Sat, Mar 7, 2015 at 1:06 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> 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
>