no global var

[6543]

last missing nit from #17919

[lunny]

What's wrong with the global variable?

[6543]

It's only used inside the function - it should never be global!!!

[wxiaoguang]

You see, it's not a simple global variable. It's a cache. It's usually nil and doesn't consume memory.

It helps the http.HandlerFunc in install.go to save time (save CPU) from making/copying the slice again and again.

I do not see the necessary to change it. If you want to change it, the logic inside getDbTypeNames should also be changed (remove the if nil check) ............

[6543]

I dont think we need to cache this as install page will only show up on installation

[wxiaoguang]

Then you should also remove the if supportedDbTypeNames == nil {

[6543]

in general we should avoid global vars if possible

they produce races, consume resources not needed after exec of code parts mostly ...

[wxiaoguang]

in general we should avoid global vars if possible

they produce races, consume resources not needed after exec of code parts mostly ...

As comments before, it's just a install page, no race there. And it only consumes 8 bytes (a pointer) memory. Just one request consume much much more than that .... Personally I won't put too many loops in the common Context construction. Indeed there is no problem to remove the cache if you like to.

[wxiaoguang]

After grep-ing ^var in *.go, I found some more global variables, like this (ps: there is a bug that the ctx is not used ....)

Maybe it's worth to do some clean up in the project.

[silverwind]

There still may be a performance benefit of keeping these top-level variables in case their instantiation is expensive. For example, I like to put regexes in top level because pre-compiling them will always be better performance than to compile them on every function call.

[wxiaoguang]

regex vars can be kept, while supportedDbTypeNames and delRepoArchiver etc can be removed safely.

[wxiaoguang]

For the supportedDbTypeNames, I made another fix, move it from global to local and still cache the slice. And there is another fix for the installation page, need 👀

• Skip frontend ROOT_URL check on installation page, remove unnecessary global var #19291