From: | "Ryan Bradetich" <rbradetich(at)gmail(dot)com> |
---|---|
To: | "David E(dot) Wheeler" <david(at)kineticode(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | [Review] Tests citext casts by David Wheeler. |
Date: | 2008-09-05 04:40:05 |
Message-ID: | e739902b0809042140o1c930fm53e092cea7ec3223@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello all,
Here is my review of the Test citext casts written by David Wheeler:
http://archives.postgresql.org/message-id/F721EFF1-553C-4E25-A293-7BD08D6957F4@kineticode.com
1. The patch applies cleanly to the latest GIT repository.
2. The citext type installs, uninstalls, and re-installs cleanly.
3. The coding style is mostly consistent with the existing code.
The only coding style difference I noticed was introduced in this patch:
In the citext.sql.in file the following functions are created using the
non-dollar quoting syntax:
* regex_matches
* regexp_replace
* regexp_split_to_array
* regexp_split_to table
* strpos
In the citext.sql.in file the following functions are created using the
dollar quoting syntax:
* replay
* split_part
* translate
I do not have a strong preference either way and I do not even care if
they are consistent. I am interested to see if there was a reason for
using both syntaxes for these functions.
4. The regression tests successfully pass when PostgreSQL is built with
--with-libxml. They fail when the PostgreSQL is built without
--with-libxml.
Since the default PostgreSQL configuration does not include --with-libxml
and is not run-time detected when the libxml2 libraries are present on
the system I would recommend adding an additional expected output
file (citext_1.out) that covers the conditions when PostgreSQL is compiled
without --with-libxml.
As an experiment, I was able to add the citext_1.out and the
regression tests
passed with and without the --with-libxml option.
Review of the additional regression tests show they provide coverage of the
new casts and functions added with this patch.
Overall I think the patch looks good. After reviewing the patch, I
played with
citext for an hour or so and I did not encounter any bugs or other surprises.
Thanks,
- Ryan
From | Date | Subject | |
---|---|---|---|
Next Message | David E. Wheeler | 2008-09-05 04:42:20 | Re: [Review] Tests citext casts by David Wheeler. |
Previous Message | Alex Hunsaker | 2008-09-05 03:48:41 | Re: hash index improving v3 |