Skip to content

Get SQL databases working again #7653

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

Open
wants to merge 41 commits into
base: dev/feature
Choose a base branch
from

Conversation

TheLimeGlass
Copy link
Contributor

@TheLimeGlass TheLimeGlass commented Feb 27, 2025

Description

  • Adds H2 as a configurable database. Will probably make default eventually or maybe in this pull request?
  • Removes SQLibrary and adds https://github.com/brettwooldridge/HikariCP as the main SQL wrapper.
  • Revamped API for better registration and handling of JDBC Java drivers.
  • Added a commit changes option which is essentially how Skript used to operate. Changed the JDBC standard to auto commit after every edit (Make the 5-min variable saving period configurable #2007). This is standard and better for performance. Skript didn't for some reason. I speculate it was to allow MySQL to sync with other servers, so either way it's an option now.

Perks

  • Better configuration options with HikariCP.
  • Way better performance with HikariCP.
  • Easier implementation and not relying on a small project like SQLibary.
  • Thread pooling potential with HikariCP.
  • This new API design allows for quite literally any JDBC implementation database.
  • Uses the SpigotLibraryLoader to load the resources at runtime rather than shadowing into the jar.

TODO

  • Test MySQL.
  • Test SQLite.
  • Test H2.
  • Run beta testing in the SkriptLang Discord.
  • Adds tests.
  • In H2 use H2's MERGE INTO instead of INSERT.
  • Forgot to add a check that HikariCP classes were loaded by SpigotLibraryLoader.
  • Rename SQLStorage to something with JDBC reference, maybe JdbcStorage as it's not only SQL anymore.
  • Test that monitor changes still do something.
  • Attach a SkriptAddon instance to the registration, so Skript knows where a storage is coming from.
  • Investigate if this solves Critical Bug with List-Variables #2022

Optional

  • Write up an API tutorial for registering custom storage types.
  • Document all the possible options for databases or have annotations for a documentation system.
  • See if Skript could handle async and multithreading now that the database has the possibility.
  • Maybe add MongoDB support More DataBase support ? MongoDB for example #1787
  • Change the existing behavior of dumping every variable into ram on server start.

Notes:

  • Should we just remove SQLite support? It's not like anyone is using it right now, it doesn't work currently.
  • H2 is a flat file SQL implementation that supports asynchronous operations and multi threaded unlike SQLite in both those regards.
  • This does not impact anything to do with the default flat file CSV.
  • This mainly only affects JDBC driver databases. The rework is still a viable experiment started by bensku https://github.com/SkriptLang/Skript/tree/feature/variables2

Testing and using this jar

To test this experimental feature out, go to the "checks" tab and then click a Java version on the side and then click the nightly artifacts to get a built jar of the latest commit.


Target Minecraft Versions: any
Requirements: none
Related Issues: #1168, #2007, #1478 and #3948

@TheLimeGlass

This comment was marked as resolved.

@sovdeeth sovdeeth reopened this Mar 10, 2025
@sovdeeth sovdeeth changed the base branch from enhancement/sql to dev/feature March 10, 2025 21:28
@sovdeeth sovdeeth changed the title Enhancement/sql update to 2.10.1 Get SQL databases working again Mar 10, 2025
@sovdeeth sovdeeth mentioned this pull request Mar 10, 2025
16 tasks
@sovdeeth sovdeeth added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. variables Related to variables and/or storing them. labels Mar 10, 2025
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

See existing change requests from pickle at previous pr, too: #5646 (review)

Comment on lines +167 to +169
### VS Code ###
.vscode/

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be in this pr

TheLimeGlass and others added 3 commits March 12, 2025 10:17
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
@TheLimeGlass TheLimeGlass requested a review from sovdeeth March 12, 2025 16:19
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Changes from #5646 not addressed

@TheJoshue
Copy link

Hello, I just want to report that synchronization works great besides 1 thing
If you remove variable from 1 server (it now displays as <none> on that server where you executed a deletion), all other servers that share this variable/mysql won't be affected and they will keep their last known variable (i assume the variable when monitor changes executes is stored in server's cache/memory), so by that it does not get synchronized properly, it should also become <none> on all servers that monitor same databases

I have been testing skript mysql implementation for more than 4 months now, not constantly, but very actively, i think this is the only issue left

I also would like to ask how optimized is the monitor-changes functionality for 2 reasons (2 questions!):

  1. If we have 2+ or even 5+ mysql databases connected on the same network of sub-servers, what should lowest optimal monitor-changes delay be? What about 0.5? Just curious
  2. What is the way of working for monitor-changes and commiting changes? Like, let's say that 2 different servers connected on same skript database try to set {synced_example::test1} and {synced_example::test2} to some value, will the first server who tries to commit changes "lock" the database and prevent second one from inserting this variable (if the timing for both servers commiting is very similar)? I am also interested to know the same thing for monitor-changes - what if multiple servers try to look up / check for changes on same database, will the first server who tries to check for changes "lock" database and make other servers fail to pickup latest changes from database?

These questions are based on my testings of plugin's this new SQL implementation

@TheJoshue
Copy link

  1. Set some variable on server1 to some value
  2. Show those variables on server2 - the values will show (after defined delay of monitor-changes)
  3. Delete those variables on server2
  4. So because of the way currently the system works (if you delete variable (making it unset) on any server that synchronizes variables, that won't be changed/synchronized to all other servers sharing the same database), the server2 will now have <none> value for those variables, even tho all other synchronized sub-servers will keep last known value and will not have it unset

So now the value of variable is removed from MySQL and all other servers should sync with to that, however, what happens next is that if you restart server2, it loads up, the variables are still <none> but now if you use server1 to set those variables to something else, server2 will never again synchronize variables, even if you restart it, even if after restart you use server2 to change variable to something else, it won't sync anywhere on network

It also will not sync any other databases for some reason that are shared also by the network sub-servers... this gets so weird, i probably should have tested this lol. Any database i had connected now cannot monitor changes (maybe it does, but there is no info about it) even after i restart them, i don't know what happened.

@Lennord
Copy link

Lennord commented Mar 19, 2025

  1. Set some variable on server1 to some value
  2. Show those variables on server2 - the values will show (after defined delay of monitor-changes)
  3. Delete those variables on server2
  4. So because of the way currently the system works (if you delete variable (making it unset) on any server that synchronizes variables, that won't be changed/synchronized to all other servers sharing the same database), the server2 will now have <none> value for those variables, even tho all other synchronized sub-servers will keep last known value and will not have it unset

So now the value of variable is removed from MySQL and all other servers should sync with to that, however, what happens next is that if you restart server2, it loads up, the variables are still <none> but now if you use server1 to set those variables to something else, server2 will never again synchronize variables, even if you restart it, even if after restart you use server2 to change variable to something else, it won't sync anywhere on network

It also will not sync any other databases for some reason that are shared also by the network sub-servers... this gets so weird, i probably should have tested this lol. Any database i had connected now cannot monitor changes (maybe it does, but there is no info about it) even after i restart them, i don't know what happened.

Hello, I've just read your 2 messages (above) and I can tell you that you're absolutely right, I reported this problem just over 2 months ago on the official new SQL test discord.

I have a question, you don't talk about auto-commit feature, did you try to disable it by setting it to disabled or enable it on enabled, do you have an error, because I had tested a few weeks ago and I still had the error.

Normally if you don't use the auto-commit feature, it's normal that it doesn't synchronize normally because auto-commit allows you to modify the state of the skript variables on each server by synchronizing them etc...

But since I started using this feature, I still can't get auto-commit to work because there are one or more errors in the console. Anyway, I just wanted to let you know if you had the same problem as me. Have a nice day!

I want to give you an example of an error I had with the feature I'm talking about: https://pastebin.com/7J5Frbzf

@Pikachu920
Copy link
Member

  1. Set some variable on server1 to some value
  2. Show those variables on server2 - the values will show (after defined delay of monitor-changes)
  3. Delete those variables on server2
  4. So because of the way currently the system works (if you delete variable (making it unset) on any server that synchronizes variables, that won't be changed/synchronized to all other servers sharing the same database), the server2 will now have <none> value for those variables, even tho all other synchronized sub-servers will keep last known value and will not have it unset

So now the value of variable is removed from MySQL and all other servers should sync with to that, however, what happens next is that if you restart server2, it loads up, the variables are still <none> but now if you use server1 to set those variables to something else, server2 will never again synchronize variables, even if you restart it, even if after restart you use server2 to change variable to something else, it won't sync anywhere on network

It also will not sync any other databases for some reason that are shared also by the network sub-servers... this gets so weird, i probably should have tested this lol. Any database i had connected now cannot monitor changes (maybe it does, but there is no info about it) even after i restart them, i don't know what happened.

Even if this PR is merged, using the same variables database across multiple severs wouldn't be supported.

@TheJoshue
Copy link

TheJoshue commented Mar 19, 2025

I have a question, you don't talk about auto-commit feature, did you try to disable it by setting it to disabled or enable it on enabled, do you have an error, because I had tested a few weeks ago and I still had the error.

Normally if you don't use the auto-commit feature, it's normal that it doesn't synchronize normally because auto-commit allows you to modify the state of the skript variables on each server by synchronizing them etc...

But since I started using this feature, I still can't get auto-commit to work because there are one or more errors in the console. Anyway, I just wanted to let you know if you had the same problem as me. Have a nice day!

I want to give you an example of an error I had with the feature I'm talking about: https://pastebin.com/7J5Frbzf

I did not touch auto commit option, and i think you mean commit changes and not auto-commit? I do not have this option, maybe my Skript build is outdated, later today or this week i will update to very latest and check if this option exist there, i only have this

	#commit changes: 0.5 seconds
	# If you want to change how frequently SQL changes are commited.
	# If this is disabled, auto commit will be enabled.
	# Pros to this would be on external databases such as MySQL where it gives other servers time to react to changes.

this is commented out for mysql example, so i assume it uses that auto commit feature, because it says "if this is disabled..." and commit changes is disabled = it's commented

Even if this PR is merged, using the same variables database across multiple severs wouldn't be supported.

Ok wow, well i did not know that, i thought that was the purpose of having MySQL implementation in Skript
Is it even planned to support that? i mean i see to some extent it works but i am only testing this, was having hopes to use that both for local (subserver/backend only) & global (network/cross-server supported) variables

@Lennord
Copy link

Lennord commented Mar 19, 2025

I have a question, you don't talk about auto-commit feature, did you try to disable it by setting it to disabled or enable it on enabled, do you have an error, because I had tested a few weeks ago and I still had the error.
Normally if you don't use the auto-commit feature, it's normal that it doesn't synchronize normally because auto-commit allows you to modify the state of the skript variables on each server by synchronizing them etc...
But since I started using this feature, I still can't get auto-commit to work because there are one or more errors in the console. Anyway, I just wanted to let you know if you had the same problem as me. Have a nice day!
I want to give you an example of an error I had with the feature I'm talking about: https://pastebin.com/7J5Frbzf

I did not touch auto commit option, and i think you mean commit changes and not auto-commit? I do not have this option, maybe my Skript build is outdated, later today or this week i will update to very latest and check if this option exist there, i only have this

	#commit changes: 0.5 seconds
	# If you want to change how frequently SQL changes are commited.
	# If this is disabled, auto commit will be enabled.
	# Pros to this would be on external databases such as MySQL where it gives other servers time to react to changes.

this is commented out for mysql example, so i assume it uses that auto commit feature, because it says "if this is disabled..." and commit changes is disabled = it's commented

Yes, I was talking about the commit changes feature, sorry for getting the name of the feature wrong. Saw the message above (from Pikachu920). I hope that one day we'll be able to synchronize variables on several servers, which unfortunately won't be the case for now :(.

@TheJoshue
Copy link

Yes, I was talking about the commit changes feature, sorry for getting the name of the feature wrong. Saw the message above (from Pikachu920). I hope that one day we'll be able to synchronize variables on several servers, which unfortunately won't be the case for now :(.

It would be really incredible to have that, i mean it is kinda working at the moment, for now you need some kind of a bridge plugin which you can utilize inside of skript itself, so basically only selection of variables can be used cross-server
Still, can't wait if this ever happens

@TheLimeGlass TheLimeGlass requested review from a team as code owners April 15, 2025 18:50
@TheLimeGlass TheLimeGlass requested review from UnderscoreTud and sovdeeth and removed request for a team April 15, 2025 18:50
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Changes from @APickledWalrus in #5646 still not addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. variables Related to variables and/or storing them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants