Skip to content

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

Merged
bsrikanth-mariadb merged 1 commit into
bb-12.3-MDEV-39368-test-replayfrom
13.0-MDEV-39405-store-necessary-plugin-engines-costs
May 22, 2026
Merged

MDEV-39405: store the necessary plugin-engines optimizer costs#5077
bsrikanth-mariadb merged 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

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

Copy link
Copy Markdown
Contributor

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 Outdated
@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

spetrunia commented May 18, 2026

Copy link
Copy Markdown
Member

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 2 times, most recently 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.

@bsrikanth-mariadb
bsrikanth-mariadb force-pushed the 13.0-MDEV-39405-store-necessary-plugin-engines-costs branch 4 times, most recently from 2f7dd2c to 6bf2c66 Compare May 22, 2026 12:01
@bsrikanth-mariadb

Copy link
Copy Markdown
Contributor Author

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?

Actually it is not required now. Changed the logic.

Comment thread sql/opt_context_store_replay.cc Outdated
static const char *excluded_sys_vars[]= {"optimizer_replay_context",
"optimizer_record_context", NULL};

struct sys_vars_ctx

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.

sys_vars_ctx is not used anymore.

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, removed it.

break;
}
store_optimizer_costs(engine_name->str,
(OPTIMIZER_COSTS *) hton->optimizer_costs,

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.

Is typecast needed? if not, please remove.

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, it is required

@spetrunia spetrunia left a comment

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.

Ok to push after the above is addressed.

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 6bf2c66 to 4bad3f1 Compare May 22, 2026 14:40
@bsrikanth-mariadb
bsrikanth-mariadb merged commit 4bad3f1 into bb-12.3-MDEV-39368-test-replay May 22, 2026
1 of 2 checks passed
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