server icon indicating copy to clipboard operation
server copied to clipboard

Delete unused global variables from mysqld.h

Open robinnewhouse opened this issue 1 year ago • 1 comments
trafficstars

Description

Several variables declared in mysqld.h appear to be old system variables that have been left over after deprecation. Delete them using IDE refactoring to automatically search for other uses. Most cases had no other uses in the code.

slave_allow_batching had a test that was effectively unused, as the result was only -ERROR HY000: Unknown system variable 'slave_allow_batching' so that was deleted as well.

Build and test still works without issue as expected.

Examples of variables that were deleted and not cleaned up:

  • 0c571f8 removed the last usage of VERS_ALTER_HISTORY_KEEP but did not delete it from the enum.
  • 7c58e97 moved key_memory_quick_index_merge_root (and similar) to mysqld.cc to mysqld.h, but still was not used anywhere.
  • 23d8586 moved key_thread_kill_server along with others to header file, but still was used nowhere. I can't actually find where it was ever used except being defined in a header.

Release Notes

None

How can this PR be tested?

Building the server and running the MTR test suite shows no failures.

Basing the PR against the correct MariaDB version

  • [x] This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

Note: As discussed with @LinuxJedi, this is targeting the earliest maintained branch to ease merging upwards, even though it is not necessarily a bugfix.

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.

robinnewhouse avatar May 08 '24 00:05 robinnewhouse

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar May 08 '24 00:05 CLAassistant

A general suggestion : can we do pure refactoring task in current development branch, i.e rebase the patch onto 11.6? In the old versions, we usually only fix bug. Large refactoring can make a merge problematic, and between 10.5 and 11.6, there are many merges.

vaintroub avatar Jun 11 '24 15:06 vaintroub

A general suggestion : can we do pure refactoring task in current development branch, i.e rebase the patch onto 11.6? In the old versions, we usually only fix bug. Large refactoring can make a merge problematic, and between 10.5 and 11.6, there are many merges.

I had debated on which branch to target this. Originally it was targeting 11.5 (latest at the time) for exactly this reason. @LinuxJedi had suggested targeting an earlier branch so that later modifications to earlier branches (say a 10.6 bugfix) would face fewer merge conflicts when merging upwards.

I am happy to switch back to targeting the latest development branch if that is what is preferred though.

robinnewhouse avatar Jun 11 '24 21:06 robinnewhouse

A general suggestion : can we do pure refactoring task in current development branch, i.e rebase the patch onto 11.6? In the old versions, we usually only fix bug. Large refactoring can make a merge problematic, and between 10.5 and 11.6, there are many merges.

I had debated on which branch to target this. Originally it was targeting 11.5 (latest at the time) for exactly this reason. @LinuxJedi had suggested targeting an earlier branch so that later modifications to earlier branches (say a 10.6 bugfix) would face fewer merge conflicts when merging upwards.

I am happy to switch back to targeting the latest development branch if that is what is preferred though.

I asked around, and the consensus is refactoring/cosmetics go into newest branch, as to avoid accidental mistakes when mergin g - it already happened that seemingly harmless refactoring causes bugs when merged (I do not think your work would cause something like that, but this is just a guideline). So, refactoring would handled more like new feature, goes into development branch, and unlike bugfixing, which goes into earliest applicable branch. @LinuxJedi agreed to change contribution guideline to explicitly mention refactoring, because currently it only talks about bugs vs new features.

With that, can you maybe base your work on 11.6?

vaintroub avatar Jun 12 '24 12:06 vaintroub

Thank you! Looks mostly good. Can we do pure whitespace fixes in a separate commit (if at all)?

vaintroub avatar Jun 19 '24 21:06 vaintroub

Thank you! Looks mostly good. Can we do pure whitespace fixes in a separate commit (if at all)?

Yes I considered that. I'll remove the pure whitespace changes.

robinnewhouse avatar Jun 19 '24 21:06 robinnewhouse