[MohamedM216]

Reference Issue

MDEV-39520

Description

This PR implements the MySQL 8.0 extension of REGEXP_INSTR():

REGEXP_INSTR(subject, pattern[, pos[, occurrence[, return_option[, match_type]]]])

• pos: the 1-based character position to start the search from (default 1)
• occurrence: which occurrence of a match to report (default 1)
• return_option: 0 = position of the match start, 1 = position after the match end (default 0)
• match_type: 'c', 'i', 'm', 'n', 'u', same letters as REGEXP_LIKE()

Examples

SELECT REGEXP_INSTR('abbabba', 'b{2}', 1, 2);             # 5
SELECT REGEXP_INSTR('abcabc', 'b', 1, 1, 1);              # 3 (position after the match)
SELECT REGEXP_INSTR('Abba', 'ABBA', 1, 1, 0, 'i');        # 1 (case-insensitive)
SELECT REGEXP_INSTR(_utf8mb4 X'CEB26161', '(?=a)', 1, 2); # 3, matches MySQL 8.0

Implementation notes

• Item_func_regexp_instr::val_int() iterates the matches with Regexp_processor_pcre::exec() and converts the byte offset of the requested occurrence into a 1-based character position with CHARSET_INFO::numchars(), the same way MySQL 8.0 (ICU) counts positions.
• Zero-length match progression: after an empty match the search continues one character past the match end (ICU does the same, one code point past an empty match). The character length is read at the match end, not at the position the search started from: a zero-width match like (?=x) is normally found away from the search start, and restarting the search inside a multibyte character makes PCRE2 report a UTF-8 error.
• A constant match_type is parsed at prepare time. A non-constant one is re-applied per row, and Regexp_processor_pcre::compile() caches the compiled pattern keyed on (pattern, library flags), so the regex is only recompiled when the effective flags actually change.
• Charsets that are not ASCII-compatible (ucs2, utf16, utf32) are converted before the match_type letters are parsed, so both the parsing and the error messages see the real value, not raw bytes.
• NULL in any argument propagates; a negative/zero pos or occurrence, a return_option outside {0,1} raise ER_WRONG_PARAMETERS_TO_NATIVE_FCT; an invalid match_type letter raises ER_WRONG_VALUE_FOR_TYPE; an argument above the 32-bit range can never match and yields 0.

Differences from MySQL 8.0

As agreed with the @sanja-byelkin , all remaining incompatibilities are documented in
the commit message. In short:

1. An empty pattern is accepted (MySQL 8.0 rejects it with ER_REGEXP_ILLEGAL_ARGUMENT); pre-existing MariaDB behaviour of the 2-argument form.
2. Error codes differ (ER_WRONG_PARAMETERS_TO_NATIVE_FCT, ER_WRONG_VALUE_FOR_TYPE instead of the ICU-specific ER_REGEXP_*).
3. The 'u' match_type letter is accepted as a no-op.
4. The MariaDB-only @@default_regex_flags remains in effect; a match_type argument only overrides the case/multiline/dotall parts.
5. The dialect is PCRE2 rather than ICU; Unicode property classes, case folding and some constructs can differ in edge cases.
6. Subjects with bytes that are not valid UTF-8 produce a warning and "no match" (PCRE2 UTF error); MySQL 8.0/ICU may behave differently. Not covered by the test file because the warning text is PCRE2-version specific.

Test

New test mysql-test/main/regexp_instr_mysql8.test covers all 2..6-argument forms, multibyte subjects, zero-length matches and zero-width lookaheads (found away from the search start), NULL and unsigned argument propagation, non-constant match_type (recompilation through the (pattern, flags) cache), prepared statements, and the error paths.

[gkodinov]

Thank you for your contribution! This is a preliminary review.

First of all: this is implementing just one sub-task of what https://jira.mariadb.org/browse/MDEV-39106 calls for. Either do all of it, as scoped in the jira, in a single go. Or create Jira sub-task(s) into the above jira and target these instead.

There's also some suggestions below.

[gkodinov]

Please fix the failing buildbot tests. Some improvement suggestions below too.

[MohamedM216]

The current failing tests aren't related to my patch and they pass locally.

[gkodinov]

LGTM. Please stand by for the final review.

[sanja-byelkin]

Found implementation problem should be fixed. Also performance and functionality test should be made after code review done

[sanja-byelkin]

If It was constant and flags was calculated on fix field what sens to repeat here again and again the same exercise?

[MohamedM216]

Thanks @sanja-byelkin for the detailed feedback!

[MohamedM216]

Hi @sanja-byelkin, just following up on this PR. I'd appreciate your feedback. Thanks!

[Copilot]

Pull request overview

This PR extends MariaDB’s REGEXP_INSTR() implementation to support MySQL 8.0-style extended syntax (2–6 arguments: pos, occurrence, return_option, match_type) and adds an MTR test to validate compatibility and edge cases.

Changes:

• Switch REGEXP_INSTR function creation to native-function parsing to accept 2–6 arguments.
• Implement extended-argument semantics in Item_func_regexp_instr (positioning, occurrences, return option, match_type flag parsing).
• Add mysql-test coverage for the extended syntax (regexp_instr_mysql8.test/.result).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
sql/item_create.cc	Updates function factory to accept 2–6 args for REGEXP_INSTR.
sql/item_cmpfunc.h	Extends regexp processor API and updates Item_func_regexp_instr ctor to take an argument list.
sql/item_cmpfunc.cc	Implements MySQL 8.0 extended REGEXP_INSTR behavior, including match_type parsing and occurrence scanning.
mysql-test/main/regexp_instr_mysql8.test	Adds a new MTR test covering the extended syntax and edge cases.
mysql-test/main/regexp_instr_mysql8.result	Expected results for the new MTR test.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sanja-byelkin]

I buildbot above tests failing with memory leak around regexp

[MohamedM216]

@sanja-byelkin I believe these failures are unrelated to my PR; the mariabackup.incremental_compressed crash is an InnoDB redo log recovery assertion in log0recv.cc and perfschema.socket_connect is a networking test.

[gkodinov]

LGTM. Please consider squashing the two commits before the end of the review.

A stylistic change proposed below.

[MohamedM216]

First of all is it MDEV-39520 or MDEV-39106 (PR title, commit comment and test headers contradicts)

It's MDEV-39520. I've updated the commit message. Sorry for confusion.

[MohamedM216]

@sanja-byelkin Thanks for the detailed feedback. I've pushed a new patch. Looking forward to your review. Thanks!

[MohamedM216]

https://buildbot.mariadb.org/#builders/719/builds/18033 has environment-specific encoding issue. It passes locally and on other buidbots. Also the only part that fails is the Multibyte characters.

[sanja-byelkin]

It looks like the fail connected to the fix https://buildbot.mariadb.org/#/builders/719/builds/18033/steps/9/logs/stdio

[gkodinov]

Requesting changes on Sanja's behalf

[MohamedM216]

It looks like the fail connected to the fix https://buildbot.mariadb.org/#/builders/719/builds/18033/steps/9/logs/stdio

@sanja-byelkin @gkodinov Fixed. Since we can't reliably force the mysqltest client's file-I/O charset from within the .test file via SQL, I've replaced the raw multibyte strings with _utf8mb4 X'...' HEX literals. This keeps the test file 100% ASCII and bypasses the buildbot's locale while still thoroughly testing the multibyte regex logic.

[gkodinov]

I do not mind a good workaround. Approved. With one suggestion.

[MohamedM216]

@gkodinov I've already added the original strings as comments above each query in the .test file. Do you mean to echo them as well in the .result file?

[gkodinov]

@gkodinov I've already added the original strings as comments above each query in the .test file. Do you mean to echo them as well in the .result file?

not really. there's one you've missed though.

[MohamedM216]

@gkodinov Yeah! You mean the last query. Actually, it uses the same string as the query before it. I've added the comment again, BTW.

[gkodinov]

LGTM. Please keep working with the final reviewer.

[MohamedM216]

Hi @sanja-byelkin, I'm just following up on this PR. Thanks!

[sanja-byelkin]

Review of 98558d5. I built both this PR and MySQL 8.4.13 (bundled ICU 77.1) and ran the same queries on both; "MySQL:" values below are from that run, not from the manual (the manual says nothing about errors for bad arguments).

Blocker

• Zero-length match advance is measured at the wrong place (sql/item_cmpfunc.cc, val_int(), the match_end == match_start branch). After an empty match the code adds the length of the character at byte_offset (where the search started) instead of the character at match_end. With mixed character widths the next search skips a character or starts inside one:

  SELECT REGEXP_INSTR(_utf8mb4 X'CEB26161', '(?=a)', 1, 2);          -- 'βaa': this PR 0, MySQL 3
  SELECT REGEXP_INSTR(_utf8mb4 X'61CEB2CEB2', _utf8mb4 X'283F3DCEB229', 1, 2);
                                                                      -- 'aββ','(?=β)': this PR 0 + warning 1139
                                                                      -- "Regex error 'bad offset into UTF string'", MySQL 3

  Measure the character at subject_ptr + match_end and add both cases to the test.

Behaviour that differs from MySQL 8.0

• pos past the end. MySQL raises error 3686 "Index out of bounds in regular expression search." already for pos = CHAR_LENGTH + 1 on a non-empty subject (REGEXP_INSTR('abc','c',4)); this PR returns 0, and the test records 0 for exactly that query. For an empty subject MySQL returns 0 for any pos, including 0.
• pos below 1 (0, -1, 18446744073709551615): MySQL error 3686, this PR error 1583. With an empty subject MySQL returns 0 for pos 0, this PR raises 1583.
• occurrence below 1: MySQL returns the first match (REGEXP_INSTR('abcabc','b',1,0) and ...,1,-5) give 2); this PR raises error 1583.
• return_option not 0/1: MySQL raises 1210 "Incorrect arguments to regexp_instr: return_option must be 1 or 0." (at prepare for a constant); this PR raises 1583, whose text names no argument. The same 1583 for pos/occurrence/return_option does not tell the user which argument is wrong.
• Unknown match_type letter: MySQL 1210 "Incorrect arguments to regexp_instr"; this PR 1411 ER_WRONG_VALUE_FOR_TYPE.
• 'u' is accepted and ignored. MySQL maps it to UREGEX_UNIX_LINES. With PCRE2 configured for LF it happens to change nothing, but the code comment "no direct PCRE2 equivalent" is wrong (PCRE2_NEWLINE_LF / pcre2_set_newline()). Please fix the comment, or implement it.
• @@default_regex_flags vs match_type (my earlier question): any match_type, including '', clears MULTILINE and DOTALL coming from @@default_regex_flags:

  SET default_regex_flags='DOTALL';
  SELECT REGEXP_INSTR('a\nb','a.b');            -- 1
  SELECT REGEXP_INSTR('a\nb','a.b',1,1,0,'');   -- 0

  In MySQL a missing match_type and '' are the same thing (there is no such variable). Please either override only the flags a letter names, or document and test this. The comment "The MySQL 8.0 behavior is that match_type fully overrides the default flags" should state what MariaDB does instead of referring to MySQL.
• Empty patterns in the test. REGEXP_INSTR('hello',''), ('','') and ('abc','',1,N) all give error 3685 "Illegal argument to a regular expression." in MySQL. MariaDB accepted the 2-argument form before this patch, so this is not a regression, but the new occurrence tests with '' record behaviour MySQL does not have; please say so in the test or drop them.

Correctness / robustness

• Constant match_type evaluated at prepare without !is_expensive() (fix_length_and_dec()). fix_owner() skips expensive constant patterns; the match_type path does not, so a subquery or stored function as match_type runs during prepare, and thd->is_error() is not checked after val_str(). (A two-row subquery still ends with error 1242 in my run, so I did not see a user-visible failure, but the pattern differs from the sibling.)
• match_type is parsed as raw bytes. A ucs2/utf16 'i' is rejected with Incorrect match_type value: '' for function regexp_instr: the message shows the value as empty. MySQL rejects it too (1210), so this is about the message: please convert to ASCII before parsing, or at least format the value with ErrConvString.
• const_cast<String*>(match_type_str)->c_ptr_safe() in the error path can write to / reallocate the argument item's own buffer; use ErrConvString(match_type_str).ptr().
• Hardcoded "regexp_instr" in Regexp_processor_pcre::init(match_type, ...). The processor is shared by all REGEXP functions, and the PR says the others follow; pass the owner's func_name().
• Per-row recompilation with a non-constant match_type: init() empties m_prev_pattern, so compile() runs on every row even if neither pattern nor flags changed, and the code bypasses recompile() by hand. Remember the flags the pattern was compiled with and recompile only when they change. This also bears on the performance test I asked for earlier, which is still needed.
• match_type evaluated twice per row (args[5]->is_null() then val_str()); drop is_null(), the null_value check after val_str() covers it.
• pos/occurrence ignore unsigned_flag: 18446744073709551615 wraps to -1.
• Nullability (my earlier comment): Item_func::fix_fields() already ORs MAYBE_NULL from all arguments, so the new loop after fix_owner() is a no-op and can go. One case remains: with a non-constant match_type, a failed per-row compile() returns NULL even when no argument is nullable.
• m_match_type_is_const is not initialized in the constructor.

Code structure and style

• The fetch / NULL / range-check / my_error block is repeated for args 2, 3, 4; check_arguments() repeats the int check three times (check_argument_types_can_return_int(2, MY_MIN(arg_count, 5)) exists); the match_type read (mt_buf[64], val_str, re.init) is duplicated in fix_length_and_dec() and val_int(). Please factor.
• The init() overload has a different contract from the existing init() (can fail, raises an error, drops the pattern cache), so consider a separate set_match_type() called after init(), which also avoids recomputing the case rule; and document it (// false = OK, true = error is all there is now).
• Coding style: a = b / column-aligned assignments instead of a= b, trailing whitespace on six new lines, C-style casts (size_t), (longlong), (int).
• Comments: the m_prev_pattern reset comment limits it to "non-constant match_type with a constant pattern" but it runs on every call; say why recompile() is skipped for a non-constant match_type; say pos counts characters; "1=end pos" should be "position after the end of the match"; drop comments that restate the code (args[5]: match_type string).

Tests

• No error path is tested: pos 0 / negative / past the end, occurrence 0, return_option 2, an unknown letter (constant and from a column), 1 and 7 arguments.
• No zero-length match on a multibyte subject (would have caught the blocker); only utf8mb4 subjects; NULL only as literals.
• "Non-constant match_type with constant pattern" uses rows 'c' and 'ic', which both resolve to case-sensitive and both print 0, so the result cannot show a per-row flag change — use 'c' and 'i'.
• The multibyte section still uses plain # comments that do not reach the result file (my earlier --echo comment).
• Header says "Improve REGEXP_INSTR for MySQL 8.0 compatibility" (neither the ticket title nor the commit subject); footer should be --echo # End of 13.2 tests.

Commit message

• The body says the multibyte section keeps the test file "100% pure ASCII", but the # comments there contain raw UTF-8.
• Please describe the user-visible behaviour: the new errors for bad arguments, the @@default_regex_flags interaction, and that the result can now be NULL from the new arguments. The first paragraph could name the use case (queries ported from MySQL 8.0 failing with error 1582). The PR description ("one of a series") cannot serve as the squashed commit message; use the commit's own message.

[sanja-byelkin]

Just to be clear I do not request one-to-one MySQL compatibilities just all incompatibilities (which we do not can/want fix) should be listed (probably so far commit message is the best place)

[MohamedM216]

Hi @sanja-byelkin , thanks for the feedback!

Zero-length match advance is measured at the wrong place (sql/item_cmpfunc.cc, val_int(), the match_end == match_start branch). After an empty match the code adds the length of the character at byte_offset (where the search started) instead of the character at match_end. With mixed character widths the next search skips a character or starts inside one

FIXED.

Correctness / robustness

applied

and tests are updated.

The patch https://github.com/MariaDB/server/compare/add85486f1f9f2f914baa3e4b19196c8471a7ffd..e3b6f45d5925c7694aaa130c2576287b535ad917

The differences between MariaDB and MySQL are listed in the commit message and in the PR description.

Thanks!