Re: [HACKERS] [PATCH] Add citext_pattern_ops to citext contrib module

[Andrew Dunstan]

On 09/18/2017 12:50 PM, Tom Lane wrote:
> Alexey Chernyshov  writes:
>> Do we need expected/citext.out? It seems that only
>> expected/citext_1.out has correct output.
> Well, for me, citext.out matches the results in C locale,
> and citext_1.out matches the results in en_US.  If you don't
> satisfy both of those cases, the buildfarm will not like you.
>
>   


I'm about to pick this one up - I will handle the expected file issue.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add citext_pattern_ops to citext contrib module

[Tom Lane]

Alexey Chernyshov  writes:
> Do we need expected/citext.out? It seems that only
> expected/citext_1.out has correct output.

Well, for me, citext.out matches the results in C locale,
and citext_1.out matches the results in en_US.  If you don't
satisfy both of those cases, the buildfarm will not like you.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add citext_pattern_ops to citext contrib module

[Alexey Chernyshov]

On Tue, 12 Sep 2017 12:59:20 -0400
Tom Lane  wrote:

> Quick comment on this patch: recently, we've decided that having
> patches replace the whole base script for an extension is too much of
> a maintenance problem, especially when there are several patches in
> the pipeline for the same contrib module.  The new style is to
> provide only a version update script (which you'd have to write
> anyway), and then rely on CREATE EXTENSION to apply the old base
> script plus the update(s). You can see some examples in the patch I
> just posted at
> 
> https://www.postgresql.org/message-id/24721.1505229...@sss.pgh.pa.us
> 
> Also, since that patch is probably going to get committed pretty
> soon, you could reformulate your patch as an add-on to its
> citext--1.4--1.5.sql script.  We don't really need to have a separate
> version of the extension for states that are intermediate between two
> PG major releases.  Only if your patch doesn't get in by v11 freeze
> would you need to make it a separate citext--1.5--1.6.sql script.
> 
>   regards, tom lane

Accented characters and different length strings tests added.
Since patch
(ttps://www.postgresql.org/message-id/24721.1505229...@sss.pgh.pa.us)
is committed, I changed the patch as you said. Thanks for your notes.
Do we need expected/citext.out? It seems that only
expected/citext_1.out has correct output.

-- 
Alexey Chernyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres CompanyFrom 35aa8d7a1890a242cdfb3bed6cabaa3b39766f1f Mon Sep 17 00:00:00 2001
From: Alexey Chernyshov 
Date: Tue, 18 Jul 2017 13:50:19 +0300
Subject: [PATCH] patch applied

---
 contrib/citext/citext--1.4--1.5.sql  |  74 +++
 contrib/citext/citext.c  | 120 
 contrib/citext/expected/citext.out   | 370 +++
 contrib/citext/expected/citext_1.out | 370 +++
 contrib/citext/sql/citext.sql|  78 
 5 files changed, 1012 insertions(+)

diff --git a/contrib/citext/citext--1.4--1.5.sql b/contrib/citext/citext--1.4--1.5.sql
index 97942cb..5ae522b 100644
--- a/contrib/citext/citext--1.4--1.5.sql
+++ b/contrib/citext/citext--1.4--1.5.sql
@@ -12,3 +12,77 @@ ALTER OPERATOR >= (citext, citext) SET (
 RESTRICT   = scalargesel,
 JOIN   = scalargejoinsel
 );
+
+CREATE FUNCTION citext_pattern_lt( citext, citext )
+RETURNS bool
+AS 'MODULE_PATHNAME'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
+
+CREATE FUNCTION citext_pattern_le( citext, citext )
+RETURNS bool
+AS 'MODULE_PATHNAME'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
+
+CREATE FUNCTION citext_pattern_gt( citext, citext )
+RETURNS bool
+AS 'MODULE_PATHNAME'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
+
+CREATE FUNCTION citext_pattern_ge( citext, citext )
+RETURNS bool
+AS 'MODULE_PATHNAME'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
+
+CREATE OPERATOR ~<~ (
+LEFTARG= CITEXT,
+RIGHTARG   = CITEXT,
+NEGATOR= ~>=~,
+COMMUTATOR = ~>~,
+PROCEDURE  = citext_pattern_lt,
+RESTRICT   = scalarltsel,
+JOIN   = scalarltjoinsel
+);
+
+CREATE OPERATOR ~<=~ (
+LEFTARG= CITEXT,
+RIGHTARG   = CITEXT,
+NEGATOR= ~>~,
+COMMUTATOR = ~>=~,
+PROCEDURE  = citext_pattern_le,
+RESTRICT   = scalarltsel,
+JOIN   = scalarltjoinsel
+);
+
+CREATE OPERATOR ~>=~ (
+LEFTARG= CITEXT,
+RIGHTARG   = CITEXT,
+NEGATOR= ~<~,
+COMMUTATOR = ~<=~,
+PROCEDURE  = citext_pattern_ge,
+RESTRICT   = scalargtsel,
+JOIN   = scalargtjoinsel
+);
+
+CREATE OPERATOR ~>~ (
+LEFTARG= CITEXT,
+RIGHTARG   = CITEXT,
+NEGATOR= ~<=~,
+COMMUTATOR = ~<~,
+PROCEDURE  = citext_pattern_gt,
+RESTRICT   = scalargtsel,
+JOIN   = scalargtjoinsel
+);
+
+CREATE FUNCTION citext_pattern_cmp(citext, citext)
+RETURNS int4
+AS 'MODULE_PATHNAME'
+LANGUAGE C STRICT IMMUTABLE PARALLEL SAFE;
+
+CREATE OPERATOR CLASS citext_pattern_ops
+FOR TYPE CITEXT USING btree AS
+OPERATOR1   ~<~  (citext, citext),
+OPERATOR2   ~<=~ (citext, citext),
+OPERATOR3   =(citext, citext),
+OPERATOR4   ~>=~ (citext, citext),
+OPERATOR5   ~>~  (citext, citext),
+FUNCTION1   citext_pattern_cmp(citext, citext);
diff --git a/contrib/citext/citext.c b/contrib/citext/citext.c
index 0ba4782..189105a 100644
--- a/contrib/citext/citext.c
+++ b/contrib/citext/citext.c
@@ -18,6 +18,7 @@ PG_MODULE_MAGIC;
  */
 
 static int32 citextcmp(text *left, text *right, Oid collid);
+static int32 internal_citext_pattern_cmp(text *left, text *right, Oid collid);
 
 /*
  *		=
@@ -59,6 +60,40 @@ citextcmp(text *left, text *right, Oid collid)
 }
 
 /*
+ * citext_pattern_cmp()
+ * Internal character-by-character comparison function for citext strings.
+ * Returns int32 negative, zero, or positive.
+ */
+static int32
+internal_citext_pattern_cmp(text *left, 

Re: [HACKERS] [PATCH] Add citext_pattern_ops to citext contrib module

[Tom Lane]

Alexey Chernyshov  writes:
> The attached patch introduces citext_pattern_ops for citext extension 
> type like text_pattern_ops for text type. Here are operators ~<~, ~<=~, 
> ~>~, ~>=~ combined into citext_pattern_ops operator class. These 
> operators simply compare underlying citext values as C strings with 
> memcmp() function.

Hi Alexey,

Quick comment on this patch: recently, we've decided that having patches
replace the whole base script for an extension is too much of a
maintenance problem, especially when there are several patches in the
pipeline for the same contrib module.  The new style is to provide only
a version update script (which you'd have to write anyway), and then
rely on CREATE EXTENSION to apply the old base script plus the update(s).
You can see some examples in the patch I just posted at

https://www.postgresql.org/message-id/24721.1505229...@sss.pgh.pa.us

Also, since that patch is probably going to get committed pretty soon, you
could reformulate your patch as an add-on to its citext--1.4--1.5.sql
script.  We don't really need to have a separate version of the extension
for states that are intermediate between two PG major releases.  Only
if your patch doesn't get in by v11 freeze would you need to make it a
separate citext--1.5--1.6.sql script.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add citext_pattern_ops to citext contrib module

[Jacob Champion]

On Tue, Jul 18, 2017 at 5:18 AM, Alexey Chernyshov
 wrote:
> Hi all,

Hi Alexey, I took a look at your patch. Builds fine here, and passes
the new tests.

I'm new to this code, so take my review with a grain of salt.

> The attached patch introduces citext_pattern_ops for citext extension type
> like text_pattern_ops for text type. Here are operators ~<~, ~<=~, ~>~, ~>=~
> combined into citext_pattern_ops operator class. These operators simply
> compare underlying citext values as C strings with memcmp() function.

Are there any cases where performing the str_tolower with the default
collation, then comparing byte-by-byte, could backfire? The added test
cases don't make use of any multibyte/accented characters, so it's not
clear to me yet what *should* be happening in those cases.

It also might be a good idea to add some test cases that compare
strings of different lengths, to exercise all the paths in
internal_citext_pattern_cmp().

> +-- test citext_pattern_cmp() function explicetily.

Spelling nitpick in the new SQL: s/explicetily/explicitly .

--Jacob


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] Add citext_pattern_ops to citext contrib module

[Alexey Chernyshov]

Hi all,

The attached patch introduces citext_pattern_ops for citext extension 
type like text_pattern_ops for text type. Here are operators ~<~, ~<=~, 
~>~, ~>=~ combined into citext_pattern_ops operator class. These 
operators simply compare underlying citext values as C strings with 
memcmp() function. This operator class isn’t supported by B-Tree index 
yet, but it is a first step to do it.


Patch includes regression tests and is applicable to the latest commit 
(c85ec643ff2586e2d144374f51f93bfa215088a2).


The problem of citext support for LIKE operator with B-Tree index was 
mentioned in [1]. Briefly, the planner doesn’t use B-Tree index for 
queries text_col LIKE ‘abc%’. I’d like to investigate how to improve it 
and make another patch. I think the start point is 
match_special_index_operator() function which doesn’t support custom 
types. I would appreciate hearing your opinion on this.


1. https://www.postgresql.org/message-id/3924.1480351187%40sss.pgh.pa.us

--
Alexey Chernyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

>From 6ff180548209fb3abb66d70686df728b307635e3 Mon Sep 17 00:00:00 2001
From: Alexey Chernyshov 
Date: Fri, 23 Jun 2017 14:40:04 +0300
Subject: [PATCH 1/3] add citext opclass citext_pattern_ops

---
 contrib/citext/Makefile  |   2 +-
 contrib/citext/citext--1.4--1.5.sql  |   1 +
 contrib/citext/citext--1.4.sql   | 501 --
 contrib/citext/citext--1.5.sql   | 575 +++
 contrib/citext/citext.c  | 120 
 contrib/citext/citext.control|   2 +-
 contrib/citext/expected/citext.out   | 334 
 contrib/citext/expected/citext_1.out | 334 
 contrib/citext/sql/citext.sql|  72 +
 9 files changed, 1438 insertions(+), 503 deletions(-)
 create mode 100644 contrib/citext/citext--1.4--1.5.sql
 delete mode 100644 contrib/citext/citext--1.4.sql
 create mode 100644 contrib/citext/citext--1.5.sql

diff --git a/contrib/citext/Makefile b/contrib/citext/Makefile
index 563cd22..8474e86 100644
--- a/contrib/citext/Makefile
+++ b/contrib/citext/Makefile
@@ -3,7 +3,7 @@
 MODULES = citext
 
 EXTENSION = citext
-DATA = citext--1.4.sql citext--1.3--1.4.sql \
+DATA = citext--1.5.sql citext--1.4--1.5.sql citext--1.3--1.4.sql \
 	citext--1.2--1.3.sql citext--1.1--1.2.sql \
 	citext--1.0--1.1.sql citext--unpackaged--1.0.sql
 PGFILEDESC = "citext - case-insensitive character string data type"
diff --git a/contrib/citext/citext--1.4--1.5.sql b/contrib/citext/citext--1.4--1.5.sql
new file mode 100644
index 000..4c8abc0
--- /dev/null
+++ b/contrib/citext/citext--1.4--1.5.sql
@@ -0,0 +1 @@
+/* contrib/citext/citext--1.4--1.5.sql */
diff --git a/contrib/citext/citext--1.4.sql b/contrib/citext/citext--1.4.sql
deleted file mode 100644
index 7b06198..000
--- a/contrib/citext/citext--1.4.sql
+++ /dev/null
@@ -1,501 +0,0 @@
-/* contrib/citext/citext--1.4.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION citext" to load this file. \quit
-
---
---  PostgreSQL code for CITEXT.
---
--- Most I/O functions, and a few others, piggyback on the "text" type
--- functions via the implicit cast to text.
---
-
---
--- Shell type to keep things a bit quieter.
---
-
-CREATE TYPE citext;
-
---
---  Input and output functions.
---
-CREATE FUNCTION citextin(cstring)
-RETURNS citext
-AS 'textin'
-LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE;
-
-CREATE FUNCTION citextout(citext)
-RETURNS cstring
-AS 'textout'
-LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE;
-
-CREATE FUNCTION citextrecv(internal)
-RETURNS citext
-AS 'textrecv'
-LANGUAGE internal STABLE STRICT PARALLEL SAFE;
-
-CREATE FUNCTION citextsend(citext)
-RETURNS bytea
-AS 'textsend'
-LANGUAGE internal STABLE STRICT PARALLEL SAFE;
-
---
---  The type itself.
---
-
-CREATE TYPE citext (
-INPUT  = citextin,
-OUTPUT = citextout,
-RECEIVE= citextrecv,
-SEND   = citextsend,
-INTERNALLENGTH = VARIABLE,
-STORAGE= extended,
--- make it a non-preferred member of string type category
-CATEGORY   = 'S',
-PREFERRED  = false,
-COLLATABLE = true
-);
-
---
--- Type casting functions for those situations where the I/O casts don't
--- automatically kick in.
---
-
-CREATE FUNCTION citext(bpchar)
-RETURNS citext
-AS 'rtrim1'
-LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE;
-
-CREATE FUNCTION citext(boolean)
-RETURNS citext
-AS 'booltext'
-LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE;
-
-CREATE FUNCTION citext(inet)
-RETURNS citext
-AS 'network_show'
-LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE;
-
---
---  Implicit and assignment type casts.
---
-
-CREATE CAST (citext AS text)WITHOUT FUNCTION AS IMPLICIT;
-CREATE CAST (citext AS varchar) WITHOUT FUNCTION AS IMPLICIT;
-CREATE CAST (citext AS bpchar)  WITHOUT