Skip to content

MDEV-36486 Optimizer hints are resolved against the INSERT part of IN… #3959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

Olernov
Copy link
Contributor

@Olernov Olernov commented Apr 8, 2025

…SERT..SELECT

  • The Jira issue number for this PR is: MDEV-36486

Description

When processing queries like
INSERT INTO t1 (..) SELECT .. FROM t1, t2 ...

there is a single query block (i.e., a single SELECT_LEX) for both INSERT and SELECT parts. During hints resolution, when hints are attached to particular TABLE_LIST's, the search is performed by table name across the whole query block.
So, if a table mentioned in an optimizer hint is present in the INSERT part, the hint is attached to the that tablet. This is obviously wrong as optimizer hints are supposed to only affect the SELECT part of and INSERT..SELECT clause.

This commit disables possible attaching hints to tables in the INSERT part and also disables hints resolution during mysql_prepare_insert_check_table(), which could lead to double resolution and warnings duplication

How can this PR be tested?

Test cases are added to opt_hints.test

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

abarkov and others added 16 commits March 17, 2025 17:55
Implementing a recursive descent parser for optimizer hints.
    This commit introduces:
    - the infrastructure for optimizer hints;
    - hints for join buffering: BNL(), NO_BNL(), BKA(), NO_BKA();
    - NO_ICP() hint for disabling index condition pushdown;
    - MRR(), MO_MRR() hint for multi-range reads control;
    - NO_RANGE_OPTIMIZATION() for disabling range optimization;
    - QB_NAME() for assigning names for query blocks.
- Using Lex_ident_sys to scan identifiers, like the SQL parser does.

  This fixes handling of double-quote-delimited and backtick-delimited identifiers,
  as well as handling of non-ASCII identifiers.

  Unescaping and converting from the client character set to the system
  character set is now done using Lex_ident_cli_st and Lex_ident_sys,
  like it's done in the SQL tokenizer/parser.
  Adding helper methods to_ident_cli() and to_ident_sys()
  in Optimizer_hint_parser::Token.

- Fixing the hint parser to report a syntax error when an empty identifiers:
    SELECT /*+ BKA(``) */ * FROM t1;

- Moving a part of the code from opt_hints_parser.h to opt_hints_parser.cc

  Moving these method definitions:
  - Optimizer_hint_tokenizer::find_keyword()
  - Optimizer_hint_tokenizer::get_token()

  to avoid huge pieces of the code in the header file.

- A Lex_ident_cli_st cleanup
  Fixing a few Lex_ident_cli_st methods to return Lex_ident_cli_st &
  instead of void, to use them easier in the caller code.

- Fixing the hint parser to display the correct line number

  Adding a new data type Lex_comment_st
  (a combination of LEX_CSTRING and a line number)
  Using it in sql_yacc.yy

- Getting rid of redundant dependencies on sql_hints_parser.h

  Moving void LEX::resolve_optimizer_hints() from sql_lex.h to sql_lex.cc

  Adding a class Optimizer_hint_parser_output, deriving from
  Optimizer_hint_parser::Hint_list. Fixing the hint parser to
  return a pointer to an allocated instance of Optimizer_hint_parser_output
  rather than an instance of Optimizer_hint_parser::Hint_list.
  This allows to use a forward declaration of Optimizer_hint_parser_output
  in sql_lex.h and thus avoid dependencies on sql_hints_parser.h.
Forbid adding optimizer hints to view definitions.
In the case when optimizer hints are added to the view definition
at a `CREATE (OR REPLACE) VIEW`/`ALTER VIEW` statement, a warning is
generated and the hints are ignored.

This commit also disables ps-protocol for test cases where
`Unresolved table/index name` warnings are generated. The reason
for this is such warnings are generated during both PREPARE
and EXECUTE stages. Since opt_hints.test has `--enable_prepare_warnings`,
running it with `--ps-protocol` causes duplication of warning messages
join_cache_level=0 disables join cache buffers, but the hint
BNL() now allows to employ BNL(H) buffers for particular tables
or query blocks.

This commit also adds a number of test cases including
OUTER JOINs to make sure hints do not break the rules of
join buffers application
BNL() hint effectively increases join_cache_level up to 4 if it is
set to value less than 4.
This commit also makes the BKA() hint override not only
`join_cache_bka` optimizer switch but `join_cache_level` as well.
I.e., BKA() hint enables BKA and BKAH join buffers both flat and
incremental despite `join_cache_level` and `join_cache_bka` setting.
It places a limit N (a timeout value in milliseconds) on how long
a statement is permitted to execute before the server terminates it.

Syntax:
SELECT /*+ MAX_EXECUTION_TIME(milliseconds) */ ...

Only top-level SELECT statements support the hint.
Since BNL() hint does not specify which whether hashed or non-hashed
join cache should be employed, allow usage of hashed ones
where possible
- add a comment that opt_hints_global->check_unresolved() is never called
- improve comments
- rename everything with "resolved_children" to "fully_resolved_children"
- Opt_hints_table::adjust_key_hints() now returns value
- less "reach-back-to-parent" logic
- rename Hint "adjustment" and "resolution" (yes, both terms were used)
     to "fixing". "Resolution" is already used for parse-tree objects
1. Disable opt_hint_timeout.test for embedded server. Make sure
the test does not crash even when started for embedded server.
Disable view-protocol since hints are not supported inside views.

2. Hints are designed to behave like regular /* ... */ comments:
   `SELECT /*+ " */ 1;` -- a valid SQL query
However, the mysql client program waits for the closing doublequote
character.
Another problem is observed when there is no space character between
closing `*/` of the hint and the following `*`:
   `SELECT /*+ some_hints(...) */* FROM t1;`
In this case the client treats `/*` as a comment section opening and
waits for the closing `*/` sequence.

This commit fixes all of these issues
 - remove get_args_printer() from hints printing
 - add append_hint_arguments(THD *thd, opt_hints_enum hint, String *str)
 - add more comments
 - rename st_opt_hint_info::hint_name to hint_type
 - add pptimizer trace support for hints
 - add dbug_print_hints()
 - make print_warn() not be a template
 - introduce Printable_parser_rule interface, make grammar rules that
     emit warnings implement it  and print_warn invokes its function)
 - remove Parser::Hint::append_args() as it is not used anywhere
     (it used to be necessary call print_warn(... (Parser::Hint*)NULL);
This commit implements optimizer hints allowing to affect the order
of joining tables:

 - JOIN_FIXED_ORDER similar to existing STRAIGHT_JOIN hint;
 - JOIN_ORDER to apply the specified table order;
 - JOIN_PREFIX to hint what tables should be first in the join;
 - JOIN_SUFFIX to hint what tables should be last in the join.
Copy link
Member

@spetrunia spetrunia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

…SERT..SELECT

When processing queries like
  INSERT INTO t1 (..) SELECT .. FROM t1, t2 ...,

there is a single query block (i.e., a single SELECT_LEX) for both INSERT and
SELECT parts. During hints resolution, when hints are attached to particular
TABLE_LIST's, the search is performed by table name across the whole
query block.
So, if a table mentioned in an optimizer hint is present in the INSERT part,
the hint is attached to the that table. This is obviously wrong as
optimizer hints are supposed to only affect the SELECT part of
and INSERT..SELECT clause.

This commit disables possible attaching hints to tables in the INSERT part
and fixes some other bugs related to INSERT..SELECT statements processing
@Olernov Olernov force-pushed the bb-12.0-MDEV-36486-fix-hint branch from b87b08a to 71cadbf Compare April 10, 2025 12:03
@Olernov
Copy link
Contributor Author

Olernov commented Apr 10, 2025

I force-pushed an improved version of the patch as the last one broke some tests

@Olernov Olernov force-pushed the bb-11.8-MDEV-34870-join-order branch from d151643 to 47208f3 Compare April 26, 2025 07:08
@spetrunia
Copy link
Member

Input:

  // INSERT..SELECT allows placing hints next to either INSERT or SELECT, i.e.:

Make the comment use /* ... */ syntax like it's done nearly everywhere.

   If hints are present for both INSERT and SELECT
  // parts, SELECT part hints are preserved while INSERT part hints are discarded

Let's emit a warning if we swallow a hint.
Later on, we can fix not to swallow it, like MySQL 8.4 does..

@Olernov
Copy link
Contributor Author

Olernov commented Apr 28, 2025

Input:

  // INSERT..SELECT allows placing hints next to either INSERT or SELECT, i.e.:

Make the comment use /* ... */ syntax like it's done nearly everywhere.
Not possible here, as we use '/' and '/' inside the comment text and they are misinterpreted by the compiler

@Olernov Olernov force-pushed the bb-11.8-MDEV-34870-join-order branch 2 times, most recently from 6ea5780 to 6cd219a Compare April 29, 2025 09:46
@CLAassistant
Copy link

CLAassistant commented Apr 30, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ Olernov
✅ spetrunia
❌ abarkov
You have signed the CLA already but the status is still pending? Let us recheck it.

@Olernov Olernov closed this Apr 30, 2025
@Olernov Olernov deleted the bb-12.0-MDEV-36486-fix-hint branch April 30, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants