New Database Layer #617
New Database Layer #617
Conversation
|
PS: See 94cfd7d for a motivation for storing default data in a non-binary format. :) |
…ds on other systems
…things, fixes Mac and Windows builds for this branch
|
Are the boost backtrace libraries really required? cmake is really not happy with them, on either ubuntu 20.04.2 or gentoo. I am not even certain I can get them installed on gentoo, without extraordinary effort. Unless these libraries provide value greater than that effort, I would prefer not to invoke them. |
|
Hmm, that's odd. Ubuntu 20.04.2 is what I run and I don't remember having to do anything special to get it to work. (The Mac build was a bit of another story.) But it's small work to disable it if you want. The backtrace is just there for diagnostics. I'll do a patch. |
|
Thanks. I could get it working in an Ubuntu VM, but not on my gentoo host systems. It is a little embarrassing, actually. I am not used to being able to get something working on Ubuntu but not gentoo. I am hesitant to approve this, to be honest. I like the approach. Database.cpp is blessedly short and does what I have wanted it to do for a long time. There's a lot that needs to be fixed, and it is exactly what I expect when we go playing with the database layers. I am also concerned some of what I am seeing is not repeatable. I had one problem that just mysteriously went away, and I have no idea why. A simple complexity issue. There four entries when trying to select the database type on the options screen. I see two entries for SQLite and two for Postgresql. That is likely something isolated to the options screen and should be a simple fix. A medium complexity issue. Change the style on any recipe and then go look at the Style tree. If you are seeing what I see, there are now two of that style listed. If you restart brewtarget, the duplicate goes away. This is likely to be an order of operations problem -- the tree is signaled to update before the "visible" flag is set to false. I am not fond of chasing these problems. Two large complexity issues. postgresql does not seem to be working. Automatic recipe snapshots are dumping core. So do we merge into develop, knowing we are breaking things? We can at least open proper tickets, and people can help resolve them. Are there other options? |
|
Thanks @mikfire. Let me see if I can fix the Options screen thing. Should be simple, as you say. I'll then see if I can work out the signals order thing on styles. For the snapshot core dump, do you have a stack trace? (I wanted to talk about automatic snapshots, as after I'd jumped through a few hoops to get it to detect any change to any element of a recipe, I started wondering whether it was really helpful to have a new version of the recipe every time every single field was changed. I wondered if it would be more helpful to say "After a Recipe has been brewed, the next change creates a new version. No further new versions (of the recipe) are created automatically until after its next brew day." What do you think?) Similarly, for PostgreSQL, are you seeing any useful diagnostics? |
|
It isn't just on styles. So far, my testing indicates it effects everything. However, odds are good that fixing for one will fix it for all. I think it has to be a signal issue, since restarting brewtarget removes the duplicate entry. The tree is either being signaled before display is set false, or a filter isn't being used that should be. I am testing on a different VM now, and things are inconsistent. The automatic snapshot created 30 snapshots in response to me adding one ingredient. My guess for that would be a snapshot was taken for every thing that got copied from the parent to the child. Previously, it just dumped core. They could be the same issue, and I just got lucky I didn't run off the stack this time. And postgresql is not working. The upgrade fails with a I am thinking that may be the result of how a QString is being built. I can keep poking at that, as I am the only person who cares about using postgresql. |
|
If I manually do the upgrade from version 9 -> 10 (add two columns, set some default values and then update the db version to 10), the postgresql access works. There is definitely something off in the upgrade. I might have some older databases laying about. Let me see if this is just a 9 -> 10 problem, or if it is a general upgrade problem. To answer your other question. Yes, that was the way I had designed the automatic snapshots. Once a recipe was brewed, it was "soft locked" and the first change would spawn a child. Any subsequent change would not spawn a child until it was brewed again. There may have been a difference between what I designed and what I delivered. |
… changed on recipe
|
Ah, OK, good. Couple of small fixes above. The versioning thing is my misunderstanding of the functionality. Should be able to get it closer to what you had. (Pulling this logic out of the Database layer involved quite a lot of change, though it also made me clean up all the setters, so it had good side-effects!) There will doubtless be still one or two bugs, but hopefully then more debuggable. The upgrade stuff will be easy to fix, if that's the only place you're getting errors. I was doing the PostgreSQL-specific stuff without having an instance of PostgreSQL to test on, so I'm hoping it's mostly just typos etc on my part. (Differences in SQL between SQLite and PostgreSQL are mostly OK, just a bit tedious when they need things in a different order.) |
|
With the upgrade error, it's probably staring me in the face, but can you say why PostgreSQL is giving SYNTAX ERROR for The answer from Evan Carroll at https://dba.stackexchange.com/questions/132029/how-to-add-a-column-with-a-foreign-key-constraint-to-a-table-that-already-exists suggested this was an OK way to add a new column with FK constraint to PostgreSQL. |
|
I found something that claims to be a version 7 database, so I am using that. It is my standard mess of a database, but I think I can make it look more normal. The first issue is in DatabaseSchemaHelper when creating any auto-incrementing primary key. In SQLite, it has to be "id INTEGER PRIMARY KEY". In postgresql, it has to be "id SERIAL PRIMARY KEY". Including the "INTEGER" is causing postgresql all sorts of heartburn. My preference would to rename Database::getDbNativeIntPrimaryKeyModifier() to be something like "getDbNativePrimaryKeyDeclaration()" The second issue I am seeing is that postgresql uses "true/false" for boolean values. SQLite uses "1/0". Trying to use "1/0" in postgresql will cause an error. I see 1 and 0 hard coded in several places in DatabaseSchemaHelper. The reason Database::dbBoolean exists is to handle this problem, and I still find Database::dbTrue() and Database::dbFalse() to be useful shortcuts. I don't know if that problem is more wide spread or not. I think the problem you are looking at isn't a syntax error in the SQL. As far as I can determine, postgresql does not like it when you prepare a statement that has no bind values. I am uncertain if it doesn't like the fact nothing was bound (ie, sql.prepare, no sql.bind, sql.exec) or it doesn't like to prepare a statement with no ? in it. Regardless, it seems to think this is a syntax error. The fix I used was to modify DatabaseSchemaHelper::executeSqlQueries() to prepare, bind and exec only if there are values. Otherwise, it just execs the string. That seems to resolve this issue. There are several places in DatabaseSchemaHelper::migrate_to_8 where it says "ALTER TABLE [thingy] ADD COLUMN inventory_id REFERENCES" which needs to have an "INTEGER" between the id and REFERENCES. I am a little concerned in Database::getDbNativeIntPrimaryKeyModifier (and it's friends) take a DbType parameter, but don't use it. I haven't gotten this deep yet, but that has a real potential to cause problems when copying one database to another. You may be handling it different, but it did catch my eye. This has gotten long. I still haven't successfully upgraded from 7 -> 8 yet, but I will stop on this thread. |
|
The migrate_to_8 is painful. I question how I even figured it all out. migrate_to_8 is failing under the new code with this error: I believe the problem here is that I don't track any inventory. I don't actually use the inventory subsystem at all. So the inner query returns empty, and trying to I am also somewhat blocked on more testing until I can figure out how to get around this. Side note. Would it be possible to modify DatabaseSchemaHelper::executeSqlQueries to break out of the for-loop as soon as an error has occured? Once there is an error, the entire commit is rolled back so doing more work after the error isn't really useful. It also has the side effect of filling my screen with error messages. Aborting the loop makes it much easier to find the first problem. |
|
Hmm, good question. I can't find a way to do it in SQL, but I might have an idea about how to handle this... |
|
Going back to your prior points: Thanks for info on the primary key in PostgreSQL. Should be an easy fix. On booleans, AIUI, it should be possible to just use true/false everywhere in SQL and the drivers will figure out to translate that into 0/1 for SQLite. So, if we've got hard-coded 0s and 1s, I just need to change them. On the prepare stuff, I would never have guessed about that limitation from the Qt documentation. Thanks for working it out! On the getDbNativeIntPrimaryKeyModifier() DbType parameter, you're right, it shouldn't be there. My approach to copying from one type of DB to another was to have a separate instance of Database for the from and the to databases (see |
|
Matt, I really hope I am not annoying you or otherwise antagonizing you. I am simply running through my standard tests and trying to doc what happened. If I am getting on your nerves or if you need a break, just let me know and I will hold off on more. I am now trying to copy SQLite->postgresql. It isn't quite as gruesome. We already know most of the fixes. A few small changes and this seems functional. I will, of course, try it the other way. But usually getting it working in one direction almost always works in the other. Before the usual laundry list of issues, why are the database tables being created without default values? I am not saying it is a good or bad thing. I just want to understand the reasoning. Also, why is copyDatabase() in DatabaseSchemaHelper? Again, I am not arguing that it shouldn't be. My first thought is that it goes in Database. The same "INTEGER" problem with the primary keys was found. I think that can be fixed by fixing ObjectStore::createTableWithoutForeignKeys to not use getDatabaseNativeTypeName when firstFieldOutput is false. ObjectStore::createTableWithoutForeignKeys also has the problem of preparing with no binds.
if ( field.type() == QVariant::Bool ) {
bindValue = newDatabase.dbBoolean(bindValue.toBool());
}As a rule, I think in the error case we need to first tell the user something went wrong. We also probably should consider not changing the dbType in the config file, but I think that has always worked that way? |
|
Right, I fixed my crash, then found another one and fixed that. Have also made a change that should at least improve the issue you're seeing above. Ideally, I'd like to go through the code and remove all uses of the SIGNAL and SLOT macros as they take away the compile-time checking you get when you use the address of the signal/slot function. |
|
Mashes are still not working correctly. These errors do not seem to be in HEAD on develop, so I am thinking they are net new. I am also willing to merge as is, and handle this as a standard bug/pr. The mash designer is not updating the "total collected wort" as I increase the infusion amount. If I say "Finish" in the mash designer, the steps are not shown in the mash table. If I just add a step, I see the step added in the mash table. Clicking "Mash Wizard" still says "There must be at least one mash step to run the wizard". If I restart brewtarget, there is nothing either in the table or the database. I am very suspicious of the lack of an objectStoreWrapper::insert in MashStepEditor::saveAndClose. To make it work, I need to put the insert after the addMashStepToMash() which feels really, really weird. If I do it before, my database will suddenly have mashsteps where mash_id IS NULL. I don't see that calling insert is setting cacheOnly to false, which I think it should be. I also see this not happening in the Equipment editor, but it seems to happen almost everywhere else. MashWizard is likely going to take a lot of work to figure out where inserts need to be called. Trying the mash wizard after my fixes above generates an assert failure, saying: Trying that same thing in a slightly less messed up database does the same core dump, but with this message: |
|
I tried to rework the signal/slot mechanisms a while ago. The problem was when we used overloaded slots. At that time, the only way to handle that was with the macros. They've since fixed that problem, or at least published a work around. I agree with fixing it, but that should be a different pull request, though. This one is gnarly enough as it is. |
|
Having a look at this. Looks like, with the Mash Designer, we're creating a Mash OK but then inserting a MashStep in the DB without having set its Mash ID. Similarly, I see what you mean about |
|
Whilst I'm looking at this, do you know what "Save Mash" (on the Mash tab of the current Recipe) is supposed to do? At the moment, it's saving a copy of the current Mash, but I wondered, if that's what it's for, whether it should be prompting the user to name the new Mash? |
|
First, a disclaimer. This is a part of the code I've never really used, understood or particularly liked. I have long felt mashing in brewtarget is the hardest part to understand, and the part most in need of a significant redesign. Which I will do, just as soon as I get the brilliant insight as to what it should look like. I believe the idea was that if you had a mash profile you always used -- like all my beers are usually single infusion, mashed at 66C for 60 minutes and initial temp around 20C -- you could create a named mash and just reuse that instead of having to build a new profile every time. There is no separate window in which to create the mash profile. The save button was how we got around that problem. You could build the mash profile in the main window and then save it. You could also load that named mash, make changes as required and then save it and the next use of that profile would have the changes. |
I will point out almost every other editor window manually sets the cacheOnly flag after the insert call. EquipmentEditor doesn't, but I suspect that is not intentional. What if we included a parameter to insert that would set the object's cacheOnly. I am thinking something like a boolean that defaults to false. I would expect the standard use case is that we want cacheOnly() set to false after the insert, so using that as the default would mean the least typing. |
|
The issue in the MashWizard is that we are not calling either Brewtarget::mainWindow()->addMashStepToMash() or mashStep->setMash() when required. Assuming we want undo, we need the first call. If we don't care, then the second call. It's still a little broken with the sorting after adding new steps, but at least they show up and get sorted properly on restart. The mash designer is supposed to clear the entire mash profile before it starts, which it doesn't seem to be doing. I will point out that the "Total Wort Collected" isn't working properly in HEAD on develop. So we will fix that as a separate problem. |
|
I must admit, I was a bit puzzled about the cacheOnly flag. I see the effect but I wasn't sure about the purpose. Originally I figured it was to mark an object as "this may not end up getting saved to the database" (eg we're creating a new thing and the user could either click save or cancel). But then I thought we could infer that from the object's key being (or not being) valid, so I guessed there must be another use case for it, but I never quite followed through all the code paths to work it out. If there's some simple rule about when it should and shouldn't be set, perhaps we could manage that somewhere centrally as you suggest. |
|
I am dead against "inferring" anything. Inference like that will always break, as people forget or new people come on board. If we need a thing, we declare it and use it for that purpose. In general, I would rather things be easy to understand than to save even a few thousand lines of code. Code is cheap, my time is not. Allow me to state that again. I hate inferring status. If you need to know if a thing is this or that, use a specific and well-named flag to indicate if the thing is this or that. In this day and age, I do not really care if my in-memory foot print is now 1 byte bigger because I used a boolean where I could have inferred. Yes, there are problem domains (embedded systems, for example) where that really does matter. I am not coding brewtarget for those problem domains, and I am going to optimize my ability to read and understand the code over memory. By the way. Have I mentioned how much I hate inferring status? It is almost always clever, and there are few things more dangerous in IT than people being "clever". It usually means you have made it difficult to impossible to ever change your approach, because there are all these side effects that simply are forgotten, unknown or otherwise not fixable. Writing code it intentionally rely on a side effect is just wrong. The intent of that flag was mostly for speed. It provided a really fast way to load an object that did not require making one query to the database for each attribute for every object. As I recall, it required something like 10,000 - 20,000 queries just to load my database. It also had any number of weird side effects, because we would do things like recipe->recalcall, which would then rewrite attributes that we were going to read next. I could turn all that off by setting the cacheOnly flag. It literally dropped the number of queries by an order of magnitude. It also became useful for the editors, so that we could create something in memory and not write it until the user clicks "save". Instead of writing each attribute as a separate statement, it allowed me to do one massive write that inserted everything. The fewer trips we make to the database, the faster performance. This had the unexpected but really pleasing side effect of not actually writing a thing to the database until the user said "Ok". It is difficult to describe how annoying it was to start adding a new hop, hit "cancel" and still have to delete the entry from the tree. This was honestly not planned, but was such a relief to me that it made the entire effort worth it. The final reason is that there had been a previous attempt at some kind of caching mechanism. It was only ever applied to one of the base tables (fermentables, as I recall). When I was trying to make the networked database more performant, I had to deal with it. In studying the problem, I felt the first attempt was optimizing the wrong way. For the most part, we want one field flushed to the database immediately. Only in rare circumstances (like the editors) do we want to cache things and then do one massive flush at the end. This was my solution. I believe I changed how I was doing things half way through and stopped using the set*() methods on load. The idea still worked for those weird, calculated fields and it was so useful for the editors, so I kept it. I still think it is useful and am really set against changing it. No. Really. Do not break this part of the code unless you have a really solid replacement that does not infer status, because I really do not like inferring status. |
|
I think I get your drift. 😄 I'm pretty sure we violently agree that we should not add complexity to code in order to make some miniscule saving of memory footprint or run speed. And I too don't like it when you have to jump through a lot of hoops to work out what state an object is in. In this instance however, it seemed to me that, whatever the original history of the flag, there's now a rather direct relationship between cacheOnly and "is stored in the database". Keeping two separate ways of tracking the same thing risks creating complexity rather than simplicity. Specifically, if we look at all possible of combinations of these two flags/states, we see the following:
If we were to eliminate the cacheOnly flag, we'd reduce these four states down to two: New Object and Stored Object, which seems like a simplification to me. In particular your points above are still addressed:
This would still be the case. Calling setters on a newly-created object doesn't try to talk to the database. Only once the user clicks "save" do we insert the object in the DB, give it a key, and thereby activate the behaviour where setters update the DB record.
The ObjectStore code already does what we want here, without needing to reference cacheOnly. When loading objects from the database, it does one query per table, reads an entire row at a time from from result set and uses that data to construct an object without calling its setters. (This is the purpose of model/NamedParameterBundle.) I don't know what performance is with PostgreSQL, but it certainly seemed fast enough with SQLite on my machine. I'm not saying we need to eliminate the cacheOnly flag as part of this commit, but, if we agree it's past its sell-by date then it would make fixing some of the other bits of the code simpler. |
|
Some fixes coming too, but will probably be tomorrow as need to do some casting or similar to get it to compile. |
|
OK, Mash and MashStep should be behaving a lot better now. (I had missed finishing off the reworking of how the reference each other.) Seems to work better on my local testing, but I'm not a guru of the Mash Wizard Designer things, so please shout if you see any oddities. |
|
Sorry for the hiatus. There are issues with the mash steps and all the editors still. The mash designer still does nothing. Saying "next" should add the step to the mashes, and it doesn't. It doesn't seem to add anything, but I haven't dug hard enough into the database to determine just what is or isn't happening. Trying to run the mash wizard on a recipe that already has mashsteps dumps core like this: I've edited that a little, because the first 4 entries really weren't interesting. I am testing with a simple infusion and two batch sparges. If there are no sparge steps, two steps get created and the math looks roughly correct. Rerunning the mash wizard at that point dumps core. If I restart brewtarget there is one batch sparge step and the infusion step. Running the mash wizard dumps core again, in the same spot. Running brewtarget again shows just the infusion and no sparges. Since there are no sparge steps, two steps get created. And so on. Attempting to remove a mash step by clicking the "-" button causes the same core dump, so I am thinking removing a mash step is causing something grief. It doesn't seem to happen with any of the other tables. Based on the core dump, I suspect you may be trying to get a name that doesn't exist any longer? It would also be nice to fix the sort. The infusion step should be before the sparge steps, but I don't know if that is a related issue. |
|
Thanks. This is all good stuff. The fixes are all simple but one needs to touch a fair bit of code, so I'll work on it over the next couple of days. The one that involves lots of small changes is to do with object ownership. Essentially when we really really delete something - eg a MashStep that has no meaningful existence once it has been removed from a Mash - we should really be using shared pointers properly to make sure we can the remove the object from the DB and ObjectStore but still have it in the undo/redo stack in case the user wants to undo the deletion. The shared pointers are all already there in the ObjectStore, I just need to get a few more bits of the code to use them. (Mostly where we come back out of the database layer into the existing code, we just reach inside the shared pointer to use the raw pointer, because everything was originally done with raw pointers (and which is still correct when you're creating a Qt object that the framework owns by some chain of parenthood etc). Anyway, this use of raw pointers works fine when we're not deleting things or are only soft deleting things or can recreate the things that are deleted, which is ~99% of the code. But I need to make more things pass the shared pointer around to ensure the right things happen in the other ~1% of cases. A side benefit of this should be that we clear up a few potential memory leaks in the existing code.) |
|
OK, things should be somewhat more robust now. I'm still uncertain about some of the finer points of the Mash Wizards and Designers (eg exactly when and how it should complain about your mash being too thick for it to be able to proceed) but I don't think this is related to the database layer! |
|
Conflicts above are from the Print Preview work by @mattiasmaahl that I just merged into trunk. Will resolve merge conflicts with this branch once @mikfire happy with everything else. |
|
The mash designer still isn't behaving properly. I think this should be an easy fix and I am willing to sign off on this merge as it exists, as long as we put a problem ticket in so we don't forget to fix this. On starting the mash designer, it is supposed to delete all the existing mashsteps. Currently, it doesn't. The behavior is very strange after that, as it seems the designer is modifying the existing steps instead of creating new ones? If I manually remove the existing steps, it behaves more as I would expect. The total collected wort is wrong. That will be a signal, I am quite certain. I've broken this screen enough times I could almost tell you what signal it is. |
|
Thanks Mik, as ever, much appreciated. I have raised #620 for the mash designer problems and have merged in the new print preview stuff from @mattiasmaahl (with a small extra tweak to make it behave a bit nicer on HighDPI displays). |
This is the new database layer, which separates out:
It also implements export to BeerXML using the same new data structures etc we created to improve reading from BeerXML.
A few other things have come along for the ride because they were minor tidy-ups I'd implemented in Brewken and it would have been more effort to exclude them than to include them.
All database stuff is now in the database subfolder of src. Key files:
Database.cpp, Database.hhandle abstraction of SQLite / PostgreSQL (as before) but no longer do any object mappingDatabaseSchemaHelper.cpp, DatabaseSchemaHelper.hhandle schema upgrades and migrations, plus default data population (as before). I've disentangled this from the old Schema files that are no longer used. In particular schema changes are now hard-coded. (If we've got the SQL that, say, upgrades from v7 to v8 of the DB, we don't want this to be tied to "current" mapping as you still want to be able to do that exact same v7 to v8 step (and v8 to v9 etc etc) at any later date, including when the "current" schema has evolved and changed from whatever v7 was.) I have also moved "default data" to BeerXML and stopped using the bt_ tables. (Now that we decided not to try to edit the user's existing data, we only needed the bt_ tables for duplicate detection - ie not to import the default Recipe/Style/Hop/etc objects that are already in the user's database. But we have such logic already in the BeerXML import code. So we can just reuse that and it will automatically pull only "new" objects out ofdata/DefaultData.xml. Of course, once we support BeerJSON, we'll switch to that.)ObjectStore.cpp, ObjectStore.hDoes all the SQL to create/read/update/delete objects in the database. Normally you won't access this directly, but instead via templated functions in theObjectStoreWrappernamespaceObjectStoreTyped.h, ObjectStoreWrapper.hadd templated wrappers around ObjectStore to save callers doing lots of tedious castingObjectStoreTyped.cppcontains all the data required to map objects to and from database tablesDbTransaction.cpp, DbTransaction.hjust provide an RAII wrapper around DB transactionsA fair bit of class-specific logic that used to be inside the Database class is now inside the individual subclasses of
NamedEntity(in thesrc/modeldirectory). Eg, more of the Recipe versioning logic now lives inside the Recipe class. I've tidied up a few things with helper functions so that we have less copy-and-paste code on setter member functions.Generally, you'll see that:
ObjectStoreWrapper::findAllMatching<Recipe>()rather than aWHEREclause that you tack onto a SQL statement in a newDatabasemember function. Similarly, default data for a newly-created object is always in that object's constructor and not in the database definition.src/modeldirectory do not need to have any knowledge of SQL or XML.Additionally, I have:
PersistentSettingsBtStringConstfor wrapping compile-time-constant strings. (See class comment inutils/BtStringConst.hfor part of the motivation for this but, amongst other things, this made the Windows and Mac builds happy.)This all turned out to be a bit more work than I imagined back in January, but I'm happy with the result. There's scope to refine things a bit further (eg I think some or all of ObjectStoreTyped could be merged into ObjectStoreWrapper) but I don't see the need to do another mega-change like this one.
As ever, questions and comments most welcome.