server icon indicating copy to clipboard operation
server copied to clipboard

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

Open andremralves opened this issue 1 year ago • 5 comments

…alue

  • [x] 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

  • [x] 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

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

andremralves avatar Mar 17 '24 10:03 andremralves

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.

andremralves avatar Mar 17 '24 10:03 andremralves

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 avatar Mar 17 '24 22:03 grooverdan

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.

andremralves avatar Mar 19 '24 12:03 andremralves

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

grooverdan avatar Mar 22 '24 03:03 grooverdan

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: https://github.com/MariaDB/server/blob/f151c5f389e4d5c5bf877d361d406eb1581ad013/storage/maria/ma_write.c#L290-L296

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?

andremralves avatar May 01 '24 11:05 andremralves