Re: Cube extension point support // GSoC'13 - Mailing list pgsql-hackers

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