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

From: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
To: Stas Kelvich <stas(dot)kelvich(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, pgsql-students(at)postgresql(dot)org, Alexander Korotkov <aekorotkov(at)gmail(dot)com>
Subject: Re: [HACKERS] Cube extension point support // GSoC'13
Date: 2013-10-11 13:56:28
Message-ID: 5258038C.9070409@vmware.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-students

On 09.10.2013 21:07, Robert Haas wrote:
> On Tue, Sep 24, 2013 at 9:07 AM, Stas Kelvich<stas(dot)kelvich(at)gmail(dot)com> wrote:
>> Hello
>>
>> There is new version of patch. I have separated ordering operators to different patch (https://commitfest.postgresql.org/action/patch_view?id=1243) fixed formatting issues and implemented backward compatibility with old-style points in cube_is_point() and cube_out().
>>
>> Also comparing output files I've discovered that this four files is combination of two types of different behavior:
>>
>> 1) SELECT '-1e-700'::cube AS cube;
>> can be (0) or (-0)
>>
>> 2) Amount of zeros in exponent of floating point, i.e. SELECT '1e27'::cube AS cube;
>> can be (1e+027) or (1e+27)
>>
>> On my system (OSX) it is second option in both situations. I've also tested it on FreeBSD 9.0 and Ubuntu 12.04 with the same results. So is there some ideas how can I reproduce such results?
>
> Heikki, are you going to review this further for this CommitFest?

Sorry, I didn't realize the ball was in my court.

I went through the patch now, kibitzing over some minor style issues.
Attached is a new version.

This seems good for commit except for two things:

1. The alternative expected output files still need to be updated. Stas
couldn't find a system where some of those file were used. One option is
to simply commit the patch as is, and see if the buildfarm goes red. If
it doesn't, we can simply remove the alternative files - they are not
used on any supported platform. If some animals go red, we'll get the
required diff from the buildfarm output and apply. So this isn't a
show-stopper.

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?

> Is there a prerequisite patch that hasn't been committed yet?

No.

- Heikki

Attachment Content-Type Size
points-2.patch text/x-diff 35.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2013-10-11 13:58:16 Re: [PATCH] Add use of asprintf()
Previous Message Arturas Mazeika 2013-10-11 13:49:11 #define ExclusiveLock in /usr/include/postgresql/9.1/server/storage/lock.h

Browse pgsql-students by date

  From Date Subject
Next Message Alexander Korotkov 2013-10-11 14:39:33 Re: Cube extension point support // GSoC'13
Previous Message Robert Haas 2013-10-09 18:07:55 Re: Cube extension point support // GSoC'13