MDEV-33971 NAME_CONST in WHERE clause replaced by inner item

[DaveGosselin-MariaDB]

Improve performance of queries like SELECT * FROM t1 WHERE field = NAME_CONST('a', 4); by, in this example, replacing the WHERE clause with field = 4 in the case of ref access.

The rewrite is done during fix_fields and we disambiguate this case from other cases of NAME_CONST by inspecting where we are in parsing. We rely on THD::where to accomplish this. To improve performance there, we change the type of THD::where to be an enumeration, so we can avoid string comparisons during Item_name_const::fix_fields. Consequently, this patch also changes all usages of THD::where to conform likewise.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[spetrunia]

One other question:

What if Item_const::fix_fields() always did the following, regardless of its location:

• call value_item->set_name(...) in the same way it does for itself in Item_const::Item_const.
• substitute itself with value_item

Will this work (EDIT: quick attempt: no, rpl.rpl_name_const fails) and if not why?

[DaveGosselin-MariaDB]

Generally, looks like a step in the right direction.

Thanks! 😄

I'm wondering if the patch could be smaller if there was a const char* thd_where_is_where_cond and then one could compare thd->where strings by comparing pointers... but since we've added an enum, let's not remove it. It has an advantage of listing the possible values of thd->where ...

I had considered string pointers very briefly, but decided against that approach for a number of reasons:

• pointer values are hard to distinguish in logs (should these get logged)
• enumerations are easier to read and (sometimes) remember
• future maintainers could look at the string pointer comparisons and say "did he really mean to compare the pointer values and not the strings themselves?" which would lead to endless stumbling.
• comparing values across different builds would be impossible

Generally, enumerations are more intentional and less ambiguous.

I think in addition to handling WHERE, we need to handle ON expressions. Handling other expressions (like HAVING) looks more risky as they may have references to select list values...

Would that be appropriate to do now or as a separate MDEV?

[DaveGosselin-MariaDB]

One other question:

What if Item_const::fix_fields() always did the following, regardless of its location:

• call value_item->set_name(...) in the same way it does for itself in Item_const::Item_const.
• substitute itself with value_item

Will this work (EDIT: quick attempt: no, rpl.rpl_name_const fails) and if not why?

This is an interesting idea, we should explore this further in another MDEV.

[spetrunia]

I don't understand how the last variant is supposed to work? I see this check

if (thd->last_sql_command == SQLCOM_SELECT &&

what is it supposed to achieve?

[spetrunia]

As far as I understand, the issue raised in https://jira.mariadb.org/browse/MDEV-33971?focusedCommentId=285058&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-285058

is still not addressed. Double-checking this. I take

commit 05e9b4a6955bcbedc237e46fafdccb2b4419d1a6 (HEAD -> 10.6-mdev-33971, origin/10.6-mdev-33971)
Author: Dave Gosselin <[email protected]>
Date:   Fri Apr 26 12:13:31 2024 -0400

    MDEV-33971 NAME_CONST in WHERE clause replaced by inner item

I'm running this testcase:

create table t1 (
  a varchar(100) collate utf8_unicode_ci,
  b int
);
insert into t1 values ('foo', 1),('bar', 1);
select * from t1;
 
select * from t1  where a= NAME_CONST('var1',_latin1'foo' COLLATE 'latin1_swedish_ci');

drop table t1;

I get:

mysqltest: At line 8: query 'select * from t1  where a= NAME_CONST('var1',_latin1'foo' COLLATE 'latin1_swedish_ci')' failed: ER_CANT_AGGREGATE_2COLLATIONS (1267): Illegal mix of collations (utf8mb3_unicode_ci,IMPLICIT) and (latin1_swedish_ci,EXPLICIT) for operation '='

Trying the same test on the previous commit:

commit 2d5cba22a98e54c0c8975d8d7fe7e04981d73c8a (HEAD)
Author: mariadb-DebarunBanerjee <[email protected]>
Date:   Thu May 16 13:10:44 2024 +0530

I get no error.

[DaveGosselin-MariaDB]

... Done the above and added a couple of comments. What do you think of applying this patch: https://gist.github.com/spetrunia/5236479b5c4d5cd286ff00f914c759a9

After that, the patch looks ok to push.

OK I will look, thank you.

[DaveGosselin-MariaDB]

... Done the above and added a couple of comments. What do you think of applying this patch: https://gist.github.com/spetrunia/5236479b5c4d5cd286ff00f914c759a9

After that, the patch looks ok to push.

Thank you, this is a nice change and I included it. Once the tests are done then I will submit, thank you for the OK.