Re: Cube extension point support // GSoC'13

On 11.10.2013 17:39, Alexander Korotkov wrote:
> On Fri, Oct 11, 2013 at 5:56 PM, Heikki Linnakangas<hlinnakangas@vmware.com
>> wrote:
>
>> 2. I didn't understand this change:
>>
>>   @@ -422,24 +439,14 @@ g_cube_union(PG_FUNCTION_ARGS)
>>>   Datum
>>>   g_cube_compress(PG_FUNCTION_**ARGS)
>>>   {
>>> -       PG_RETURN_DATUM(PG_GETARG_**DATUM(0));
>>> +       GISTENTRY  *entry = (GISTENTRY *) PG_GETARG_POINTER(0);
>>> +       PG_RETURN_POINTER(entry);
>>>   }
>>>
>>>   Datum
>>>   g_cube_decompress(PG_FUNCTION_**ARGS)
>>>   {
>>>          GISTENTRY  *entry = (GISTENTRY *) PG_GETARG_POINTER(0);
>>> -       NDBOX      *key = DatumGetNDBOX(PG_DETOAST_**DATUM(entry->key));
>>> -
>>> -       if (key != DatumGetNDBOX(entry->key))
>>> -       {
>>> -               GISTENTRY  *retval = (GISTENTRY *)
>>> palloc(sizeof(GISTENTRY));
>>> -
>>> -               gistentryinit(*retval, PointerGetDatum(key),
>>> -                                         entry->rel, entry->page,
>>> -                                         entry->offset, FALSE);
>>> -               PG_RETURN_POINTER(retval);
>>> -       }
>>>          PG_RETURN_POINTER(entry);
>>>   }
>>>
>>>
>> What did the removed code do, and why isn't it needed anymore?
>
> I don't understand this place even more generally. What decompress function
> do is to detoast key and return new gist entry with it if needed. But can
> GiST key ever be toasted? My experiments show that answer is no, it can't.
> When too long key is passed then GiST falls during inserting key into page.
> And I didn't find any code doing GiST key toast in git.

I guess it can't happen. Or is it possible that a toasted value that
came from disk will be passed to these functions, without detoasting
them somewhere along the way? Not sure.

In any case, I've committed this patch now. I left out the changes to
compress and decompress functions, as that's unrelated to the patch at
hand. I ran the regression test on a Windows VM, and updated the
expected output file that the Windows build used. That leaves two
alternative expected output files unmodified - the buildfarm will tell
us whether they're still needed.

Thanks and congrats on your first committed patch, Stas! :-)

- Heikki