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
introducedin 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
seeif 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
PostgreSQLis built without
--with-libxml.
Since the default PostgreSQL configuration does not include --with-libxml and is not run-time detected when the
libxml2libraries 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
thispatch.
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
