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

[bug][regression] mysql2.setMaxParserCache fails with TypeError #2752

Closed
constb opened this issue Jun 10, 2024 · 4 comments · Fixed by #2757 or #2988
Closed

[bug][regression] mysql2.setMaxParserCache fails with TypeError #2752

constb opened this issue Jun 10, 2024 · 4 comments · Fixed by #2757 or #2988
Labels

Comments

@constb
Copy link
Contributor

constb commented Jun 10, 2024

Hi!

If I call setMaxParserCache imported from mysql2, I get an error:

<snip>/node_modules/mysql2/lib/parsers/parser_cache.js:54
  parserCache.max = max;
                  ^
TypeError: Cannot set property max of #<LRUCache> which has only a getter

I really need to downsize this cache because it always ends up being full and consuming about 300Mb RAM. Probably something about my use case that makes it grow so big…

I believe somewhere along the way lru-cache changed its internals and cannot be resized anymore. I'd like if setMaxParserCache would throw away an old cache and create a new one with right size in its place…

@sidorares
Copy link
Owner

Can you try to find a way to change the size in lru-cache? You a probably right, looks like this was an option in earlier versions of lru-cache but no longer is

Alternatively we can refactor parserCache to be part of a single connection and also accept { max } parameter from connection options. Right now it's a global variable

@constb
Copy link
Contributor Author

constb commented Jun 10, 2024

Can you try to find a way to change the size in lru-cache?

According to its readme, they initialize internal structures based on max size and it's too complicated to adjust them to the new size, so it cannot be changed now…

Alternatively we can refactor parserCache to be part of a single connection

There already is per-connection cache for prepared statements. I doubt there needs to be another one. Also it would reduce cache hit-rate in cases where same queries can be used across multiple connections or even connection pools.

Actually global parser cache makes total sense. As an alternative an option to bypass the cache on specific queries could be implemented, but that maybe a bit too complicated too…

Setting size by recreating cache seems like a simple solution. Instead of parserCache.max = newMax just do parserCache = new LRU({ max: newMax })

constb added a commit to constb/node-mysql2 that referenced this issue Jun 11, 2024
@constb
Copy link
Contributor Author

constb commented Jun 11, 2024

made a PR to speed up resolution of this one :)

@wellwelwel
Copy link
Sponsor Collaborator

wellwelwel commented Aug 27, 2024

I believe it's worth keeping this issue open (or open a new one if it's more convenient).

For context:

let parserCache = new LRU({
max: 15000,
});

function setMaxCache(max) {
parserCache = new LRU({ max });
}


Although it's a very specific case, an example of when we lost 15,000 caches without benefit after v3.10.1:

for (let i = 0; i < 15_000; i++) {
  await conn.execute(`SELECT ${i}+${i} as total${i}`);
}

// Allowing 50,000 caches
mysql.setMaxParserCache(50_000);
  • The expected behavior is to keep all previous cache (15,000), allowing more 135,000.
  • Currently the 15,000 caches are lost and a new one is started.

The same problem reducing cache size:

for (let i = 0; i < 15_000; i++) {
  await conn.execute(`SELECT ${i}+${i} as total${i}`);
}

// Allowing 10,000 caches
mysql.setMaxParserCache(10_000);
  • The expected behavior is to keep 10,000 previous cache (from most recently used), removing the oldest 5,000 if exist.

When setMaxParserCache is used before a cache is generated, there is no loss.

Also, it's not our case, but if we use dispose in the current format, callbacks would never be called.


Note

I opened a PR (#2988) proposing a library change, where we could basically do this internally:

parserCache.resize(max);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants