Re: Range Types - typo + NULL string constructor

On 07.10.2011 06:41, Jeff Davis wrote:
> The repo is available here:
>
> http://git.postgresql.org/gitweb/?p=users/jdavis/postgres.git;a=log;h=refs/heads/rangetypes

I took a look at this, fixed a bunch of minor issues, and pushed to my
git repo: git://git.postgresql.org/git/users/heikki/postgres.git.
Attached is an updated patch including those changes, for the archives.

A few issues that I didn't fix straight away:

* Why do you need to be a superuser to create a range type? In
DefineRange, the comment explaining why superuser privilege is required
seems bogus, copy-pasted from base types. So what is the reason?

* The binary i/o format includes the length of the lower and upper
bounds twice, once explicitly in range_send, and second time within the
send-function of the subtype. Seems wasteful.

* In findRangeSubOpclass, I think it's OK to hard-code "btree" into the
error message, no need to look that up from pg_am.

* Do we really need non_empty(anyrange) ? You can just do "NOT empty(x)"

* Range_before and range_after: I think "input range is empty" would
sound better than "undefined for empty ranges".

* In range_hash, rotating the hash of the initial flags byte seems pointless

* range_constructor_internal - I think it would be better to move logic
to figure out the the arguments into the callers.

* The gist support functions frequently call range_deserialize(), which
does catalog lookups. Isn't that horrendously expensive?

* In range_serialize and range_deserialize, reduce the number of catalog
lookups by combining the get_range_subtype, get_typlen, get_typalign,
get_typbyval and get_typstorage calls
with just one catalog lookup, and fetch all those fields at once.
Something like get_typlenbyvalalign()

* Have you tested this on an architecture with strict alignment? I don't
see any alignment bugs, but I think there's a lot of potential for them..

Documentation
-------------
* Section "Built-in Range Types". How about "PostgreSQL comes with the
following built-in range types:" <examples> "In addition, you can define
your own"

* In section "Inclusive and Exclusive Bounds", it is said "An inclusive
lower bound is represented by ...". That begs the question: where is it
represented like that? That doesn't become clear until you read the
section on Input/Output format. Reordering the sections might help. Are
the double-quotes around [ and ( necessary?

* It would be good to have some examples on the input formats in the
Input/Output section. There's one in the Examples section, but it's not
very clear that it's related. Perhaps the whole Examples section should
be moved to the end, or the examples be dispersed into the other sections.

* Likewise it would be nice to have some examples in the Inclusive and
Exclusive Bounds section.

* subtype_float, how is it supposed to work? I couldn't find any
explanation in the docs. Can we come up with a better name for the function?

* Constructing Ranges section: should describe the 0-3 argument forms of
the constructors, not only in the examples but in the main text too.

* What exactly is canonical function supposed to return? It's not clear
what format one should choose as the canonical format when writing a
custom range type. And even for the built-in types, it would be good to
explain which types use which canonical format (I saw the discussions on
that, so I understand that might still be subject to change).

* Defining New RangeTypes section: A more general description in
addition to the example would be good.

* "GiST Indexing" title: how about making it just "Indexing". Also, it
would be nice to list exactly which operators can be sped up by gist.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com