Re: [HACKERS] Cube extension point support // GSoC'13

On Mon, Oct 21, 2013 at 11:06 PM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)com> wrote:

> On 11.10.2013 17:39, Alexander Korotkov wrote:
>
>> On Fri, Oct 11, 2013 at 5:56 PM, Heikki Linnakangas<hlinnakangas(at)**
>> vmware.com <hlinnakangas(at)vmware(dot)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.
>

I will ask Teodor about it. When situation will be completely clear we
should cleanup confusing comments.
Also, too long GiST key shouldn't cause GiST to fall in its internals. We
should add check somewhere before.

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! :-)

Stas, congratulations from me too! Now, it's time to rebase another patches
against head. :)

------
With best regards,
Alexander Korotkov.