Skip to content
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

MDEV-33660: Warns the user if they try to set auto_increment <= max_v… #3131

Open
wants to merge 1 commit into
base: 11.5
Choose a base branch
from

Conversation

andremralves
Copy link
Contributor

…alue

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

Description

This PR adds a warning for the cases where ALTER TABLE .. AUTO_INCREMENT = X is set to a value lower than or equal to the current max_value in the table.

MariaDB [test]> alter table t1 auto_increment=3;
Query OK, 0 rows affected, 1 warning (0.012 sec)
Records: 0  Duplicates: 0  Warnings: 1

MariaDB [test]> show warnings\G
*************************** 1. row ***************************
  Level: Warning
   Code: 167
Message: Can not set AUTO_INCREMENT to 3 which is lower than or equal to the current max value in the table: 5
1 row in set (0.000 sec)

Points to review:
Is the error code HA_ERR_AUTOINC_ERANGE ok or should I use/create another one?

Release Notes

Warns the user if they try to set auto_increment to a value lower than or equal max_value.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

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 and the PR is based against the latest MariaDB development 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.

@andremralves
Copy link
Contributor Author

Is it necessary to add tests for this kind of change? I'm thinking about a test that checks if a warning was generated, but I couldn't find any example.

@grooverdan
Copy link
Member

Should be possible to add to mysql-test/main/auto_increment_ranges.inc. This is included by the auto_increment_ranges_innodb and auto_increment_ranges_myisam tests.

From the test results here, tests innodb.innodb-autoinc-44030 vcol.innodb_autoinc_vcol innodb.autoinc_persist main.alter_table_autoinc-5574 also need to be re-recorded.

mysql-test/mtr --record {testname} and look at the diff in the result to ensure that it is correct. Then commit amend and force push to the same branch here.

You'll note that aria (storage/maria) and myisam and memory engine (storage/heap) code also need to generate the same warning.

@grooverdan grooverdan added the need feedback Can the contributor please address the questions asked. label Mar 19, 2024
@andremralves
Copy link
Contributor Author

Hi @grooverdan, thanks for your feedback it helped me a lot. I've updated the tests results, but I'm still having a hard time to figure out how to generate the warnings for the other engines like myisam. In innodb I was using the function push_warning_printf to generate a new warning while in myisam I thought the equivalent function would be mi_check_print_warning, but that doesn't seem to be the case.

@grooverdan
Copy link
Member

push_warning_printf still can be used in MyISAM/Aria. Its not used frequently but still can be.

@andremralves
Copy link
Contributor Author

Hi, sorry for the late response. I got a little bit stuck on this task and started doing other tasks to become more familiar with the codebase and then come back here.

I felt that the way other engines, like Aria, handles autoincrement is a little bit different from Innodb. While in Innodb there was a piece of code that compares the max_value in the table with the new autoincrement - and I could just add a warning there. In Aria I couldn't find a similar comparison, I think we just call maria_write for each row and update the auto_increment as we go. If the current row id is greater than auto_increment we update it. That makes adding a warning a little difficult since maria_write is called multiple times.

Here is the piece of code I'm referring to:

if (share->base.auto_key != 0)
{
const HA_KEYSEG *keyseg= share->keyinfo[share->base.auto_key-1].seg;
const uchar *key= record + keyseg->start;
set_if_bigger(share->state.auto_increment,
ma_retrieve_auto_increment(key, keyseg->type));
}

push_warning_printf still can be used in MyISAM/Aria. Its not used frequently but still can be.

I tried to use it, but I got some errors. Also, I couldn't find any examples of push_warning_printf being used in C files. Is there some other options?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need feedback Can the contributor please address the questions asked.
2 participants