Обсуждение: [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 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
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@gmail.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@kineticode.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@kineticode.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@kineticode.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@kineticode.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