|From:||"Erik Rijkers" <er(at)xs4all(dot)nl>|
|To:||"Jeff Davis" <pgsql(at)j-davis(dot)com>|
|Subject:||Re: REVIEW Range Types|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Sun, February 6, 2011 07:41, Jeff Davis wrote:
> New patch. All known TODO items are closed, although I should do a
> cleanup pass over the code and docs.
> Fixed in this patch:
> * Many documentation improvements
> * Added INT8RANGE
> * Renamed PERIOD[TZ] -> TS[TZ]RANGE
> * Renamed INTRANGE -> INT4RANGE
> * Improved parser's handling of whitespace and quotes
> * Support for PL/pgSQL functions with ANYRANGE arguments/returns
> * Make "subtype_float" function no longer a requirement for GiST,
> but it should still be supplied for the penalty function to be
I'm afraid even the review is WIP, but I thought I'd post what I have.
Context: At the moment we use postbio (see below) range functionality, to search ranges and
overlap in large DNA databases ('genomics'). We would be happy if a core data type could replace
that. It does look like the present patch is ready to do those same tasks, of which the main one
for us is gist-indexed ranges. We also use btree_gist with that, so to include that in core would
make sense in this regard.
./ configure \
compile, make, check, install all OK.
* Is the patch in context diff format?
* Does it apply cleanly to the current git master?
It applied cleanly. (after the large serialisation commit 2011.02.08 it will need some
* Does it include reasonable tests, necessary doc patches, etc?
Yes, there are many tests; the documentation is good. Small improvements below.
Read what the patch is supposed to do, and consider:
* Does the patch actually implement that?
* Do we want that?
* Do we already have it?
contrib/seg has some similar functionalities: "seg is a data type for representing line segments,
or floating point intervals".
And on pgFoundry there is a seg spin-off "postbio", tailored to genomic data (with gist-indexing).
(see postbio manual: http://postbio.projects.postgresql.org/ )
* Does it follow SQL spec, or the community-agreed behavior?
I don't know - I couldn't find much in the SQL-spec on a range datatype.
The ranges behaviour has been discussed on -hackers.
* Does it include pg_dump support (if applicable)?
dump/restore were fine in the handful of range-tables
which I moved between machines.
* Are there dangers?
Not that I could find.
* Have all the bases been covered?
I think the functionality looks fairly complete.
* Does the feature work as advertised?
The patch seems very stable. My focus has been mainly on the intranges. I tested by parsing
documentation- and regression examples, and parametrising them in a perl harness, to generate many
thousands of range combinations. I found only a single small problem (posted earlier - Jeff Davis
solved it already apparently).
* Are there corner cases the author has failed to consider?
* Are there any assertion failures or crashes?
I haven't seen a single one.
table 9-42. range functions:
The following functions are missing (I encountered them in the regression tests):
section 'Constructing Ranges' (8.16.6):
In the code example, remove the following line:
"-- the int4range result will appear in the canonical format"
it doesn't make sense there. At this place "canonical format" has not been discussed;
maybe it is not even discussed anywhere.
also (same place):
'where "_" is used to mean "exclusive" and "" is used to mean "inclusive".'
'where "_" is used to mean "exclusive" and "i" is used to mean "inclusive".'
And btw: it should mention here that the range*inf* functions,
an underscore to separate 'range' from the rest of the function name, e.g.:
range_linfi_() => infinite lower bound, inclusive upper bound
I still want to do Performance review and Coding review.
FWIW, I would like to repeat that my impression is that the patch is very stable, especially with
regard to the intranges (tested extensively).
|Next Message||Heikki Linnakangas||2011-02-08 19:48:53||Re: MVCC doc typo fix|
|Previous Message||Magnus Hagander||2011-02-08 19:34:15||Re: Sync Rep for 2011CF1|