Add player key index in database to speedup player lookups

[lflare]

Not entirely sure if this conforms to standards and guidelines, but as I was doing some lookups the other day, I realised that all lookups involving players with global radius and insane times resulted in searches taking forever. This might help to alleviate that.

[Narimm]

This only adds the feature to new databases - we may need to consider a database structure update

[lflare]

I believe the functionality of database structure upgrades can be further explored down the line. Adding this PR will effectively only bring a performance boost without any unintended side-effects for those that have already created the database.

[addstar]

@lflare Great suggestion, thanks! I thought this was already implemented, I'm surprised it's not.

Any chance you could demonstrate the improvements with a comparative set of EXPLAIN queries and/or provide some timings? It certainly seems like a good thing to have, but I'm always hesitant about DB changes without data to justify the improvement.

[addstar]

Also, @Narimm / @lflare
There's already a built-in schema updater (we're currently at schema version 7) and it's fairly simple to add new schema versions which will update existing schemas. Any schema change should have a corresponding schema update added, otherwise down the line users may face inconsistency issues with varying installs. I think it's just asking for trouble.

Add schema update here: https://github.com/AddstarMC/Prism-Bukkit/blob/master/src/main/java/me/botsko/prism/database/sql/SqlPrismDataSourceUpdater.java#L78

Bump version to 8 and call the update here: https://github.com/AddstarMC/Prism-Bukkit/blob/master/src/main/java/me/botsko/prism/Updater.java#L29

[Narimm]

I am aware of the schema updater... it will need to be added before this is accepted

[lflare]

@lflare Great suggestion, thanks! I thought this was already implemented, I'm surprised it's not.

Any chance you could demonstrate the improvements with a comparative set of EXPLAIN queries and/or provide some timings? It certainly seems like a good thing to have, but I'm always hesitant about DB changes without data to justify the improvement.

I am not currently able to provide such timings comparisons, but the comparisons can be simply replicated with /pr l p:[PLAYER] t:1y r:global, adding the index manually with ALTER TABLE data ADD KEY `player` (`player_id`); and re-running the query above with another player. This can then later be removed with ALTER TABLE DROP KEY `player`;.

Either way, having an index on player_id will do far more good to lookup performance than harm. The only issue I can see from this is the database server taking up just a tad more storage for the indexing.

[alchemistmatt]

I approve of the addition of this index; it will definitely help query performance. Good to see you will also auto-create it. My only suggestion would be to name the index ix_player_id instead of just player, but the name doesn't matter much, and the other index is already just location

[Narimm]

To support the addition of database changes in SNAPSHOT releases . I would prefer that we dont version the db with every commit . I am not quite sure yet how to handle snapshot db updates - because the problem is anyone who installs a snapshot build wont get any updates in the final release.

My tendency is to enforce running the latest update in any snapshot build - so we need any table alter statements to handles being run multiple times on a potentitall already updated DB--- OR check the db first and skip.

[addstar]

To support the addition of database changes in SNAPSHOT releases . I would prefer that we dont version the db with every commit . I am not quite sure yet how to handle snapshot db updates - because the problem is anyone who installs a snapshot build wont get any updates in the final release.

This should work the same way you'd do it in any production deployment with schema changes. You don't version the DB with each commit, you version the change to the actual schema itself. The code commit is irrelevant, what's important is the merge to master. If you merge a schema change to master, you should always consider it final. Any further adjustment to the schema (even if you change your mind on how the previous schema change was done) should create a new version bump.

So an individual PR (not commit) should contain a single schema version bump and once it's merged, that version and schema change is committed and can never be changed.

My tendency is to enforce running the latest update in any snapshot build - so we need any table alter statements to handles being run multiple times on a potentitall already updated DB--- OR check the db first and skip.

I don't really understand what you mean here, but IMO, we should just always be enforcing all merged schema changes in all environments. We should not require anyone who uses a snapshot to do anything special, there's no need for it. When the schema update is merged and they run the snapshot with it, it will update their schema and the schema version in the DB so it will never attempt the update again.

[lflare]

I approve of the addition of this index; it will definitely help query performance. Good to see you will also auto-create it. My only suggestion would be to name the index ix_player_id instead of just player, but the name doesn't matter much, and the other index is already just location

That's my intention, to follow the convention of whatever has been done prior.

To support the addition of database changes in SNAPSHOT releases . I would prefer that we dont version the db with every commit . I am not quite sure yet how to handle snapshot db updates - because the problem is anyone who installs a snapshot build wont get any updates in the final release.

My tendency is to enforce running the latest update in any snapshot build - so we need any table alter statements to handles being run multiple times on a potentitall already updated DB--- OR check the db first and skip.

I reckon it will be quite simple to handle these kind of things, by having a value in the database that specifies the current database schema version, and from that, retroactively apply all the updates that need to be applied to match the database schema of the current.

With that in mind, I believe it is best practice to version the schema as long as it makes it into a public build, like a snapshot. If changes are made to the schema, that should be added to the history, irregardless of however minor it is. It might not be a bad idea to take a look at how Laravel handles database schema versioning too.

With all of that said and done, I do not believe that there is anything lacking in this PR?

[Narimm]

Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

See the complete overview on Codacy

[Narimm]

Issue found: The instance method name 'v7_to_v8' doesn't match '[a-z][a-zA-Z0-9]*'

[lflare]

Please confirm if you want this resolved.