Re: [Review] Tests citext casts by David Wheeler.

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

On Sep 4, 2008, at 21:40, Ryan Bradetich wrote:

> 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 for the review, Ryan!

Best,

David

"Ryan Bradetich" <rbradetich(at)gmail(dot)com> writes:
> Here is my review of the Test citext casts written by David Wheeler:

Thanks for reviewing. I've committed this with your suggestions and
one additional non-cosmetic change: schema-qualify names in the
bodies of the SQL functions so that they are not search_path dependent.

One thing that didn't make a lot of sense to me was the last new
function:

CREATE OR REPLACE FUNCTION translate( citext, citext, text ) RETURNS TEXT AS $$
SELECT pg_catalog.translate( pg_catalog.translate( $1::pg_catalog.text, pg_catalog.lower($2::pg_catalog.text), $3), pg_catalog.upper($2::pg_catalog.text), $3);
$$ LANGUAGE SQL IMMUTABLE STRICT;

Why is it using upper()?

regards, tom lane

On Sep 5, 2008, at 11:30, Tom Lane wrote:

> Thanks for reviewing. I've committed this with your suggestions and
> one additional non-cosmetic change: schema-qualify names in the
> bodies of the SQL functions so that they are not search_path
> dependent.

Thanks, I'll check that out.

> One thing that didn't make a lot of sense to me was the last new
> function:
>
> CREATE OR REPLACE FUNCTION translate( citext, citext, text ) RETURNS
> TEXT AS $$
> SELECT
> pg_catalog.translate( pg_catalog.translate( $1::pg_catalog.text,
> pg_catalog.lower($2::pg_catalog.text), $3),
> pg_catalog.upper($2::pg_catalog.text), $3);
> $$ LANGUAGE SQL IMMUTABLE STRICT;
>
> Why is it using upper()?

To make translate() work case-insensitively, it does two translates:
One lowercase and one uppercase. This allows the translated value to
be returned with its original casing in tact. No, this isn't ideal,
but it was simple to do.

Best,

David

On Sep 5, 2008, at 11:33, David E. Wheeler wrote:

> On Sep 5, 2008, at 11:30, Tom Lane wrote:
>
>> Thanks for reviewing. I've committed this with your suggestions and
>> one additional non-cosmetic change: schema-qualify names in the
>> bodies of the SQL functions so that they are not search_path
>> dependent.
>
> Thanks, I'll check that out.

Finally got to this; sorry for the delay.

Two things I noticed:

1. Did I neglect to include the documentation patch? I've attached it
here. It's necessary because of the addition of the new functions.

2. Many thanks for switching to using the network_show function
instead of the SQL-based casting I had. Can you tell me how to go
about finding such functions? Because for my 8.3 version of citext, I
have a whole bunch of functions that do casting like this:

CREATE OR REPLACE FUNCTION int8(citext)
RETURNS int8
AS 'SELECT int8( $1::text )'
LANGUAGE SQL IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION citext(int8)
RETURNS citext
AS 'SELECT text( $1 )::citext'
LANGUAGE SQL IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION int4(citext)
RETURNS int4
AS 'SELECT int4( $1::text )'
LANGUAGE SQL IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION citext(int4)
RETURNS citext
AS 'SELECT text( $1 )::citext'
LANGUAGE SQL IMMUTABLE STRICT;

...and so on. I'd love to be able to replace these (and many others)
with internal C functions, if only I could figure out what those
functions were. A pointer to making that determination (if they even
exist in 8.3) would be greatly appreciated.

Thanks,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> 1. Did I neglect to include the documentation patch? I've attached it
> here. It's necessary because of the addition of the new functions.

Maybe it got left out of the later patch iterations? Anyway,
will take care of it.

> 2. Many thanks for switching to using the network_show function
> instead of the SQL-based casting I had. Can you tell me how to go
> about finding such functions?

Er, look into pg_cast and then pg_proc? For instance

select oid::regprocedure, prosrc from pg_proc
where oid in (select castfunc from pg_cast);

regards, tom lane

On Sep 12, 2008, at 10:58, Tom Lane wrote:

>> 1. Did I neglect to include the documentation patch? I've attached it
>> here. It's necessary because of the addition of the new functions.
>
> Maybe it got left out of the later patch iterations? Anyway,
> will take care of it.

Great, thank you.

>> 2. Many thanks for switching to using the network_show function
>> instead of the SQL-based casting I had. Can you tell me how to go
>> about finding such functions?
>
> Er, look into pg_cast and then pg_proc? For instance
>
> select oid::regprocedure, prosrc from pg_proc
> where oid in (select castfunc from pg_cast);

That looks like *exactly* what I need. Thanks!

Best,

David

On Sep 12, 2008, at 11:06, David E. Wheeler wrote:

>> Er, look into pg_cast and then pg_proc? For instance
>>
>> select oid::regprocedure, prosrc from pg_proc
>> where oid in (select castfunc from pg_cast);
>
> That looks like *exactly* what I need. Thanks!

Pity. Looks like there were only a few I wasn't using, text_char,
char_text, text_name, and texttoxml. Do I really need to keep all my
other casts like these in 8.3?

CREATE OR REPLACE FUNCTION int8(citext)
RETURNS int8
AS 'SELECT int8( $1::text )'
LANGUAGE SQL IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION citext(int8)
RETURNS citext
AS 'SELECT text( $1 )::citext'
LANGUAGE SQL IMMUTABLE STRICT;

Thanks,

David

On Sep 12, 2008, at 11:14, David E. Wheeler wrote:

> Pity. Looks like there were only a few I wasn't using, text_char,
> char_text, text_name, and texttoxml.

Oh, and text_name seems to give me this error:

ERROR: compressed data is corrupt

That's when I have this cast:

CREATE OR REPLACE FUNCTION citext(name)
RETURNS citext
AS 'text_name'
LANGUAGE internal IMMUTABLE STRICT;

This version does not give me an error:

CREATE OR REPLACE FUNCTION citext(name)
RETURNS citext
AS 'SELECT text( $1 )::citext'
LANGUAGE SQL IMMUTABLE STRICT;

Maybe I did something wrong?

Thanks,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> Oh, and text_name seems to give me this error:

> ERROR: compressed data is corrupt

> That's when I have this cast:

> CREATE OR REPLACE FUNCTION citext(name)
> RETURNS citext
> AS 'text_name'
> LANGUAGE internal IMMUTABLE STRICT;

I think you've got the direction backwards.

BTW, I removed the "Limitations" entry about I/O casting not working
with citext; we fixed that, no?

regards, tom lane

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> Pity. Looks like there were only a few I wasn't using, text_char,
> char_text, text_name, and texttoxml. Do I really need to keep all my
> other casts like these in 8.3?

> CREATE OR REPLACE FUNCTION int8(citext)
> RETURNS int8
> AS 'SELECT int8( $1::text )'
> LANGUAGE SQL IMMUTABLE STRICT;

Yeah, those are all replaced by the CoerceViaIO mechanism.

regards, tom lane

On Sep 12, 2008, at 11:31, Tom Lane wrote:

> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> Oh, and text_name seems to give me this error:
>
>> ERROR: compressed data is corrupt
>
>> That's when I have this cast:
>
>> CREATE OR REPLACE FUNCTION citext(name)
>> RETURNS citext
>> AS 'text_name'
>> LANGUAGE internal IMMUTABLE STRICT;
>
> I think you've got the direction backwards.

Oh. Duh.

> BTW, I removed the "Limitations" entry about I/O casting not working
> with citext; we fixed that, no?

Yes, we did. Thanks for the catch.

I've got another patch I'm working on adding support for "char" (and
tests for char). Just to fill out a gap I saw in the casting coverage.
I'm trying to get it done now. With that, AFAIK, citext will work just
like text.

Best,

David

On Sep 12, 2008, at 11:34, Tom Lane wrote:

>> CREATE OR REPLACE FUNCTION int8(citext)
>> RETURNS int8
>> AS 'SELECT int8( $1::text )'
>> LANGUAGE SQL IMMUTABLE STRICT;
>
> Yeah, those are all replaced by the CoerceViaIO mechanism

Okay, thanks for the sanity check. The SQL versions are fine for me in
8.3.

Best,

David

On Sep 12, 2008, at 11:35, David E. Wheeler wrote:

> I've got another patch I'm working on adding support for "char" (and
> tests for char). Just to fill out a gap I saw in the casting
> coverage. I'm trying to get it done now. With that, AFAIK, citext
> will work just like text.

Looks like the IO conversions handle char and "char", so the attached
patch just updates the regression test.

Best,

David

David E. Wheeler escribió:
> On Sep 12, 2008, at 11:35, David E. Wheeler wrote:
>
>> I've got another patch I'm working on adding support for "char" (and
>> tests for char). Just to fill out a gap I saw in the casting coverage.
>> I'm trying to get it done now. With that, AFAIK, citext will work just
>> like text.
>
> Looks like the IO conversions handle char and "char", so the attached
> patch just updates the regression test.

There are unresolved conflicts in the patch ...

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Sep 12, 2008, at 12:49, Alvaro Herrera wrote:

>> Looks like the IO conversions handle char and "char", so the attached
>> patch just updates the regression test.
>
> There are unresolved conflicts in the patch ...

Bah! Sorry. Let me try that again.

Best,

David

Just want to make sure that this wasn't lost in the shuffle somewhere…

Best,

David
On Sep 14, 2008, at 15:42, David E. Wheeler wrote:

> On Sep 12, 2008, at 12:49, Alvaro Herrera wrote:
>
>>> Looks like the IO conversions handle char and "char", so the
>>> attached
>>> patch just updates the regression test.
>>
>> There are unresolved conflicts in the patch ...
>
> Bah! Sorry. Let me try that again.
>
> Best,
>
> David