On 2019-09-17 17:17, Tom Lane wrote: > My recommendation is to get rid of the run-time checks and instead > put a hack like this into DefineIndex or some minion thereof: > > if ((opclass == TEXT_PATTERN_BTREE_CLASS_OID || > opclass == VARCHAR_PATTERN_BTREE_CLASS_OID || > opclass == BPCHAR_PATTERN_BTREE_CLASS_OID) && > !get_collation_isdeterministic(collid)) > ereport(ERROR, ...);
Here is a draft patch. It will require a catversion change because those operator classes don't have assigned OIDs so far. The comment block I just moved over for the time being. It should probably be rephrased a bit. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 3b1f52e5292303cba8804ae1f6676973ef903ec0 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@eisentraut.org> Date: Wed, 18 Sep 2019 14:28:31 +0200 Subject: [PATCH v1] Prohibit creating text_pattern_ops indexes with nondeterministic collations Discussion: https://www.postgresql.org/message-id/22566.1568675...@sss.pgh.pa.us --- src/backend/catalog/index.c | 40 +++++++++++++++++++++++++++ src/backend/utils/adt/varchar.c | 32 +++++----------------- src/backend/utils/adt/varlena.c | 43 +++++------------------------- src/include/catalog/pg_opclass.dat | 9 ++++--- 4 files changed, 58 insertions(+), 66 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 1dac2803b0..f2adee3156 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -762,6 +762,46 @@ index_create(Relation heapRelation, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("user-defined indexes on system catalog tables are not supported"))); + /* + * XXX We cannot use a text_pattern_ops index for nondeterministic + * collations, because these operators intentionally ignore the collation. + * However, the planner has no way to know that, so it might choose such + * an index for an "=" clause, which would lead to wrong results. This + * check here doesn't prevent choosing the index, but it will at least + * error out if the index is chosen. A text_pattern_ops index on a column + * with nondeterministic collation is pretty useless anyway, since LIKE + * etc. won't work there either. A future possibility would be to + * annotate the operator class or its members in the catalog to avoid the + * index. Another alternative is to stay away from the *_pattern_ops + * operator classes and prefer creating LIKE-supporting indexes with + * COLLATE "C". + */ + for (i = 0; i < indexInfo->ii_NumIndexAttrs; i++) + { + Oid collation = collationObjectId[i]; + Oid opclass = classObjectId[i]; + + if (collation) + { + if (!get_collation_isdeterministic(collation) && + (opclass == TEXT_BTREE_PATTERN_OPS_OID || + opclass == VARCHAR_BTREE_PATTERN_OPS_OID || + opclass == BPCHAR_BTREE_PATTERN_OPS_OID)) + { + HeapTuple classtup; + + classtup = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass)); + if (!HeapTupleIsValid(classtup)) + elog(ERROR, "cache lookup failed for operator class %u", opclass); + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("nondeterministic collations are not supported for operator class \"%s\"", + NameStr(((Form_pg_opclass) GETSTRUCT(classtup))->opcname)))); + ReleaseSysCache(classtup); + } + } + } + /* * Concurrent index build on a system catalog is unsafe because we tend to * release locks before committing in catalogs. diff --git a/src/backend/utils/adt/varchar.c b/src/backend/utils/adt/varchar.c index 332dc860c4..b21137c583 100644 --- a/src/backend/utils/adt/varchar.c +++ b/src/backend/utils/adt/varchar.c @@ -1105,23 +1105,12 @@ hashbpcharextended(PG_FUNCTION_ARGS) */ static int -internal_bpchar_pattern_compare(BpChar *arg1, BpChar *arg2, Oid collid) +internal_bpchar_pattern_compare(BpChar *arg1, BpChar *arg2) { int result; int len1, len2; - check_collation_set(collid); - - /* - * see internal_text_pattern_compare() - */ - if (!get_collation_isdeterministic(collid)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("nondeterministic collations are not supported for operator class \"%s\"", - "bpchar_pattern_ops"))); - len1 = bcTruelen(arg1); len2 = bcTruelen(arg2); @@ -1144,7 +1133,7 @@ bpchar_pattern_lt(PG_FUNCTION_ARGS) BpChar *arg2 = PG_GETARG_BPCHAR_PP(1); int result; - result = internal_bpchar_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_bpchar_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -1160,7 +1149,7 @@ bpchar_pattern_le(PG_FUNCTION_ARGS) BpChar *arg2 = PG_GETARG_BPCHAR_PP(1); int result; - result = internal_bpchar_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_bpchar_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -1176,7 +1165,7 @@ bpchar_pattern_ge(PG_FUNCTION_ARGS) BpChar *arg2 = PG_GETARG_BPCHAR_PP(1); int result; - result = internal_bpchar_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_bpchar_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -1192,7 +1181,7 @@ bpchar_pattern_gt(PG_FUNCTION_ARGS) BpChar *arg2 = PG_GETARG_BPCHAR_PP(1); int result; - result = internal_bpchar_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_bpchar_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -1208,7 +1197,7 @@ btbpchar_pattern_cmp(PG_FUNCTION_ARGS) BpChar *arg2 = PG_GETARG_BPCHAR_PP(1); int result; - result = internal_bpchar_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_bpchar_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -1221,17 +1210,8 @@ Datum btbpchar_pattern_sortsupport(PG_FUNCTION_ARGS) { SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); - Oid collid = ssup->ssup_collation; MemoryContext oldcontext; - check_collation_set(collid); - - if (!get_collation_isdeterministic(collid)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("nondeterministic collations are not supported for operator class \"%s\"", - "bpchar_pattern_ops"))); - oldcontext = MemoryContextSwitchTo(ssup->ssup_cxt); /* Use generic string SortSupport, forcing "C" collation */ diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 0864838867..b44f895d95 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -2996,34 +2996,12 @@ textgename(PG_FUNCTION_ARGS) */ static int -internal_text_pattern_compare(text *arg1, text *arg2, Oid collid) +internal_text_pattern_compare(text *arg1, text *arg2) { int result; int len1, len2; - check_collation_set(collid); - - /* - * XXX We cannot use a text_pattern_ops index for nondeterministic - * collations, because these operators intentionally ignore the collation. - * However, the planner has no way to know that, so it might choose such - * an index for an "=" clause, which would lead to wrong results. This - * check here doesn't prevent choosing the index, but it will at least - * error out if the index is chosen. A text_pattern_ops index on a column - * with nondeterministic collation is pretty useless anyway, since LIKE - * etc. won't work there either. A future possibility would be to - * annotate the operator class or its members in the catalog to avoid the - * index. Another alternative is to stay away from the *_pattern_ops - * operator classes and prefer creating LIKE-supporting indexes with - * COLLATE "C". - */ - if (!get_collation_isdeterministic(collid)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("nondeterministic collations are not supported for operator class \"%s\"", - "text_pattern_ops"))); - len1 = VARSIZE_ANY_EXHDR(arg1); len2 = VARSIZE_ANY_EXHDR(arg2); @@ -3046,7 +3024,7 @@ text_pattern_lt(PG_FUNCTION_ARGS) text *arg2 = PG_GETARG_TEXT_PP(1); int result; - result = internal_text_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_text_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -3062,7 +3040,7 @@ text_pattern_le(PG_FUNCTION_ARGS) text *arg2 = PG_GETARG_TEXT_PP(1); int result; - result = internal_text_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_text_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -3078,7 +3056,7 @@ text_pattern_ge(PG_FUNCTION_ARGS) text *arg2 = PG_GETARG_TEXT_PP(1); int result; - result = internal_text_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_text_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -3094,7 +3072,7 @@ text_pattern_gt(PG_FUNCTION_ARGS) text *arg2 = PG_GETARG_TEXT_PP(1); int result; - result = internal_text_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_text_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -3110,7 +3088,7 @@ bttext_pattern_cmp(PG_FUNCTION_ARGS) text *arg2 = PG_GETARG_TEXT_PP(1); int result; - result = internal_text_pattern_compare(arg1, arg2, PG_GET_COLLATION()); + result = internal_text_pattern_compare(arg1, arg2); PG_FREE_IF_COPY(arg1, 0); PG_FREE_IF_COPY(arg2, 1); @@ -3123,17 +3101,8 @@ Datum bttext_pattern_sortsupport(PG_FUNCTION_ARGS) { SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); - Oid collid = ssup->ssup_collation; MemoryContext oldcontext; - check_collation_set(collid); - - if (!get_collation_isdeterministic(collid)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("nondeterministic collations are not supported for operator class \"%s\"", - "text_pattern_ops"))); - oldcontext = MemoryContextSwitchTo(ssup->ssup_cxt); /* Use generic string SortSupport, forcing "C" collation */ diff --git a/src/include/catalog/pg_opclass.dat b/src/include/catalog/pg_opclass.dat index fdfea85efe..9bab83e91e 100644 --- a/src/include/catalog/pg_opclass.dat +++ b/src/include/catalog/pg_opclass.dat @@ -146,13 +146,16 @@ opcfamily => 'btree/datetime_ops', opcintype => 'timestamp' }, { opcmethod => 'hash', opcname => 'timestamp_ops', opcfamily => 'hash/timestamp_ops', opcintype => 'timestamp' }, -{ opcmethod => 'btree', opcname => 'text_pattern_ops', +{ oid => '4187', oid_symbol => 'TEXT_BTREE_PATTERN_OPS_OID', + opcmethod => 'btree', opcname => 'text_pattern_ops', opcfamily => 'btree/text_pattern_ops', opcintype => 'text', opcdefault => 'f' }, -{ opcmethod => 'btree', opcname => 'varchar_pattern_ops', +{ oid => '4188', oid_symbol => 'VARCHAR_BTREE_PATTERN_OPS_OID', + opcmethod => 'btree', opcname => 'varchar_pattern_ops', opcfamily => 'btree/text_pattern_ops', opcintype => 'text', opcdefault => 'f' }, -{ opcmethod => 'btree', opcname => 'bpchar_pattern_ops', +{ oid => '4189', oid_symbol => 'BPCHAR_BTREE_PATTERN_OPS_OID', + opcmethod => 'btree', opcname => 'bpchar_pattern_ops', opcfamily => 'btree/bpchar_pattern_ops', opcintype => 'bpchar', opcdefault => 'f' }, { opcmethod => 'btree', opcname => 'money_ops', opcfamily => 'btree/money_ops', -- 2.23.0
