From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: adam.warland@infor.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19341: REPLACE() fails to match final character when using nondeterministic ICU collation
Date: Thu, 04 Dec 2025 18:14:51 +0100
Message-ID: <792a0aeb486e240ea34f10f895e0368fc434b8b7.camel@cybertec.at> (raw)
In-Reply-To: <640025.1764774727@sss.pgh.pa.us>
References: <19341-1d9a22915edfec58@postgresql.org>
<60b467ac47d5c6bbb9c21c3bdfecce8e5de60cda.camel@cybertec.at>
<6935ea1e-cfb1-400b-9057-3d43b55d621a@iki.fi>
<9f47c6b24d6cf59002603caf19b8b3f4854176be.camel@cybertec.at>
<375807.1764696352@sss.pgh.pa.us>
<464a14fcbee9567ace136716714b8c18f9bd87fb.camel@cybertec.at>
<436944.1764708826@sss.pgh.pa.us>
<0bbe14779f2c37e5a2fbd514861ebedaf8f53df6.camel@cybertec.at>
<640025.1764774727@sss.pgh.pa.us>
On Wed, 2025-12-03 at 10:12 -0500, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > On Tue, 2025-12-02 at 15:53 -0500, Tom Lane wrote:
> > > Looking at the code overall, I wonder if the outer loop doesn't have
> > > the same issue. The comments claim that we should be able to handle
> > > zero-length matches, but if the overall haystack is of length zero,
> > > we will fail to check for such a match.
>
>
> After further thought, it seems to me that this comment is an
> unjustified extrapolation from what Peter actually said, which was
> that the match substring could be physically shorter than the needle.
> Which is certainly true, for instance case-folding or accent-stripping
> might shorten the string. But it doesn't follow that a nonempty
> needle could ever match an empty substring; and that does not seem
> like it could be sane behavior to me. We're considering string
> comparison here, not regexes.
>
> We do require callers to eliminate the empty-needle case, and given
> that I think we could assume that match substrings must be at least
> 1 byte long. That assumption is what justifies the current API for
> these functions, and perhaps we can also simplify this loop by
> using it.
I think I get it. I don't see an explicit requirement for a non-empty
needle, but all callers of text_position_next_internal() handle that
case separately.
The attached v5 patch simplifies the loop to a do-while loop, assuming
that we cannot find a zero-length match.
I have also updated the comments to no longer mention the possibility
of an empty match, and for good measure I have added an Assert() that
the needle cannot be empty.
Yours,
Laurenz Albe
Attachments:
[text/x-patch] v5-0001-Fix-greedy-substring-search-for-non-deterministic.patch (5.1K, ../792a0aeb486e240ea34f10f895e0368fc434b8b7.camel@cybertec.at/2-v5-0001-Fix-greedy-substring-search-for-non-deterministic.patch)
download | inline diff:
From e265fc5790d4c8224bb758518470b2424418d913 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Thu, 4 Dec 2025 18:07:06 +0100
Subject: [PATCH v5] Fix greedy substring search for non-deterministic
collations
Due to an off-by-one error, the code failed to find matches at the
end of the haystack. Fix by rewriting the loop.
While at it, fix a comment that claimed that the function could find
a zero-length match. Such a match could send a caller into an
endless loop. However, zero-length matches can only occur with an
empty search string, and that case is explicitly excluded by all
callers. To make sure it stays that way, add an Assert.
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reported-By: Adam Warland <adam.warland@infor.com>
Reviewed-By: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-By: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19341-1d9a22915edfec58%40postgresql.org
---
src/backend/utils/adt/varlena.c | 24 ++++++++++++-------
.../regress/expected/collate.icu.utf8.out | 7 ++++++
src/test/regress/sql/collate.icu.utf8.sql | 3 +++
3 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.cindex 3894457ab40..26fbb9e969b 100644--- a/src/backend/utils/adt/varlena.c+++ b/src/backend/utils/adt/varlena.c@@ -1110,10 +1110,10 @@ text_position_next_internal(char *start_ptr, TextPositionState *state)
const char *haystack_end = &haystack[haystack_len];
const char *hptr;
- Assert(start_ptr >= haystack && start_ptr <= haystack_end);-
state->last_match_len_tmp = needle_len;
+ Assert(start_ptr >= haystack && start_ptr <= haystack_end && needle_len > 0);+
if (!state->locale->deterministic)
{
/*
@@ -1123,9 +1123,9 @@ text_position_next_internal(char *start_ptr, TextPositionState *state)
* needle under the given collation.
*
* Note, the found substring could have a different length than the
- * needle, including being empty. Callers that want to skip over the- * found string need to read the length of the found substring from- * last_match_len rather than just using the length of their needle.+ * needle. Callers that want to skip over the found string need to+ * read the length of the found substring from last_match_len rather+ * than just using the length of their needle.
*
* Most callers will require "greedy" semantics, meaning that we need
* to find the longest such substring, not the shortest. For callers
@@ -1136,6 +1136,8 @@ text_position_next_internal(char *start_ptr, TextPositionState *state)
hptr = start_ptr;
while (hptr < haystack_end)
{
+ const char *test_end;+
/*
* First check the common case that there is a match in the
* haystack of exactly the length of the needle.
@@ -1146,11 +1148,14 @@ text_position_next_internal(char *start_ptr, TextPositionState *state)
return (char *) hptr;
/*
- * Else check if any of the possible substrings starting at hptr- * are equal to the needle.+ * Else check if any of the possible non-empty substrings starting+ * at hptr are equal to the needle.
*/
- for (const char *test_end = hptr; test_end < haystack_end; test_end += pg_mblen(test_end))+ test_end = hptr;+ do
{
+ test_end += pg_mblen(test_end);+
if (pg_strncoll(hptr, (test_end - hptr), needle, needle_len, state->locale) == 0)
{
state->last_match_len_tmp = (test_end - hptr);
@@ -1158,7 +1163,8 @@ text_position_next_internal(char *start_ptr, TextPositionState *state)
if (!state->greedy)
break;
}
- }+ } while (test_end < haystack_end);+
if (result_hptr)
break;
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.outindex b8579a1efc6..0a14c1d93ff 100644--- a/src/test/regress/expected/collate.icu.utf8.out+++ b/src/test/regress/expected/collate.icu.utf8.out@@ -1484,6 +1484,13 @@ SELECT array_sort('{a,B}'::text[] COLLATE "C");
{B,a}
(1 row)
+-- test replace() at the end of the string (bug #19341)+SELECT replace('testX' COLLATE case_insensitive, 'x' COLLATE case_insensitive, 'es');+ replace +---------+ testes+(1 row)+
-- test language tags
CREATE COLLATION lt_insensitive (provider = icu, locale = 'en-u-ks-level1', deterministic = false);
SELECT 'aBcD' COLLATE lt_insensitive = 'AbCd' COLLATE lt_insensitive;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sqlindex 6f5abac0dc0..5e3fef9b381 100644--- a/src/test/regress/sql/collate.icu.utf8.sql+++ b/src/test/regress/sql/collate.icu.utf8.sql@@ -568,6 +568,9 @@ SELECT 'abc' <= 'ABC' COLLATE case_insensitive, 'abc' >= 'ABC' COLLATE case_inse
SELECT array_sort('{a,B}'::text[] COLLATE case_insensitive);
SELECT array_sort('{a,B}'::text[] COLLATE "C");
+-- test replace() at the end of the string (bug #19341)+SELECT replace('testX' COLLATE case_insensitive, 'x' COLLATE case_insensitive, 'es');+
-- test language tags
CREATE COLLATION lt_insensitive (provider = icu, locale = 'en-u-ks-level1', deterministic = false);
SELECT 'aBcD' COLLATE lt_insensitive = 'AbCd' COLLATE lt_insensitive;
--
2.52.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: laurenz.albe@cybertec.at, tgl@sss.pgh.pa.us, hlinnaka@iki.fi, adam.warland@infor.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19341: REPLACE() fails to match final character when using nondeterministic ICU collation
In-Reply-To: <792a0aeb486e240ea34f10f895e0368fc434b8b7.camel@cybertec.at>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox