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

Improve performance of advanced map list #995

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

civ0
Copy link

@civ0 civ0 commented Oct 31, 2020

Motivation or cause

See #994

Change description

The advanced map list used to retrieve the records from the database by
performing two database queries per map in a loop over all maps. This
commit changes this to one big query to the database that gets performed
outside of the loop.

Checklist of pull request

Make sure that your pull request follow the following 'rules':

  • My code follows the code style of this project.
    • I am not sure about this one, ORM code indentation is tricky with tabs
  • All new and existing tests passed, except in few situations.
    • I did not run the tests because I could not find documentation on how to run them. Running the unit tests from the Makefile resulted in an error.

Additional Comments (optional)

This PR contains the first version of the implementation and I am still not completely happy with it, but also want to get feedback on the general approach.

The advanced map list used to retrieve the records from the database by
performing two database queries per map in a loop over all maps. This
commit changes this to one big query to the database that gets performed
outside of the loop.
@tomvlk tomvlk self-assigned this Nov 27, 2020
@tomvlk tomvlk self-requested a review November 27, 2020 10:52
@reaby
Copy link
Contributor

reaby commented Dec 18, 2020

This method has only one minor issue: it requires so new mysql/mariadb that many don't just have it.
(for the partition-query)

@civ0
Copy link
Author

civ0 commented Dec 18, 2020

@reaby

  • It seems that MySQL 8.0 or higher is required for this. MySQL 5.7 unfortunately still has support until 2023, so this might stay around for a while.
  • MariaDB introduced the RANK() function with version 10.2, which is the oldest version that is not end of life.

For me it seems that only MySQL is blocking this and I haven't tested PostgreSQL yet, but it should work. The docs show examples comparable to my query: https://www.postgresql.org/docs/current/tutorial-window.html
I specifically used the RANK() function instead of ROW_NUMBER(), because that seems to be MySQL only.

Maybe we can detect the DB version and only use this code if the DB is recent enough?

@reaby
Copy link
Contributor

reaby commented Dec 18, 2020

Yeah, let me see my mariadb version for reference since only when updating the very newest one started to work.
10.1.38 doesn't work, i believe it's since the partition query...
10.4.17 started to work...

And even i have such old mysql, how many else has :)

@tomvlk
Copy link
Member

tomvlk commented Feb 5, 2021

This isn't supported by most versions yet indeed:

Traceback (most recent call last):
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/core/events/dispatcher.py", line 184, in execute_receiver
    return receiver, await receiver(**kwargs)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/core/ui/components/manialink.py", line 169, in handle
    return await self.handle_catch_all(player, action_name, values)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/views/generics/list.py", line 200, in handle_catch_all
    await action(player, values, view=self)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/apps/contrib/jukebox/views.py", line 286, in action_advanced
    await self.refresh(player=self.player)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/views/generics/list.py", line 230, in refresh
    await self.display(player=player)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/apps/contrib/jukebox/views.py", line 235, in display
    return await super().display(player or self.player)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/views/generics/list.py", line 259, in display
    return await super().display(player_logins=[login])
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/views/template.py", line 160, in display
    return await super().display(player_logins, **kwargs)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/core/ui/components/manialink.py", line 121, in display
    return await self.manager.send(self, player_logins, **kwargs)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/core/ui/__init__.py", line 107, in send
    body = await manialink.render()
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/views/template.py", line 108, in render
    kwargs['data'] = await self.get_context_data()
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/views/generics/list.py", line 313, in get_context_data
    context.update(await self.get_object_data())
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/views/generics/list.py", line 438, in get_object_data
    frame = pd.DataFrame(await self.get_data())
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/apps/contrib/jukebox/views.py", line 102, in get_data
    records = await self.app.instance.apps.apps['local_records'].get_all_maps_first_and_player_records(
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/apps/contrib/local_records/__init__.py", line 139, in get_all_maps_first_and_player_records
    records = await Map.execute(Map.select(Map,
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/pyplanet/core/db/model.py", line 57, in execute
    return await cls.objects.execute(query)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/peewee_async.py", line 269, in execute
    return (yield from execute(query))
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/peewee_async.py", line 433, in execute
    return (yield from coroutine(query))
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/peewee_async.py", line 574, in select
    cursor = yield from _execute_query_async(query)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/peewee_async.py", line 1558, in _execute_query_async
    return (yield from _run_sql(query.database, *query.sql()))
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/peewee_async.py", line 1545, in _run_sql
    return cursor
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/peewee.py", line 3656, in __exit__
    reraise(new_type, new_type(*exc_args), traceback)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/peewee.py", line 135, in reraise
    raise value.with_traceback(tb)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/peewee_async.py", line 1540, in _run_sql
    yield from cursor.execute(operation, *args, **kwargs)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/aiomysql/cursors.py", line 239, in execute
    await self._query(query)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/aiomysql/cursors.py", line 457, in _query
    await conn.query(q)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/aiomysql/connection.py", line 428, in query
    await self._read_query_result(unbuffered=unbuffered)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/aiomysql/connection.py", line 622, in _read_query_result
    await result.read()
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/aiomysql/connection.py", line 1105, in read
    first_packet = await self.connection._read_packet()
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/aiomysql/connection.py", line 593, in _read_packet
    packet.check_error()
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/pymysql/protocol.py", line 220, in check_error
    err.raise_mysql_exception(self._data)
  File "/home/tom/IdeaProjects/maniaplanet/pyplanet-core/env/lib/python3.8/site-packages/pymysql/err.py", line 109, in raise_mysql_exception
    raise errorclass(errno, errval)
peewee.ProgrammingError: (1064, "You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '(PARTITION BY `t5`.`map_id` ORDER BY `t5`.`score` ASC) AS local_rank, `t5`.`id`,' at line 1")

You could do the 'old style' for older MySQL servers, you can most likely obtain the version of the MySQL server with the client.
Can you improve this pull request so we can merge it once fixed for older versions?

Copy link
Member

@tomvlk tomvlk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't support older MySQL versions.

@tomvlk tomvlk added the Blocked label Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants