Skip site navigation (1) Skip section navigation (2)

[Review] Tests citext casts by David Wheeler.

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 (view raw or flat)
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

Responses

pgsql-hackers by date

Next:From: David E. WheelerDate: 2008-09-05 04:42:20
Subject: Re: [Review] Tests citext casts by David Wheeler.
Previous:From: Alex HunsakerDate: 2008-09-05 03:48:41
Subject: Re: hash index improving v3

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group