PostgreSQL: deal with case insensitivity

According http://www.postgresql.org/docs/9.3/static/citext.html there's still dependency on LC_* settings.
Another trick that CITEXT introduced in 8.3 but as option to compile so probably we need to update requirements https://www.drupal.org/requirements pointing that we need 8.4 at least

Added some questions.

It's easy to install postgresql-contrib package on CentOS and Debian dedicated and managed servers, but what about Drupal hosting companies from the larger companies to the smaller companies (postgresql hosts)?

I know one smaller web host, and I'm not sure if Acquia's clients that use postgres are using DevCloud.

I'm not sure if everything should be case insensitive.

The issue with LC_* may be relevant for a Drupal 8 Multilingual install where a site builder has enabled multiple languages but the database collation is the same. Does it matter for UTF-8? Gabor may have an answer for that.

Debian Jessie drops this month https://lists.debian.org/debian-devel-announce/2015/03/msg00016.html making wheezy oldstable which contains 9.1.15. Ubuntu 10.04 also ends in a month, Ubuntu 12.04 defaults to 9.1.15 as well. http://packages.ubuntu.com/precise/postgresql We can't deal with the 10 year cycle of RHEL/CentOS, RHEL5 EOL is extended to March 31, 2017 -- released in 2007. PostgreSQL has a http://yum.postgresql.org/repopackages.php repo for them.

http://www.postgresql.org/docs/9.1/static/release-9-1-2.html 9.1.2 was released more than three years ago and given the citext related upgrade problem that's the release where I would put the requirement. Also, http://www.postgresql.org/support/versioning/ 9.0 is EOL this September so putting a requirement on it makes little sense.

tl;dr: raise to 9.1.2 and be done.

It seems to me like this is approaching the problem backward -- trying to fix something with a workaround in PostgreSQL to "fix" something that is arguably a problem (or simple difference of opinion) with regards to text comparisons in MySQL.

Here are my reasons for leaning away from using the CITEST module:

1. The CITEXT extension certainly adds further complication, and isn't typically available in most shared hosting environments. (Full disclosure... I work for one of the largest shared hosting companies in the world. I'm speaking from experience here.) I'm concerned that this makes using Drupal on PostgreSQL even more complicated than it already is.
2. Using CITEXT assumes that Drupal is *always* going to do case-insensitive comparisons. Is that the case? Will there every be a time when we *do* want to do case-sensitive comparisons?
3. Unicode/multi-lingual support. To quote the PostgreSQL manual:
   

   CITEXT's case-folding behavior depends on the LC_CTYPE setting of your database. How it compares values is therefore determined when the database is created. It is not truly case-insensitive in the terms defined by the Unicode standard. Effectively, what this means is that, as long as you're happy with your collation, you should be happy with citext's comparisons. But if you have data in different languages stored in your database, users of one language may find their query results are not as expected if the collation is for another language.PostgreSQL Manual

4. This is likely to expose a large number of additional failures in the testing, which will be automatically attributed to PostgreSQL -- just at a time when the number of tests failing due to PostgreSQL is at a historic low.

> Using CITEXT assumes that Drupal is *always* going to do case-insensitive comparisons. Is that the case? Will there every be a time when we *do* want to do case-sensitive comparisons?

We have a field and a schema setting for case sensitive collations. Please refer to #2068655: Entity fields do not support case sensitive queries

Would using UPPER()/LOWER() on comparisons with binary columns be a valid workaround?

Hmm it looks like that is what the CITEXT extension does anyway. Only problem there seems to be that not all upper case letters have a matching lower case and vice versa so results won't 100% match MySQL.

So just to clarify, I was suggesting we update the pgsql query classes so that case insenstive columns turn into LOWER('String') = LOWER('STRING') .. or if we want to be brave we parse the final query and do the LOWER() wrapping there. We'd also have to update the Schema class and create indexes on LOWER(case_insensitive_column).

You'd need to ORDER BY on LOWER() as well. SQL string parsing from PHP is a questionable choice I am afraid although of course using prepared statements as we have it helps tremendously because you do not need to account for user data in there.

Also, you'll have some problems knowing on the SQL level which columns are case sensitive.

Edit: I looked up the citext source, it's very short and understandable and yes, it does lowercase everything before comparison and doesn't do anything else.

Also, you'll have some problems knowing on the SQL level which columns are case sensitive.

@chx what do you mean by that, can you explain why this is a problem? Are we talking about unique constraints etc? Aren't most of the issues covered by sending the right query? Thanks to the schema we know in PHP which columns are case sensitive...

@jaredsmith:

Using CITEXT assumes that Drupal is *always* going to do case-insensitive comparisons. Is that the case?

Not necessarily, it looks like with CITEXT you can define case insensitivity on a column level. Only columns which have binary=TRUE in the schema need have case sensitivity.

The CITEXT extension certainly adds further complication, and isn't typically available in most shared hosting environments. (Full disclosure... I work for one of the largest shared hosting companies in the world. I'm speaking from experience here.) I'm concerned that this makes using Drupal on PostgreSQL even more complicated than it already is.

Not only that, it could also be a showstopper on large enterprise deployments where there are rigid constraints in terms of customization. Some of which still use 8.4 by the way, although with it being EOL'd that may change by the time D8 comes out .

The DBTNG driver will face an almost insurmountable challenge of figuring out which database columns are case sensitive and which are not. This information is available to, say, an entity field storage driver but not the DB driver.

What you can do instead of parsing SQL string is to override various storage drivers in D8 and set up the queries as you need them. This will be quite an endeavour -- there are a lot.

chx yes that was what I suggested earlier, the drawback is we would have no support for case insensitivity in raw db_query statements.

As to figuring out case insensitive fields, couldn't we cache this information and pass it to the DB driver, it's only a handful of table.column combinations which won't really change that often.

Well, the Schema class on create and alter time can certainly capture those columns and store somewhere.

Do we know how well the LOWER() trick works though? Does it work well with the full UTF8 charset?

I looked into this a bit, and it's a long shot as I don't know how this will work with UTF8 data (if at all), but the cs_CZ.ISO8859-2 collation is case insensitive, and PostgreSQL 9.1 allows "per column" collations: http://michael.otacoo.com/postgresql-2/collation-in-postgresql-9-1/

$ sort test 
A
B
C
F
a
b
d
e
g
$ export LC_COLLATE="cs_CZ.ISO8859-2"
$ sort test
A
a
B
b
C
d
e
F
g
$ export LC_COLLATE="cs_CZ.UTF8"
$ sort test 
A
B
C
F
a
b
d
e
g

/edit: but seems pretty limited http://collation-charts.org/fbsd54/fbsd54.cs_CZ.ISO8859-2.html

So as a first step, would the following make sense as far as the pgsql driver is concerned?

• In Schema::createFieldSql() we store whether a varchar/character/text column is binary or not somewhere (maybe in a separate drupal_pgsql_case_sensitive_fields table in the database?), so this information gets saved on field creation/alteration.
• In the Connection object, we load this information from the database and put it into a property $caseSensitiveFields = array('tablename.fieldname', 'tablename2.fieldname2');
• We then wrap the field names that are case insensitive with LOWER() on indexes, orderBy, on field names in conditions and in Select::__toString()

Just to give an update, I discussed this with @amateescu and he pointed me to CREATE FUNCTION, however I haven't been able to find any functionality that is smart enough to alter incoming queries the way we need to. The only thing I can think of is adding another "copy" column for every case insensitive field with a trigger that lowers the case for the "copied" value, which would essentially double the storage usage of the database. Not really a workable solution :)

He also pointed to the table name parsing in the Select class, which would make db_select etc easy to process but it seems we can't use that for regular db_query() statements. These would be more challenging as without CITEXT we'd inevitably need to do some sort of query parsing. However from an initial look at the db_query statements used in core and contrib, covering 100% of queries in core and ~99.9% of queries in the most popular contrib module looks doable and shouldn't pose a huge performance penalty as 99% of db_query() statements will be simple anyway.

We could do smarter parsing for the other 0.9% of queries, and ask people to rewrite the other 0.1% that are too complicated to process in a simple parser (that wouldn't try to cover any possible MySQL query), offering a pgsql_citext driver as a module in contrib for those who need 100% case sensitivity accuracy as opposed to >99.9%.

I will have a look at this over the next week. If we don't manage to implement a workable LOWER() solution, I think we'll have to require the postgresql citext module...

My big concern here is security. Are there places where a case sensitive query versus a case insensitive can lead to information disclosure or worse? I do not think so... but I haven't went through the possible cases really.

Well the opposite might be true (doing case insensitive checks where we really want case sensitive checks), but it's hard to imagine accidental case sensitivity to pose a problem, at least in core. We usually don't really care about case.

In any case this would need a security audit, I guess wrapping placeholders in LOWER() would be possible to implement safely but I'd be very wary of doing this in queries with user input where the placeholders have already been applied.

Password columns shouldn't be stored as case-insensitive text (citext), and those columns are in schema as type "text" (varchar), right?

Users table is still generated by hook_schema()? Or does it use TypedData Data Type plugins? PasswordItem exists if we can use the latter.

The password column is actually case insensitive right now ;)

Technically it shouldn't be but it's not like we store the actual password, just a hash

Created #2475539: Make password entity fields case sensitive by default anyway

Also see: #2477413: Increase minimum version requirement for Postgres to 9.1.2

I'm going to tackle/discuss this tomorrow with people, but need to organize the comments from the past month.

This is a citext patch:

1. Requires super user to create extension citext.
2. Fixes TermTest
3. Does not fix GlossaryTest because Views does a SUBSTRING which compares string to citext = fail.
4. I think number of additional fails may be low, but need to run a test.

Tomorrow I think I will not do this and look at a suggestion from @jaredsmith regarding trying to change Conditions into Having statements. I tried to add LOWER in Condition, but ran into too many regular expression hacks in the driver. I could also have been low on blood sugar because lunch was really really really late.

I couldn't quickly get the drupalci testbot working with citext because the postgres user must create the extension in a specific schema, but the web container does not have access to do this and the database container does not know the database.

Here is a different approach that fixes the 2 term tests, but breaks taxonomy create/delete and possibly lots of other tests.

Okay, this gives me an idea of the test fails that the patch above created.

It looks like langcode in condition needs to be handled and that entity delete is not working in tests (I tried taxonomy term delete page manually and it worked). However I didn't try multiple terms.

This patch fixes some bad query construction in the previous patch, which probably caused many of the exceptions in the test run for #30.

Edit: There are still many fails.

Okay, yeah, that's much better... 1 exception, 1 fatal error, and 3 fails.

VocabularyPermissionsTest passes for me on my local development environment.

So then we would only support entity queries ?

@mradcliffe wouldn't we have to do something similar for db_query() etc?

I wanted to focus on the queries inside the failing tests, but yes, it's possible that something similar needs to be done for the query builder, but I also think it will be very messy per @chx's comment in #14 after I tried to do something similar for entity queries and modify escapeField to handle it.

Entity Query and Entity Manager will be what developers use to fetch entities, but it does use more memory and is not as efficient as fetching one column. Maybe also Views queries?

Other things that needs to happen:

- @jaredsmith mentioned indexes need to be created as case insensitive, but this should be done for any database system.

The locales_source table is using specific MySQL storage type "BLOB" but the locale() translation algorithm is designed to work with case sensitive data.

The result is that locale() internal caching is half broken on non MySQL engines under some circumstances.

Maybe I'm wrong, but this means a big bunch of queries on every t() call because some strings will not get cached.

#2490976: Locale caching algorithm is broken on Non MySQL/PostgreSQL databases

@david_garcia BLOBs are binary data and as such they should always be case sensitive anyway, right?

That is exactly the issue, take a look at this field definition:

 'source' => array(
        'type' => 'text',
        'mysql_type' => 'blob',
        'not null' => TRUE,
        'description' => 'The original string in English.',
      ),

This field is going to be case sensitive on MySQL but case insensitive on any other database engine.

For this field definition to be portable it should be something like this:

 'source' => array(
        'type' => 'text',
        'mysql_type' => 'blob',
        'not null' => TRUE,
        'binary' => TRUE,
        'description' => 'The original string in English.',
      ),

'binary': A boolean indicating that MySQL should force 'char', 'varchar' or 'text' fields to use case-sensitive binary collation. This has no effect on other database types for which case sensitivity is already the default behavior.

Or is the default behaviour of a text field supposed to be 'BINARY=TRUE' ? All the usages of 'binary' I have found do a 'binary=true' which hints as binary=FALSE being the default behaviour.

Or maybe this has changed from D7 to D8 and I'm totally confused :(

Just tried the citext patch in #27

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -131,6 +135,24 @@ protected function checkEncoding() {
+      $version = db_query("SELECT installed_version FROM pg_available_extensions WHERE name = %name", array('%name' => 'citext'))->fetchField();

Getting an error during installation - should this be :name instead of %name?

For the testbot.

The patch from comment #31 fixes the tests from #2443679: PostgreSQL: Fix taxonomy\Tests\TermTest.

I think that for should create a followup issue for fixing the case insensitivity for db_query().

The patch looks good to me. It gets a RTBC from me.

I mentioned this in #17 as well but PostgreSQL uses the collations from the system -- and there aren't any case insensitive collations for UTF8 that I'm aware of.

As bzrudi71 in the previous comment stated: the patch from #31 does not pass all the tests with a PostgreSQL backend. So back to need work!

#2443679: PostgreSQL: Fix taxonomy\Tests\TermTest is in.

seems chx is still working on that issue so we may want to hold off on the reroll?

attempt at a reroll anyway, I assume this will need further work once @chx finishes working on the other issue

cross-posting from @Damien Tournoud in #2477413: Increase minimum version requirement for Postgres to 9.1.2:

(..) I'm not optimistic about citext. It would make more sense to me to switch to case-sensitive, collation-sensitive *by default everywhere* (including on MySQL), and handle case and collation sensitivity manually in PHP. This is the only way to have a well-defined, consistent and efficient behavior on all database engines.

...which would be quite an invasive change but may actually be a good idea...

I have to agree with @stefan.r and @damien Tournoud.

I don't think using citext is the right answer either... I'd rather we switch to handling case-sensitive, collation-sensitive everywhere as well.

It may be an invasive change, but I think it's worthwhile.

Btw CITEXT as capability could be exposed at driver level and drivers should take collation under control

Pg12 compatibility fixed

It used to select pg_trgm in #2988018: [PP-1] Performance issues with path alias generated queries on PostgreSQL
Probably this one is duplicate

I do not think this is a duplicate, but it is related. Maybe we can fix this issue in Drupal 9 with the requirement of the pg_trgm extension.

Yeah, path_alias is definitely slow, but there are multiple queries being affected by this. This problem is a huge bottleneck for performance, and I think the numbers was around 100 % faster on our site after doing a test migration to MySQL.

Re-rolling #2464481-57: PostgreSQL: deal with case insensitivity compatible with latest 8.9.x. We're still using this fix.

The patch #71 has some conflicts with Pathauto, after applying this patch I got this error:

The alias [alias] is already in use in this language.

The query change from this:

SELECT base_table.revision_id AS revision_id, base_table.id AS id
FROM
{path_alias} base_table
INNER JOIN {path_alias} path_alias ON path_alias.id = base_table.id
WHERE (path_alias.alias LIKE :db_condition_placeholder_0 ESCAPE '\\') AND (path_alias.langcode = :db_condition_placeholder_1) AND (path_alias.path NOT LIKE :db_condition_placeholder_2 ESCAPE '\\')
LIMIT 1 OFFSET 0

to this:

SELECT base_table.revision_id AS revision_id, base_table.id AS id
FROM
{path_alias} base_table
INNER JOIN {path_alias} path_alias ON path_alias.id = base_table.id
WHERE ((LOWER(path_alias.alias) = LOWER(:value))) AND (path_alias.langcode = :db_condition_placeholder_0) AND ((LOWER(path_alias.path) <> LOWER(:value)))
LIMIT 1 OFFSET 0

Which got some results on this file core/lib/Drupal/Core/Path/Plugin/Validation/Constraint/UniquePathAliasConstraintValidator.php on this line:

if ($result = $query->range(0, 1)->execute()) { }

is anyone else experiencing this problem?

Yes, same issues with my patch in #71, doesn't work, as you indicate in #73

As per #73 and #76.

See patch here that should solve condition issue: https://www.drupal.org/project/drupal/issues/2490294#comment-14546671

We are not going to use the LOWER operator on PostgreSQL. The decision has been made to use GIST indexes instead. For that is the pg_trgm extension required for Drupal 10. See: https://www.drupal.org/docs/system-requirements/database-server-requirem....

The biggest problem is with performance problems with the path alias queries. The patch from that issue also add the code to create those indexes. See: #2988018-71: [PP-1] Performance issues with path alias generated queries on PostgreSQL . When that has landed, we can use that solution in other places, like with #2490294: User email should not be case sensitive.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Added a simple benchmark for the current implementation of case-insensitive conditions in Postgres.
#3361618: Postgres forcing cases case-insensitivity causes serious performance degradation.