Upgrade HSQLDB to 2.x#876
Conversation
Signed-off-by: Iwao AVE! <harawata@gmail.com>
Signed-off-by: Iwao AVE! <harawata@gmail.com>
…default. To change the behavior, append 'sql.enforce_size=false' to the default JDBC URL. http://hsqldb.org/doc/guide/dbproperties-chapt.html#N1580D Signed-off-by: Iwao AVE! <harawata@gmail.com>
…configurable via 'sql.nulls_first' property. http://hsqldb.org/doc/guide/dbproperties-chapt.html#N159E5 Signed-off-by: Iwao AVE! <harawata@gmail.com>
…dded with spaces before comparison'. This behavior is configurable via 'sql.pad_space' property. http://hsqldb.org/doc/guide/dbproperties-chapt.html#N15A42 Signed-off-by: Iwao AVE! <harawata@gmail.com>
Signed-off-by: Iwao AVE! <harawata@gmail.com>
|
The CI failure seems to be not directly related. I'll look into it within a day or two. |
|
I'm not so sure, but the CI error may be caused by some newly reported CVE vulnerabilities. |
|
After more testing, the scanning slowdown is caused by the same reasons fixed by #864 for external databases. I can get it to work as fast as before by applying the patch below on your branch: diff --git a/airsonic-main/src/main/resources/applicationContext-db-legacy.xml b/airsonic-main/src/main/resources/applicationContext-db-legacy.xml
index f866f38..6a6bbdf 100644
--- a/airsonic-main/src/main/resources/applicationContext-db-legacy.xml
+++ b/airsonic-main/src/main/resources/applicationContext-db-legacy.xml
@@ -5,7 +5,7 @@
profile="legacy">
<bean id="dataSource"
- class="org.springframework.jdbc.datasource.DriverManagerDataSource">
+ class="org.apache.commons.dbcp2.BasicDataSource">
<property name="driverClassName" value="org.hsqldb.jdbcDriver" />
<property name="url"
diff --git a/airsonic-main/src/test/java/org/airsonic/player/api/jukebox/AbstractAirsonicRestApiJukeboxIntTest.java b/airsonic-main/src/test/java/org/airsonic/player/api/jukebox/AbstractAirsonicRestApiJukeboxIntTest.java
index 89db89d..3768e5e 100644
--- a/airsonic-main/src/test/java/org/airsonic/player/api/jukebox/AbstractAirsonicRestApiJukeboxIntTest.java
+++ b/airsonic-main/src/test/java/org/airsonic/player/api/jukebox/AbstractAirsonicRestApiJukeboxIntTest.java
@@ -95,13 +95,6 @@ public abstract class AbstractAirsonicRestApiJukeboxIntTest {
dataBasePopulated = false;
}
- @AfterClass
- public static void cleanDataBase() {
- staticDaoHelper.getJdbcTemplate().execute("DROP SCHEMA PUBLIC CASCADE");
- staticDaoHelper = null;
- dataBasePopulated = false;
- }
-
/**
* Populate test datas in the database only once.
*Note that I had to disable DB cleanup to make the tests pass, because I get the following error if I don't. Maybe @biconou has an idea on how to get it work again? |
|
And, yes, the CI failures are newly reported CVE vulnerabilities. I generally update the library version when I find those, but it's annoying when it happens... |
|
Thank you, @fxthomas ! Replacing DriverManagerDataSource makes sense. Good catch! And the following patch resolved the CVE errors. diff --git a/airsonic-main/cve-suppressed.xml b/airsonic-main/cve-suppressed.xml
index 9a5c02a..1dd9b33 100644
--- a/airsonic-main/cve-suppressed.xml
+++ b/airsonic-main/cve-suppressed.xml
@@ -143,11 +143,13 @@
<notes><![CDATA[False Positive for stax]]></notes>
<gav regex="true">^stax.*$</gav>
<cve>CVE-2017-16224</cve>
+ <cve>CVE-2018-1000840</cve>
</suppress>
<suppress>
<notes><![CDATA[False Positive for stax]]></notes>
<gav regex="true">^javax\.xml\.stream:stax.*$</gav>
<cve>CVE-2017-16224</cve>
+ <cve>CVE-2018-1000840</cve>
</suppress>
<suppress>
<notes><![CDATA[We do not use slf4j ext]]></notes>
diff --git a/pom.xml b/pom.xml
index 4f2f1a2..b0b2999 100644
--- a/pom.xml
+++ b/pom.xml
@@ -116,12 +116,12 @@
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-core</artifactId>
- <version>2.9.6</version>
+ <version>2.9.8</version>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
- <version>2.9.6</version>
+ <version>2.9.8</version>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>Please let me know if I should include the above change(s) to this PR. |
|
As an update, without the database cleanup, if you run multiple test cases in a sequence, one of the test classes pass, but not the other (the database is populated twice). Simply dropping the in-memory data source and recreating it is apparently impossible, and I couldn't figure out how to make transactions run (adding Aside from this, I found that you can use this in your tests to only run a migration for HSQLDB 1.x: <preConditions onFail="MARK_RAN">
<dbms type="hsqldb" />
<customPrecondition className="org.airsonic.player.spring.DbmsVersionPrecondition" >
<param name="major" value="1" />
</customPrecondition>
</preConditions> |
This seemed to happen when running the tests in IDE (Eclipse in my case). After reading the jukebox test classes carefully, I think I found the cause. diff --git a/airsonic-main/src/main/resources/applicationContext-db-legacy.xml b/airsonic-main/src/main/resources/applicationContext-db-legacy.xml
index f866f38..6a6bbdf 100644
--- a/airsonic-main/src/main/resources/applicationContext-db-legacy.xml
+++ b/airsonic-main/src/main/resources/applicationContext-db-legacy.xml
@@ -5,7 +5,7 @@
profile="legacy">
<bean id="dataSource"
- class="org.springframework.jdbc.datasource.DriverManagerDataSource">
+ class="org.apache.commons.dbcp2.BasicDataSource">
<property name="driverClassName" value="org.hsqldb.jdbcDriver" />
<property name="url"
diff --git a/airsonic-main/src/test/java/org/airsonic/player/api/jukebox/AbstractAirsonicRestApiJukeboxIntTest.java b/airsonic-main/src/test/java/org/airsonic/player/api/jukebox/AbstractAirsonicRestApiJukeboxIntTest.java
index 89db89d..850f873 100644
--- a/airsonic-main/src/test/java/org/airsonic/player/api/jukebox/AbstractAirsonicRestApiJukeboxIntTest.java
+++ b/airsonic-main/src/test/java/org/airsonic/player/api/jukebox/AbstractAirsonicRestApiJukeboxIntTest.java
@@ -112,8 +112,8 @@ public abstract class AbstractAirsonicRestApiJukeboxIntTest {
* </ul>
*/
private void populateDatabase() {
+ staticDaoHelper = daoHelper;
if (!dataBasePopulated) {
- staticDaoHelper = daoHelper;
assertThat(musicFolderDao.getAllMusicFolders().size()).isEqualTo(1);
MusicFolderTestData.getTestMusicFolders().forEach(musicFolderDao::createMusicFolder); |
|
Sorry, it didn't work. I need to get some sleep, it seems. |
|
Okay, seems that moving database preparation/cleanup to before/after each test instead of the class itself works well. This commit (based on your branch) appears to do the trick. |
|
Awesome! Thanks a lot for your work, @fxthomas ! |
|
Thank you @muff1nman and @fxthomas for your work! |
|
I managed to trigger an other race-condition-looking exception, that might be fixed by this PR: |
|
@jvoisin , |
|
Why are we deleting the record instead of updating it by the way? |
|
Pushing this to 11.0.0 means that it will ikley be out of sync by the time, and would have to be redone :/ |
|
According to Travis, there seems to be no issue so far. :) I myself am totally fine with using a local build, but may I ask why this is targeted to 11.0.0 rather than an earlier milestone? |
|
@harawata Did you leave this running for a while without noticing side-effects? Since this is a big change with potential side-effects, I'd rather be really, really sure that we don't mess this up, especially since there is no rolling back from the migration (once you're on 2.x you're stuck there). I'll pull that on my server and leave it open for a while myself, will report if I find anything! |
|
Thank you for the reply, @fxthomas ! I have been using a local build from this branch for months and haven't noticed any issue, but it may be a good idea to test it on other users' environments. |
|
Eh... I'm getting a lot of issues with upgrading an existing database, that only appear once the server is restarted once more after the migration. I also tested HSQLDB 2.5.0, which exhibits the same issues. I did not test creating a server from scratch (which I suspect works). The main issue (that appears when trying to update a Rolling back for now until we know more. |
|
Hi @fxthomas , I just did the following test and it worked without an error.
Could you search |
|
While this might be a nice way to test it, keep in mind that it isn't a good on for users to upgrade ;) |
|
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
|
Okay, now that I'm back from holidays, time to revisit this one! 😄 I want to eliminate the hypothesis that my current production database is just slightly corrupted and borks everything on upgrade while working well for usual operations. Therefore, I'm going to try the following plan:
I'll leave that running for a while ; I made a filesystem snapshot right before the upgrade so I'll be able to tell if there is any issue. Will report in a few days. |
|
Welp, datetime corruption strikes again. My database being corrupted before the start of the upgrade is therefore not very likely (the SQL script had what looked were correct data between step 1 and 2, and I don't have any issues for daily usage with HSQLDB 1.8). The issue is either a) somewhere inside the conversion process between 1.8 and 2.5.0 or b) somewhere in the Airsonic code. I have no idea where to look for next, but I found a few things strange:
|
|
Welcome back, @fxthomas ! The '1000x ratio' is a good clue.
FYI, I noticed that, in my 'airsonic.script' file, the timestamp format was changed during upgrade. p.s. |
|
Thank you for the help ; I tried it and got the same issue... but I found more! 😉 TL;DR: Upgrading to 2.x should be okay, provided we do the following:
More detailed findings (see my upgrade-hsqldb-to-2.x branch, especially the last two WIP commits):
Additionally, the old DAO schema scripts used HSQLDB I believed it to be the trigger for the bug, so I added some debugging code to confirm that ; with it enabled, I have the issue immediately after doing the following steps:
Log excerpt right after restarting the HSQDLB 2.5 server a second time: |
|
So far I don't see any issues anymore with HSQLDB 2.5.0. I'm going to leave it enabled on my server for a while and see how it fares over time, but I have good hopes. @muff1nman @jvoisin @harawata After thinking about it, here is the new plan of action I propose:
That will, I think, prevent most of the issues, and changing the behavior of either migrations or DAO objects should not be necessary. How does that sound? |
|
Thank you so much for your work, @fxthomas ! I couldn't reproduce the issue myself, but your explanation looks very convincing. Migrating table type from CACHED to MEMORY and converting |
|
@fxthomas that plan sounds decent to me though it might be a bit tricky to ensure it gets done at the right time on startup. However, taking a step back - are we sure we want to go from hsqldb 1.8 to hdqldb 2.x? Have we considered going to another lightweight database? Maybe sqlite? The reason I ask is that we should think about this decision lasting for the next 5 years. Granted going to another database might be trickier - but still it might be worthwhile to consider our options. |
|
@muff1nman I'm not that much familiar with the Java ecosystem, so I'm not confident I can see every pros and cons of switching the default embedded db entirely. I have amassed a pretty good view of HSQLDB over the years though, so let's say I'm more familiar with it. Otherwise, here are the results of a quick search:
Also, I quite like the fact that Airsonic's embedded database stores things in a plain text SQL file by default. It's not any less reliable and there is not a big speed difference for small setups, and it makes it much, much easier for users to understand what's going on. Especially when they have limited knowledge of external databases. If possible, I would like to push for keeping it in some form. |
|
Closing in favor of #1345, where I added a controlled/automatic upgrade procedure. |
Originally posted by @fxthomas in #876 (comment) Sqlite has other advantages. For example, it's quite good at preventing data loss or corruption. I had an issue earlier when I had a power outage while airsonic was in the middle of writing the file; I had to remove the partially-written line (an incomplete transaction) in order to partially repair my database (I lost play counts on about half of it; fortunately the list of folders, song ratings, user accounts, podcasts, etc are all stored near the top, and I use auto-imported playlists generated by other software). If you were using an sqlite database (or likely any other binary database that doesn't rewrite the file from scratch), this wouldn't have happened. To deal with this in the future, I'm setting up a scheduled database backup, but this won't help figure out why nothing responds if I'm remote. If considering databases in the future, please keep this in mind--ability to edit the database isn't everything. |
The primary goal is to fix #872 and its side effects like #531 .