Skip to content

MDEV-40388: sequence.simple fails on replay#5427

Open
bsrikanth-mariadb wants to merge 1 commit into
bb-12.3-MDEV-39368-test-replay-preview-treefrom
13.1-MDEV-40388-sequence.simple-fails-on-replay
Open

MDEV-40388: sequence.simple fails on replay#5427
bsrikanth-mariadb wants to merge 1 commit into
bb-12.3-MDEV-39368-test-replay-preview-treefrom
13.1-MDEV-40388-sequence.simple-fails-on-replay

Conversation

@bsrikanth-mariadb

@bsrikanth-mariadb bsrikanth-mariadb commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

When recording is enabled for the query such as,
explain select * from seq_1_to_10;
it recorded the table context having a DDL definition as: -

CREATE TABLE seq_1_to_10 (
-> seq bigint(20) unsigned NOT NULL,
-> PRIMARY KEY (seq)
-> ) ENGINE=SEQUENCE DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_uca1400_ai_ci;

Now, when that context is replayed, the DDL statement is executed.
But, we cannot create such a table, and instead it errors out saying
ERROR 1050 (42S01): Table 'seq_1_to_10' already exists.

Solution is to not record a DDL statement or any stats for sequence
tables such as seq_1_to_10.

There is a different way to use sequences as: -
Create sequence s1;
Explain select * from s1;

Here, we should be recording the DDL statement, but no need to store the
stats for it.

@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 updates the logic in Optimizer_context_recorder::dump_sql_script to identify sequence tables by checking if the storage engine name is "SEQUENCE" instead of checking the sequence flag on the table share. The reviewer suggested a performance and code-simplification improvement: accessing the storage engine's handlerton directly from the table share (tbl->table->s->db_type) instead of calling the virtual method partition_ht() on the handler.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread sql/opt_context_store_replay.cc Outdated
Comment on lines +749 to +755
if (!tbl->is_view() && tbl->table)
{
handlerton *hton= tbl->table->file->partition_ht();
const LEX_CSTRING *engine_name= hton_name(hton);
if (strcmp(engine_name->str, "SEQUENCE") == 0)
continue;
}

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.

medium

Instead of calling the virtual method partition_ht() on the handler (tbl->table->file), you can directly access the storage engine's handlerton from the table share (tbl->table->s->db_type). This is more direct, avoids a virtual function call, and simplifies the code.

    if (!tbl->is_view() && tbl->table)
    {
      const LEX_CSTRING *engine_name= hton_name(tbl->table->s->db_type);
      if (strcmp(engine_name->str, "SEQUENCE") == 0)
        continue;
    }

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.

actually here gemini absolutely right, why not tbl->table->file ?

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.

Why it is SQL_SEQUENCE or SEQUENCE if you need probably one, or could you say what is the difference?

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.

maybe use lex_string_eq ?

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.

Why it is SQL_SEQUENCE or SEQUENCE if you need probably one, or could you say what is the difference?

SEQUENCE is for the following eg: -
explain select * from seq_15_to_1_step_12345;

and SQL_SEQUENCE is for the following eg: -
create sequence s1;
EXPLAIN select * from s1;

@bsrikanth-mariadb bsrikanth-mariadb Jul 22, 2026

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.

actually here gemini absolutely right, why not tbl->table->file ?

For this example
create sequence s1;
EXPLAIN select * from s1;

const LEX_CSTRING *engine_name= hton_name(tbl->table->s->db_type());
is seen as InnoDB or MyISAM, and not SQL_SEQUENCE or SEQUENCE

@bsrikanth-mariadb
bsrikanth-mariadb force-pushed the 13.1-MDEV-40388-sequence.simple-fails-on-replay branch 2 times, most recently from d78faae to b2f5d98 Compare July 22, 2026 00:48
@sanja-byelkin
sanja-byelkin requested a review from Copilot July 22, 2026 09:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes MDEV-40388 by ensuring optimizer context recording/replay does not attempt to capture DDL/stats for sequence tables, which can break replay (e.g., sequence.simple).

Changes:

  • Update Optimizer_context_recorder::dump_sql_script() to skip sequence tables based on storage engine name instead of TABLE_SHARE::sequence.
  • Add a regression test case to verify sequence tables are not recorded in optimizer context output.
  • Update the expected result file accordingly.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
sql/opt_context_store_replay.cc Changes sequence-table detection logic to skip recording sequence tables during context dump.
mysql-test/main/opt_context_store_stats.test Adds a regression test query against a sequence under optimizer_record_context=ON.
mysql-test/main/opt_context_store_stats.result Records expected output for the new regression test section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sql/opt_context_store_replay.cc Outdated
Comment on lines +749 to +758
if (!tbl->is_view() && tbl->table)
{
handlerton *hton= tbl->table->file->partition_ht();
const LEX_CSTRING *engine_name= hton_name(hton);
if (strcmp(engine_name->str, "SEQUENCE") == 0 ||
strcmp(engine_name->str, "SQL_SEQUENCE") == 0)
{
continue;
}
}
Comment thread mysql-test/main/opt_context_store_stats.result
Comment thread mysql-test/main/opt_context_store_stats.test

@sanja-byelkin sanja-byelkin 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.

The direction is correct, but please fix small things and answer questions

Comment thread sql/opt_context_store_replay.cc Outdated
Comment on lines +749 to +755
if (!tbl->is_view() && tbl->table)
{
handlerton *hton= tbl->table->file->partition_ht();
const LEX_CSTRING *engine_name= hton_name(hton);
if (strcmp(engine_name->str, "SEQUENCE") == 0)
continue;
}

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.

actually here gemini absolutely right, why not tbl->table->file ?

Comment thread sql/opt_context_store_replay.cc Outdated
Comment on lines +749 to +755
if (!tbl->is_view() && tbl->table)
{
handlerton *hton= tbl->table->file->partition_ht();
const LEX_CSTRING *engine_name= hton_name(hton);
if (strcmp(engine_name->str, "SEQUENCE") == 0)
continue;
}

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.

Why it is SQL_SEQUENCE or SEQUENCE if you need probably one, or could you say what is the difference?

Comment thread mysql-test/main/opt_context_store_stats.test
Comment thread mysql-test/main/opt_context_store_stats.test Outdated
Comment thread sql/opt_context_store_replay.cc Outdated
Comment on lines +749 to +755
if (!tbl->is_view() && tbl->table)
{
handlerton *hton= tbl->table->file->partition_ht();
const LEX_CSTRING *engine_name= hton_name(hton);
if (strcmp(engine_name->str, "SEQUENCE") == 0)
continue;
}

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.

maybe use lex_string_eq ?

@bsrikanth-mariadb
bsrikanth-mariadb force-pushed the 13.1-MDEV-40388-sequence.simple-fails-on-replay branch from b2f5d98 to 5abaa6b Compare July 22, 2026 11:00
@vuvova

vuvova commented Jul 22, 2026

Copy link
Copy Markdown
Member

Why the test fails on replay? Your commit comment doesn't explain anything besides repeating what the code does.

@vuvova
vuvova self-requested a review July 22, 2026 13:13
@bsrikanth-mariadb

Copy link
Copy Markdown
Contributor Author

Why the test fails on replay? Your commit comment doesn't explain anything besides repeating what the code does.

When the recording is enabled for the query
explain select * from seq_15_to_1_step_12345; it recorded the table context having a DDL definition as: -

CREATE TABLE `seq_15_to_1_step_12345` (
    ->   `seq` bigint(20) unsigned NOT NULL,
    ->   PRIMARY KEY (`seq`)
    -> ) ENGINE=SEQUENCE DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_uca1400_ai_ci;

Now, when that context is replayed, the DDL statement is executed. But, we cannot create such the table and instead it errors out
ERROR 1050 (42S01): Table 'seq_15_to_1_step_12345' already exists

@bsrikanth-mariadb
bsrikanth-mariadb force-pushed the 13.1-MDEV-40388-sequence.simple-fails-on-replay branch 2 times, most recently from fe2e034 to a9983bf Compare July 22, 2026 14:40
Comment thread sql/opt_context_store_replay.cc Outdated
@bsrikanth-mariadb
bsrikanth-mariadb force-pushed the 13.1-MDEV-40388-sequence.simple-fails-on-replay branch 2 times, most recently from 53e01d6 to 9636bdf Compare July 24, 2026 08:15
@mariadb-pavithrapandith

Copy link
Copy Markdown

Verified: It fixes below tests
sequence.simple
sequence.group_by

Comment thread sql/opt_context_store_replay.cc Outdated
@bsrikanth-mariadb
bsrikanth-mariadb force-pushed the 13.1-MDEV-40388-sequence.simple-fails-on-replay branch 2 times, most recently from 77103a4 to ee1b2a8 Compare July 24, 2026 10:24
@vuvova

vuvova commented Jul 24, 2026

Copy link
Copy Markdown
Member

I suspect the best approach here is to use CREATE IF NOT EXISTS.

If you absolutely must have a check, check for hton->discover_table != NULL, but I don't think it's needed.

This was about seq_15_to_1_step_12345. Why do you check for "SQL_SEQUENCE" ? It doesn't auto-discover.

@bsrikanth-mariadb

Copy link
Copy Markdown
Contributor Author

I suspect the best approach here is to use CREATE IF NOT EXISTS.

If you absolutely must have a check, check for hton->discover_table != NULL, but I don't think it's needed.

This was about seq_15_to_1_step_12345. Why do you check for "SQL_SEQUENCE" ? It doesn't auto-discover.

"CREATE IF NOT EXISTS", might not be appropriate for all the situations. For "SEQUENCE" it should be fine. But, for user tables, if the replay environment already has a table with that name, then its definition could be entirely different from the table used in replay query.

Yes, hton->discover_table != NULL is working for "SEQUENCE" situation. But, for "SQL_SEQUENCE", it is NULL, hence I am planning to using existing condition tbl->table->s->sequence.

@vuvova

vuvova commented Jul 26, 2026

Copy link
Copy Markdown
Member

"CREATE IF NOT EXISTS", might not be appropriate for all the situations. For "SEQUENCE" it should be fine. But, for user tables, if the replay environment already has a table with that name, then its definition could be entirely different from the table used in replay query.

It can be for any discovering engine, you never know what definition the table will have.
So either you trust that if the table with the same name exists or is discovered, then it has the correct structure — which is practically always the case — or you must verify it. Either way, trusting discovering engines and not trusting others makes no sense.

Yes, hton->discover_table != NULL is working for "SEQUENCE" situation. But, for "SQL_SEQUENCE", it is NULL, hence I am planning to using existing condition tbl->table->s->sequence.

You didn't answer what was the problem with sequences, why do you need to have special handling for them?

@bsrikanth-mariadb

bsrikanth-mariadb commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

"CREATE IF NOT EXISTS", might not be appropriate for all the situations. For "SEQUENCE" it should be fine. But, for user tables, if the replay environment already has a table with that name, then its definition could be entirely different from the table used in replay query.

It can be for any discovering engine, you never know what definition the table will have. So either you trust that if the table with the same name exists or is discovered, then it has the correct structure — which is practically always the case — or you must verify it. Either way, trusting discovering engines and not trusting others makes no sense.

Yes, hton->discover_table != NULL is working for "SEQUENCE" situation. But, for "SQL_SEQUENCE", it is NULL, hence I am planning to using existing condition tbl->table->s->sequence.

You didn't answer what was the problem with sequences, why do you need to have special handling for them?

One point to consider for this feature is that, we only try to capture as much context as possible for all the base tables that were used in the "QUERY" we are interested in. We want the replay environment to be clean and only use the context that was recorded earlier. So, if the replay environment already had tables with same names, then it would definitely interfere with the replay of that query. So, we DROP those tables if they exist.

For "SEQUENCE", we don't need to capture context for those tables, as they are available in the replay environment. But, it was getting captured now, which is what I want to avoid.

I think "SEQUENCE" is a virtual table as in "SELECT * from seq_1_to_2", and we can't use a "CREATE STATEMENT" to create one. So, no need to capture this in the query context.

MariaDB [test]> show tables;
Empty set (0.001 sec)

MariaDB [test]> show create table seq_1_to_2;
| Table      | Create Table                                                                                                                                                    |
| seq_1_to_2 | CREATE TABLE `seq_1_to_2` (
  `seq` bigint(20) unsigned NOT NULL,
  PRIMARY KEY (`seq`)
) ENGINE=SEQUENCE DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_uca1400_ai_ci |
1 row in set (0.001 sec)

MariaDB [test]> CREATE TABLE `seq_1_to_2` (   `seq` bigint(20) unsigned NOT NULL,   PRIMARY KEY (`seq`) ) ENGINE=SEQUENCE DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_uca1400_ai_ci;
ERROR 1050 (42S01): Table 'seq_1_to_2' already exists

The ENGINE used for "select * from seq_1_to_2" is "SEQUENCE", and the "discover_table" is defined for this one (and 3 others such as ha_archive, ha_s3, and ha_perfschema).

However, The ENGINE used for "CREATE SEQUENCE s1" is either "InnoDB", or "MyISAM" depending on the default engine type specified, and such engines doesn't have "discover_table" defined. So, we can use "tbl->table->s->sequence" field that was set for these.

MariaDB [test]> create sequence s1;
Query OK, 0 rows affected (0.010 sec)

MariaDB [test]> show create table s1;
| Table | Create Table|
s1    | CREATE TABLE `s1` (
  `next_not_cached_value` bigint(21) NOT NULL,
  `minimum_value` bigint(21) NOT NULL,
  `maximum_value` bigint(21) NOT NULL,
  `start_value` bigint(21) NOT NULL COMMENT 'start value when sequences is created or value if RESTART is used',
  `increment` bigint(21) NOT NULL COMMENT 'increment value',
  `cache_size` bigint(21) unsigned NOT NULL,
  `cycle_option` tinyint(1) unsigned NOT NULL COMMENT '0 if no cycles are allowed, 1 if the sequence should begin a new cycle when maximum_value is passed',
  `cycle_count` bigint(21) NOT NULL COMMENT 'How many cycles have been done'
) ENGINE=MyISAM SEQUENCE=1

@bsrikanth-mariadb
bsrikanth-mariadb force-pushed the 13.1-MDEV-40388-sequence.simple-fails-on-replay branch from ee1b2a8 to b7f3119 Compare July 27, 2026 06:44
When recording is enabled for the query such as,
explain select * from seq_1_to_10;
it recorded the table context having a DDL definition as: -

CREATE TABLE `seq_1_to_10` (
    ->   `seq` bigint(20) unsigned NOT NULL,
    ->   PRIMARY KEY (`seq`)
    -> ) ENGINE=SEQUENCE DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_uca1400_ai_ci;

Now, when that context is replayed, the DDL statement is executed.
But, we cannot create such a table, and instead it errors out saying
ERROR 1050 (42S01): Table 'seq_1_to_10' already exists.

Solution is to not record a DDL statement or any stats for sequence
tables such as seq_1_to_10.

There is a different way to use sequences as: -
  Create sequence s1;
  Explain select * from s1;

Here, we should be recording the DDL statement, but no need to store the
stats for it.
@bsrikanth-mariadb
bsrikanth-mariadb force-pushed the 13.1-MDEV-40388-sequence.simple-fails-on-replay branch from b7f3119 to 29f52f7 Compare July 27, 2026 06:47
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.

6 participants