Re: [HACKERS] 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(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.

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