rspadim as per your parent issue comment yes only a propper parse will probably do. ./sql/sql_parse.cc:mysql_parse contains some FIXMEs that seem to indicate this. Attempting to parse the query as just a SELECT before deciding on query_cache_send_result_to_client is one way, but looking at the bison generated parse code I'm out of my depth.

rspadim,

Can you provide a test case which demonstrates the actual problem?

hi Elena, well i can't "explain" with tests, but i can "explain" the "problem"
before checking query at query cache we execute a "string search" instead of parser+check sql_no_cache/sql_cache hint

1) sql query -> check sql_cache/sql_no_cache hint -> seach query at query cache (SQL_CACHE) -> parser -> execute query -> return/save at query cache
2) sql query -> check sql_cache/sql_no_cache hint -> parser (SQL_NO_CACHE) -> execute query -> return
3) sql query -> check sql_cache/sql_no_cache hint -> seach query at query cache (SQL_NO_CACHE at wrong order) -> parser -> execute query -> return/save at query cache

string search is faster and consume less cpu/memory than parser, that's not a bug/problem, but since we only execute string search we don't know if SQL_CACHE/SQL_NO_CACHE is really a "hint" or just a comment or something like it, and if it's not at the right order "SELECT SQL_CACHE" it don't optimize the query cache "check"
for example "SELECT SQL_NO_CACHE ..." will not search query at query cache
but "SELECT SQL_SMALL_RESULT SQL_NO_CACHE ..." will search query at query cache cause SQL_NO_CACHE isn't after SELECT

in other words:
4) sql query -> parser -> check sql_cache/sql_no_cache hint (SQL_CACHE) -> seach query at query cache -> execute query -> return/save at query cache
5) sql query -> parser -> check sql_cache/sql_no_cache hint (SQL_NO_CACHE) -> execute query -> return

rspadim,

But shouldn't at least (3) be very easy to reproduce, if it's so?
QCache_inserts and QCache_queries_in_cache should show if SELECT SQL_SMALL_RESULT SQL_NO_CACHE ... is actually saved at query cache, or QCache_hits will show if it's taken from the query cache?

short answer:
>But shouldn't at least (3) be very easy to reproduce, if it's so?

it can be reproduced but not easily checked

>QCache_inserts and QCache_queries_in_cache should show if SELECT SQL_SMALL_RESULT SQL_NO_CACHE ... is actually saved at query cache, or QCache_hits will show if it's taken from the query cache?

no we don't have a status variable to sum how many queries was searched at query cache
we have QCache_hits, Qcache_queries_in_cache and Queries variables, but not a QCache_searchs variable

full answer:
well thinking about a test case, we have a problem at (7,9):

1) CREATE DATABASE IF NOT EXISTS test
1) USE test (or another database?)
2) DROP TABLE IF EXISTS qcache_test
3) CREATE TABLE IF NOT EXISTS qcache_test (i int) ENGINE=MYISAM
4) SET query_cache_size=10000000, query_cache_type=2; (i'm using 2 to ensure that only queries with SQL_CACHE will be cached)
5) RESET QUERY CACHE
6) SELECT SQL_CACHE * FROM qcache_test
7) SELECT SQL_NO_CACHE * FROM qcache_test
8) SHOW STATUS LIKE 'Qcache_queries_in_cache' (should return = 1)
—
there isn't a status variable to sum how many checks to query cache search was executed (Query_cache::send_result_to_client calls), i will call it as "QCache_searchs" status var
—
9) SHOW STATUS LIKE "QCache_searchs"
9.1) here it MUST be 1 (query 6)
9.2) more than 1 we have a problem (maybe we have a search with SQL_NO_CACHE hint? or others queries calling send_result_to_client function? or a bug?)
9.3) less than 1 we don't have query cache enabled, or engine don't work with query cache, or database don't have query cache at compile time, or a bug?

well for test case i think it's ok, what you think?

queries with SQL_NO_CACHE is never cached after query execution that could be tested with a qcache_info plugin, or "Qcache_queries_in_cache" variable after reseting query cache and executing two queries one with SQL_NO_CACHE and one with SQL_CACHE, queries cached must be = 1
—

ideas from this MDEV:
when we execute SELECT with SQL_NO_CACHE we could avoid query cache without parser (using SELECT SQL_NO_CACHE ....)
https://github.com/rspadim/server/blob/a5f8ce91bfe1cc1cef3d70346c346cd48b100eb4/sql/sql_cache.cc#L811 (has_no_cache_directive function)

to solve this useless search in any case, database should execute parser before query cache check and check if SQL_NO_CACHE exists

or ignore (reducing cpu/memory use with parser before a cached query) and reporting a warning about SQL_NO_CACHE, for example:
"SELECT SQL_SMALL_RESULT SQL_NO_CACHE/SQL_CACHE ..." could return rows + a warning "SQL_CACHE/SQL_NO_CACHE should be used after SELECT to avoid useless query cache search"

SQL_NO_CACHE in the query cache is just fast check to see it before parser (if we do not cache it so probably no sens to check cache because none of the query with SQL_NO_CACHE could be found in the cache) the check could fail, but then parser will care.

Other misunderstanding here that SQL_CACHE could force caching in case of SQL_NO_CACHE:
https://dev.mysql.com/doc/refman/5.1/en/query-cache-in-select.html
says that SQL_NO_CACHE prevent caching (in any case)

SQL_CACHE works only for on demand cache.

My opinion is that it is not a bug.

If I understand Roberto's initial complaint correctly, he says that the current algorithm is sub-optimal performance-wise. However, currently it's just a theory. rspadim, if you can provide a proof-of-concept where the performance improvement from your suggestion will be clearly seen, please comment with it to re-open the issue. For now, I am closing it as sanja suggested.

hi guys
—
example 1) (with query cache on or demand)
1 query cached:
SELECT SQL_CACHE * FROM TABLE_1;

if i execute
SELECT SQL_CACHE * FROM TABLE_1;
the query should be checked at query cache and returned

—
example 2) (with query cache demand)
0 query cached:

if i execute
SELECT * FROM TABLE_1;
the query should NOT be checked at query cache and returned

—
example 3) (with query cache demand / query cache on)
0 query cached:

if i execute
SELECT SQL_NO_CACHE * FROM TABLE_1;
the query should NOT be checked at query cache and returned

—
the problem is
if we execute this: with query cache on/demand
SELECT SQL_SMALL_RESULT SQL_NO_CACHE * FROM TABLE_1;
the query will be checked

should be interesting report a warning to db client telling that SQL_NO_CACHE should be used after SELECT, avoiding a query cache check
that's not a bug, but a feature request

—
the query flow:
receive query -> check query cache (SQL_NO_CACHE/SQL_CACHE change this flow when possible) -> return from cache or not -> return from optimizer/storage

the idea is always report warnings when using SQL_NO_CACHE and "check query cache" happen (when the SQL_NO_CACHE isn't after SELECT)
or execute parser before query cache (i don't like this idea) and check SQL_NO_CACHE

sanja,

Looking at the syntax of SELECT command and definition of SQL_CACHE/SQL_NO_CACHE options, formally the complaint is justified (given that it's valid technically, I did not actually check). So, I think the better way to close this bug is probably not as 'Not a bug', but as 'Won't fix', and maybe add a note about it to our documentation to avoid further arguments about it.

The only good reason to fix this bug would be a presence of a quantifiable proof that it would give considerable performance improvement, which I don't see so far.

rspadim, do you have a patch which fixes the problem you are describing and shows the performance improvement?

this MDEV is related to MDEV-6631

no i didn't included a patch that create warnings after parser, this should be something like:

parser
check if SQL_NO_CACHE or SQL_CACHE exists,
if yes, check if SQL_CACHE/NO_CACHE is after >>>FIRST!<<< SELECT
if not, create a warning

why FIRST? SELECT SQL_CACHE ..... FROM (SELECT SQL_NO_CACHE ...) AS t
this should report a warning, SQL_NO_CACHE should be at first SELECT not after second, parser and optimizer can execute this query without problem, but query cache check will be executed even with a SQL_NO_CACHE hint

about performance, query cache check is only executed before parser, if we check query cache every time even with SQL_NO_CACHE we have a performace loss, i don't know if it's a O(1) O O(n^2) O(log n) O(n log n), i didn't tested, but it's a performace loss and easy to 'solve'/help developer, even if we couldn't not check query cache we have a performace loss at query cache lock timeout (something like 30us must check query cache source code)

MDEV-6631 show performace loss!

rspadim,
As discussed many times before, please don't spread information about one problem across different JIRA entries, and don't put different problems in one entry, it's really impossible to track or takes way too much time which we simply can't afford.
If this issue is the same as MDEV-6631, the problem should be fixed in MDEV-6631 and this one should be closed as a duplicate.
If this issue is not the same as MDEV-6631, then it's unclear why MDEV-6631 describes performance degradation which relates to the problem described here, and how to extract that particular piece of information from MDEV-6631, which is also full of scattered thoughts and side observations.

Same goes for "feature requests" you were coming up with while commenting on this issue, they will simply be ignored, because they have nothing to do with the original complaint.

All in all, theories are great, and undoubtedly every additional operation is a performance loss, but unless we have a quantifiable proof of performance loss that is meaningful in real life, it will stand little to no chance to be fixed.

mdev 6631 -> avoid when possible query cache check with SQL_CACHE / SQL_NO_CACHE hints (in this report that we checked query cache when it shouldn't be checked)
mdev 7132 -> what should be done? report warning or execute parser before query cache check

1) SQL parser check it with any number of spaces and comments (see sql/sql_yacc.yy which is the parser description)
2) What you refer is an optimization to catch most common cases. if make it more complex it will slow down all other queries so it not acceptable.

nice, no problem, should we document that sql_no_cache should be after first select to avoid one query cache check?