server icon indicating copy to clipboard operation
server copied to clipboard

don't require resolveip if it won't be used

Open mscdex opened this issue 1 year ago • 4 comments

  • [ ] The Jira issue number for this PR is: MDEV-______

Description

This avoids checking for resolveip if it won't be used by using the same conditional used before deciding to execute resolveip. I ran into this while reducing mariadb's installation footprint.

Release Notes

How can this PR be tested?

Install without resolveip?

Basing the PR against the correct MariaDB version

Eh I'm not really sure which one this falls under.

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

mscdex avatar May 12 '24 07:05 mscdex

Hi @mscdex, thanks for the contribution!

  • Could you also add an MTR test showing the behaviour with different options (--force, etc.)?
  • I'm also curious if this results in measurable improvements in installation performance. Do you have any results showing total installation time?

robinnewhouse avatar May 14 '24 19:05 robinnewhouse

  • Could you also add an MTR test showing the behaviour with different options (--force, etc.)?

I'm not sure exactly what you're proposing here. Can you explain a bit more?

  • I'm also curious if this results in measurable improvements in installation performance. Do you have any results showing total installation time?

I do not. I was more concerned about just being able to successfully initialize mariadb for the first time. I don't utilize hostnames for users, so either way it's a non-issue for me.

mscdex avatar May 14 '24 19:05 mscdex

I guess the question is, what problem does this solve? when you say "reducing mariadb's installation footprint", how does that manifest?

I suggest an MTR test as it's a useful way to show the behaviour your change fixes. I see there is one for testing the mysql_install_db script on windows, so that might provide a starting point. https://github.com/MariaDB/server/blob/11.5/mysql-test/main/mysql_install_db_win.test

robinnewhouse avatar May 14 '24 22:05 robinnewhouse

I guess the question is, what problem does this solve?

I'm having to patch the various CMakeFiles in order to remove stuff that I don't need/want in order to reduce the overall footprint of MariaDB since there are no flags to toggle all of the things I'm removing. resolveip ends up (indirectly) being one of those casualties as a result.

Either way, IMO it makes sense to not check that something exists if it won't be used (when any of those special conditions are met).

I suggest an MTR test as it's a useful way to show the behaviour your change fixes.

The problem is this would require deleting/moving resolveip and ensuring mysql_install_db doesn't fail. After looking through the documentation and some of the existing tests it's not immediately clear how that would be accomplished. Not only that, I wasn't even able to find a definitive list of variables (especially for all relevant directories so I would know how to properly delete/move resolveip, as that would depend on installation prefixes and such) exposed to all tests. On top of that, I've never written a line of Perl in my life.

mscdex avatar May 15 '24 00:05 mscdex

Are you ok with this being pushed into 11.5? Or do you need it in an earlier version? This is basically a bug fix, it could go into 10.5, if you'd rebase it onto 10.5 or if you'd let me rebase it.

vuvova avatar May 25 '24 08:05 vuvova

Are you ok with this being pushed into 11.5? Or do you need it in an earlier version? This is basically a bug fix, it could go into 10.5, if you'd rebase it onto 10.5 or if you'd let me rebase it.

I'm fine with rebasing it on 10.11, which is the current LTS, however will it propagate forward (to the next LTS branch too)? Especially in this case since there is a merge conflict when rebasing on 10.11, so I'm not sure which side I should be pulling from.

mscdex avatar May 25 '24 08:05 mscdex

Yes, it will propagate. If you'd like, I can take your commit and push it into 10.11

vuvova avatar May 25 '24 12:05 vuvova

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 25 '24 17:05 CLAassistant

I've rebased against 10.11.

mscdex avatar May 25 '24 17:05 mscdex