Re: [HACKERS] Review: B-Tree emulation for GIN

2009-04-04 Thread Teodor Sigaev
* After this new look at the code I think that matchPartialInPendingList is completely broken. Surely it's an infinite loop if the comparePartial function returns -1. I also don't understand why it fixed stops short of examining the tuple at maxoff, or for that matter why it's doing any

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-04-04 Thread Tom Lane
Teodor Sigaev teo...@sigaev.ru writes: [ fixes for the GIN stuff I complained about before ] This all looks good to me, please apply. One little suggestion: ! /* !* entryRes array is used for: !* - as an argument for consistentFn !

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-26 Thread Jeff Davis
On Wed, 2009-03-25 at 19:31 -0400, Tom Lane wrote: * I'd also like to come to some agreement about getting rid of the fail-on-NULL-scankey problem in newScanKey(). As I noted in the comment there, we could make that work cleanly if we are willing to assume that all GIN-indexable operators are

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-26 Thread Tom Lane
Jeff Davis pg...@j-davis.com writes: Also, if extractQuery is non-strict, shouldn't we call it and see if it returns some useful keys? Perhaps. One risk factor for approaching it that way is that there are probably a lot of opclasses out there that haven't bothered to mark these functions

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-25 Thread Tom Lane
[ back to this patch... ] Teodor Sigaev teo...@sigaev.ru writes: Looked at this a bit ... do you think it's really a good idea to remove the strategy number argument of comparePartial? The argument given in the docs for it is that it might be needed to determine when to end the scan, and

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-25 Thread Tom Lane
BTW ... while I'm thinking about it: it seems to me to be a serious error that the consistent() function isn't given nkeys so that it can know the length of the arrays it's being handed. I suppose it's possible for it to re-deduce nkeys by examining the query datum, but that could be quite

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-25 Thread Andrew Gierth
Tom == Tom Lane t...@sss.pgh.pa.us writes: Tom BTW ... while I'm thinking about it: it seems to me to be a Tom serious error that the consistent() function isn't given nkeys Tom so that it can know the length of the arrays it's being handed. Tom I suppose it's possible for it to re-deduce

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-25 Thread Tom Lane
Andrew Gierth and...@tao11.riddles.org.uk writes: Tom == Tom Lane t...@sss.pgh.pa.us writes: Tom BTW ... while I'm thinking about it: it seems to me to be a Tom serious error that the consistent() function isn't given nkeys Tom so that it can know the length of the arrays it's being handed.

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-25 Thread Tom Lane
Teodor Sigaev teo...@sigaev.ru writes: [ btree_gin 0.12 ] Committed with some editorializations. There are still a few loose ends: * the question about zero-key queries that I mentioned before * After this new look at the code I think that matchPartialInPendingList is completely broken.

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-18 Thread Teodor Sigaev
It's easy to un-dirty that hack, but before I'd like to see your comments about thoughts above. Yeah, please revert that hack. Done. Also, I changed void *extra_data to Pointer extra_data and corresponding **extra_data and ***extra_data to simplify understanding. -- Teodor Sigaev

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-16 Thread Heikki Linnakangas
Teodor Sigaev wrote: You might want to declare extra_data as just void *, instead of an array of pointers. The data type implementation might want to store something there that's not per-key, but applies to the whole query. I see that you're passing it to comparePartial, but that seems to be

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-05 Thread Teodor Sigaev
The GIN_EXTRACT_VALUE macro returns a pointer to a static 'entries' variable. That doesn't seem safe. Is it really never possible to have to two GIN searches in a plan, both calling and using the value returned by extractValue simultaneously? In any case that seems like a pretty weak

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-03-04 Thread Heikki Linnakangas
The GIN_EXTRACT_VALUE macro returns a pointer to a static 'entries' variable. That doesn't seem safe. Is it really never possible to have to two GIN searches in a plan, both calling and using the value returned by extractValue simultaneously? In any case that seems like a pretty weak

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-02-11 Thread Teodor Sigaev
Looking through the code again, gin_compare_prefix_##type looks a little confusing. Is there a reason for using: (data-strategy == BTLessStrategyNumber || data-strategy == BTLessEqualStrategyNumber ) ? PointerGetDatum(data-datum) : a rather than just using:

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-02-06 Thread Jeff Davis
On Wed, 2009-02-04 at 20:22 +0300, Teodor Sigaev wrote: The description of extractQuery's extra_data parameter seems confusing too. AFAICS it is incorrect, or at least misleading, to describe it as void ** extra_data[]; it is really void ***extra_data, because there is only one object

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-02-06 Thread Jeff Davis
On Mon, 2009-01-19 at 21:41 +0300, Teodor Sigaev wrote: gin_numeric_cmp() can be called from regular SQL. I missed this before, but that function will segfault if you call gin_numeric_cmp(NULL, 1) (in v0.7 at least). Fixed, gin_numeric_cmp is marked as strict. And how does GIN handle

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-02-04 Thread Teodor Sigaev
Looked at this a bit ... do you think it's really a good idea to remove the strategy number argument of comparePartial? The argument given in the docs for it is that it might be needed to determine when to end the scan, and that still seems plausible to me. Strategy number is not removed, it's

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-02-02 Thread Tom Lane
Looked at this a bit ... do you think it's really a good idea to remove the strategy number argument of comparePartial? The argument given in the docs for it is that it might be needed to determine when to end the scan, and that still seems plausible to me. The description of extractQuery's

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Alvaro Herrera
Jeff Davis escribió: I like the fact that this patch does not modify the numeric ADT. It still relies on the fact that the numeric type will never make use of the minimal varlena struct, however. I bring this up in case someone sees it as a problem. Greg Stark was working on a patch to make

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Teodor Sigaev
still relies on the fact that the numeric type will never make use of the minimal varlena struct, however. I bring this up in case someone sees it as a problem. Greg Stark was working on a patch to make certain values use the short representation. Fake value contains only VARHDRSZ bytes. --

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Tom Lane
Jeff Davis pg...@j-davis.com writes: I like the fact that this patch does not modify the numeric ADT. It still relies on the fact that the numeric type will never make use of the minimal varlena struct, however. I bring this up in case someone sees it as a problem. I'm pretty certain I recall

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Jeff Davis
On Mon, 2009-01-19 at 11:35 -0500, Tom Lane wrote: Jeff Davis pg...@j-davis.com writes: I like the fact that this patch does not modify the numeric ADT. It still relies on the fact that the numeric type will never make use of the minimal varlena struct, however. I bring this up in case

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Teodor Sigaev
I'm pretty certain I recall Greg Stark recommending that we adopt something like that as the standard numeric representation of zero. It didn't get done yet, but it might happen someday. Changes: - use NULL as left-most value. It's safe because NULL numeric value cannot be an argument for any

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Jeff Davis
On Mon, 2009-01-19 at 20:15 +0300, Teodor Sigaev wrote: Changes: - use NULL as left-most value. It's safe because NULL numeric value cannot be an argument for any function except gin_numeric_cmp and it cannot be returned in regular SQL query. gin_numeric_cmp() can be called from regular

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Teodor Sigaev
gin_numeric_cmp() can be called from regular SQL. I missed this before, but that function will segfault if you call gin_numeric_cmp(NULL, 1) (in v0.7 at least). Fixed, gin_numeric_cmp is marked as strict. And how does GIN handle SQL NULL values in the column? Does it index them at all, or

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Jeff Davis
On Mon, 2009-01-19 at 21:41 +0300, Teodor Sigaev wrote: And how does GIN handle SQL NULL values in the column? Does it index them at all, or just ignore them? SQL NULL: GIN doesn't support it (amindexnulls/amsearchnulls == false) C NULL: NULL-numeric could be returned only by

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-19 Thread Jeff Davis
On Mon, 2009-01-19 at 21:41 +0300, Teodor Sigaev wrote: gin_numeric_cmp() can be called from regular SQL. I missed this before, but that function will segfault if you call gin_numeric_cmp(NULL, 1) (in v0.7 at least). Fixed, gin_numeric_cmp is marked as strict. And how does GIN handle

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-18 Thread Jeff Davis
On Fri, 2009-01-16 at 17:42 +0300, Teodor Sigaev wrote: Unfortunately, that means numeric is not supportable for btree-gin because there is no left-most value from which to start the scan. Do you see an easy workaround to support numeric? Hmm, let we use minimal varlena struct (with size

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-16 Thread Teodor Sigaev
I have merged this with HEAD, written a brief document (which is mostly a copy of the btree-gist page), added the module to the contrib Makefile, and made some very minor changes. Patch attached. Thank you A couple comments: 1. Right now, to implement less than you need to start at the

Re: [HACKERS] Review: B-Tree emulation for GIN

2009-01-11 Thread Jeff Davis
On Sun, 2008-12-28 at 23:41 -0800, Jeff Davis wrote: 2. Why do you create a shell type int32 from btree_gin.sql? I tried doing a search-and-replace to use int4 instead, and it seems to work fine (and eliminates the warnings). I left it with int32 in my version of the patch because I thought

Re: [HACKERS] Review: B-Tree emulation for GIN

2008-12-28 Thread Jeff Davis
On Fri, 2008-12-19 at 13:26 +0300, Teodor Sigaev wrote: Updated patch. I have merged this with HEAD, written a brief document (which is mostly a copy of the btree-gist page), added the module to the contrib Makefile, and made some very minor changes. Patch attached. A couple comments: 1. Right

Re: [HACKERS] Review: B-Tree emulation for GIN

2008-12-19 Thread Teodor Sigaev
Updated patch. Ibrar Ahmed wrote: Hi Teodor Sigaev, I am getting server crash in contrib regression. May be I am doing something wrong here. Regression diff is attached. BTW these queries work fine outside the regression. -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com

Re: [HACKERS] Review: B-Tree emulation for GIN

2008-12-19 Thread Ibrar Ahmed
Thanks, On Fri, Dec 19, 2008 at 3:26 PM, Teodor Sigaev teo...@sigaev.ru wrote: Updated patch. Ibrar Ahmed wrote: Hi Teodor Sigaev, I am getting server crash in contrib regression. May be I am doing something wrong here. Regression diff is attached. BTW these queries work fine outside

Re: [HACKERS] Review: B-Tree emulation for GIN

2008-12-17 Thread Teodor Sigaev
will see, may be it's needed to update the patch Ibrar Ahmed wrote: Hi Teodor Sigaev, I am getting server crash in contrib regression. May be I am doing something wrong here. Regression diff is attached. BTW these queries work fine outside the regression. -- Ibrar Ahmed EnterpriseDB