Skip to content

MDEV-39405: store the necessary plugin-engines optimizer costs#5077

Open
bsrikanth-mariadb wants to merge 1 commit into
bb-12.3-MDEV-39368-test-replayfrom
13.0-MDEV-39405-store-necessary-plugin-engines-costs
Open

MDEV-39405: store the necessary plugin-engines optimizer costs#5077
bsrikanth-mariadb wants to merge 1 commit into
bb-12.3-MDEV-39368-test-replayfrom
13.0-MDEV-39405-store-necessary-plugin-engines-costs

Conversation

@bsrikanth-mariadb
Copy link
Copy Markdown
Contributor

store the necessary plugin-engines optimizer costs that are used by the query into the context, so that the replay server uses the same while computing query cost

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request enhances the optimizer context replay mechanism by capturing and storing storage engine-specific optimizer costs. It introduces logic to identify relevant storage engines for a query and generates the corresponding global configuration statements. The review feedback correctly identifies several potential memory leaks where hashes are not freed during early returns and suggests optimizations for the cost-storing logic to improve efficiency and safety.

Comment thread sql/opt_context_store_replay.cc
Comment thread sql/opt_context_store_replay.cc
@bsrikanth-mariadb bsrikanth-mariadb force-pushed the 13.0-MDEV-39405-store-necessary-plugin-engines-costs branch from c0d48c6 to c77a130 Compare May 14, 2026 12:14
Comment thread sql/opt_context_store_replay.cc Outdated
res= true; // OOM
}

if (!res && thd->variables.table_plugin)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had to look up what is this. It seems to be the plugin for @@default_storage_engine.
Any reason why we need it in the context?

I assume we need plugins for the tables we're querying, plugin for in-memory temp.table, plugin for on-disk temp.table.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this should be cleaned.

Comment thread sql/opt_context_store_replay.cc Outdated
if (!res && thd->variables.table_plugin)
{
const LEX_CSTRING *engine_name= plugin_name(thd->variables.table_plugin);
DB_NAME_KEY *engine_name_key;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the point of using DB_NAME_KEY here? It looks confusing.
Why not LEX_STRING?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or LEX_CSTRING ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, used LEX_CSTRING. Also removed TABLE_NAME_KEY, and DB_NAME_KEY, and instead using LEX_CSTRING there as well.

@spetrunia
Copy link
Copy Markdown
Member

spetrunia commented May 18, 2026

When we're looking at the query's tables:

  const LEX_CSTRING *engine_name= plugin_name(tbl->table->s->db_plugin);

We can get the plugin name, table's hton and hton->optimizer_costs.
This seems to be everything that we need to dump the costs.
(I assume the plugin is locked at this point because the query uses it. Is this correct? )

Question: why do we need to iterate plugins separately and compare name to see if the plugin was used, etc?

@spetrunia
Copy link
Copy Markdown
Member

create table t20 (a int, b int) engine=rocksdb partition by hash(a) partitions 2;
set optimizer_record_context=1;
select * from t20 where a=333;
select * from information_schema.optimizer_context\G

no ROCKSDB.OPTIMIZER_DISK_READ_COST here...
I saw it inserting "partition" as the plugin name.

create table t21 (a int, b int) engine=rocksdb;
set optimizer_record_context=1;
select * from t21 where a=333;
select * from information_schema.optimizer_context\G

Now I can see: SET GLOBAL ROCKSDB.OPTIMIZER_DISK_READ_COST=10.24

Check handler->partition_ht().

@spetrunia spetrunia self-requested a review May 18, 2026 12:51
@bsrikanth-mariadb bsrikanth-mariadb force-pushed the 13.0-MDEV-39405-store-necessary-plugin-engines-costs branch from c77a130 to 8d9199e Compare May 19, 2026 08:55
store the necessary plugin-engines optimizer costs that are used by the query
into the context, so that the replay server uses the same while computing
query cost
@bsrikanth-mariadb bsrikanth-mariadb force-pushed the 13.0-MDEV-39405-store-necessary-plugin-engines-costs branch from 8d9199e to f9cc1a9 Compare May 20, 2026 13:26
@bsrikanth-mariadb
Copy link
Copy Markdown
Contributor Author

create table t20 (a int, b int) engine=rocksdb partition by hash(a) partitions 2;
set optimizer_record_context=1;
select * from t20 where a=333;
select * from information_schema.optimizer_context\G

no ROCKSDB.OPTIMIZER_DISK_READ_COST here... I saw it inserting "partition" as the plugin name.

create table t21 (a int, b int) engine=rocksdb;
set optimizer_record_context=1;
select * from t21 where a=333;
select * from information_schema.optimizer_context\G

Now I can see: SET GLOBAL ROCKSDB.OPTIMIZER_DISK_READ_COST=10.24

Check handler->partition_ht().

Yes, using handler->partition_ht() is giving the underlying storage engine name for both partitioned and non-partitioned tables.

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.

2 participants