Re: proposal: searching in array function - array_position - Mailing list pgsql-hackers
| From | Pavel Stehule |
|---|---|
| Subject | Re: proposal: searching in array function - array_position |
| Date | |
| Msg-id | CAFj8pRATG-0fQoC7NxDqVws8ak1R0VL7vg51JAYTn7rAMm1xPg@mail.gmail.com Whole thread Raw |
| In response to | Re: proposal: searching in array function - array_position (Robert Haas <robertmhaas@gmail.com>) |
| Responses |
Re: proposal: searching in array function - array_position
(Tom Lane <tgl@sss.pgh.pa.us>)
|
| List | pgsql-hackers |
2015-03-10 15:30 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:
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;
}
On Sat, Mar 7, 2015 at 1:06 PM, Petr Jelinek <petr@2ndquadrant.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;
}
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;
}
{
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;
{
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
pgsql-hackers by date: