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

Feature Request: Mysql/PostgreSQL Database option #89

Closed
CaptiveCreeper opened this issue Oct 14, 2018 · 34 comments
Closed

Feature Request: Mysql/PostgreSQL Database option #89

CaptiveCreeper opened this issue Oct 14, 2018 · 34 comments

Comments

@CaptiveCreeper
Copy link

It has been my experience that SQLite isn't the most reliable database out there and it would be nice to have the option to use a mysql database instead of SQLite. I noticed you are using LessQL as a ORM which supports mysql. How hard would it be to convert the DB gen scripts to mysql?

@berrnd
Copy link
Member

berrnd commented Oct 14, 2018

Would be doable, but definitely needs some work. I limited the data types used to them available in SQLite. Also a lot of the "core logic" is done SQL views where some SQLite specifc SQL syntax/functions are used.

Choosed SQLite because of the "everything should be as simple as possible" approach. I personally never had problems with any application that uses SQLite as the database. Also the whole Android app world is based on that database, so it's pretty stable, I think. The general data amount of grocy is also not that big (I use grocy now for over a year and the database file is still under 1 MB).

I for myself will not work on that, sorry that I have to say that - but if anyone wants to do it, I'm of course available for questions.

@dgw
Copy link

dgw commented Dec 23, 2018

SQLite is fine for low-traffic databases. MySQL would be most useful for busy instances of grocy, as the more often database writes occur, the more likely it is that SQLite will throw "Database is locked" errors. (I learned this from IRC bots based on SQLite trying to maintain "Last seen" data in busy channels.)

The best chance for this request getting implemented might be if grocy gets adopted by an operation—something bigger and more company-like than a household, though a non-profit would be good too—that starts to have scaling problems under SQLite because of the number of users they have. The organization might be willing to spend the time on adding MySQL support instead of switching platforms.

@jeauxlb
Copy link

jeauxlb commented Dec 7, 2019

Hi, I've spent some time today looking at what would be involved for enabling a MySQL backend. So far I've just modified DatabaseService.php to utilise a MySQL database rather than an sqlite database, and have been looking at modifying the queries in other parts of the code.

I did get stuck at a point early on, when I got the following Slim Application Error:
SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active. Consider using PDOStatement::fetchAll(). Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute.

This was generated in in DatabaseMigrationService.php, by the line $rowCount = $this->DatabaseService->ExecuteDbQuery('SELECT COUNT(*) FROM migrations WHERE migration = ' . $migrationId)->fetchColumn();

I have managed to work my way around that for now, but looking at MySQL logs it does seem like a lot of database connections are opened before closing others — this may be a problem to be reckoned with in the future.

For now, it the biggest challenge is the time-consuming process of converting everything in ./migrations, as the applications needs a working schema to function. @berrnd, to focus efforts on meaningful files, do you have a list of those files that are made partially/completely redundant by subsequent updates? Or better yet, a working schema in MySQL that would mean I can play around and make sure the code changes are working? 😁

The current state of play is that all paramaterised queries with a where clause, e.g. $settingRows = $this->Database->user_settings()->where('user_id = :1', $userId)->fetchAll(); need to be converted to a format MySQL can deal with. This involves replacing all instances of :n with ?, e.g. `$settingRows = $this->Database->user_settings()->where('user_id = ?', $userId)->fetchAll();

Will touch base again with any other findings, if I make further progress. I intend to update my fork of grocy in the coming days with what progress I have made.

@berrnd
Copy link
Member

berrnd commented Dec 7, 2019

Thanks for your work on this! :)

@berrnd, to focus efforts on meaningful files, do you have a list of those files that are made partially/completely redundant by subsequent updates?

No. I just put every new migration into a new file, which may depends on the migrations before. I thought about also shipping "the currrent final schema" for new installations in a single file, but so far I saw no problems about speed and the number of migrations...

A big part of the logic is done in SQL Views and Triggers and I would say a bigger part of them use SQLite specific things - so maybe that would be a lot to do/change to make it compatible...

I also still don't see the benefit. Have you hit problems with SQLite and grocy? If it's about database size and/or performance, see stats about the maybe biggest, yet still tiny, grocy production database (my personal one) in #438...

@jeauxlb
Copy link

jeauxlb commented Dec 8, 2019

Thanks for your work, more broadly!

In terms of the benefit, it's a pretty specific use-case: I intend to run grocy like a lot of my other docker containers — as High Availability (HA) services across geographic sites around the country, for my family-wide VPN. In that situation, if a single site goes down (as it can often do on residential broadband), MySQL replication will have enough data to maintain most operations before the link comes back up. I agree that many wouldn't see much any difference between a switch to MySQL from SQLite.

On the views and triggers, I've managed to cobble together a MySQL schema that works. I can now get the /stockoverview page to load successfully, but for one error in the top right of the screen:
Screen Shot 2019-12-08 at 11 14 45 pm
Screen Shot 2019-12-08 at 11 14 49 pm

My javascript is much weaker than my php, so I've been let down by not being to ascertain the flow there. Wondering if you have any tips on this, or how $userSettings is constructed more generally?

@berrnd
Copy link
Member

berrnd commented Dec 8, 2019

There are API routes (GET/PUT /user/settings/{settingKey}) to get/set user settings, which use these functions at the end.

Additionally there is the global JavaScript variable Grocy.UserSettings, which holds all user settings to not request each single one via the API, populated here.

So probably the error is about :1 and ? for SQL parameters you mentioned above?

@NikoGrano

This comment was marked as spam.

@NikoGrano
Copy link

@jeauxlb Do you have fork, where you have commited these changes, as I could also try work on this. Seems stypid me to start again doing the conversion if you have already done some.

@jeauxlb
Copy link

jeauxlb commented Dec 8, 2019

Ah — MySQL thinks you mean TABLE KEY when it sees an unescaped "key" in a where clause. Escaped now, page loads successfully.

My complete steps so far are:

  1. Add @zebardy's DatabaseService.php over the top
  2. Remove duplicate calls to DB in ExecuteDbQuery (might need to fix this up later as there's little/no error handling now)
  3. Find & replace all queries having ":[0-9]+" with "?"
  4. Go through migirations/*.sql and try to convert to MySQL. I found that running each of them in order with for SQL in *.sql; do echo $SQL; mysql -u grocy --password=grocy grocy < $SQL; done to work better than relying on Grocy to invoke

I will try to post the code later this week but it's nowhere near where it needs to be to do that right now.

@NikoGrano
Copy link

Sounds good. If you can create own fork and keep it to date, would be awesome. Just to follow your progress 👍

@zebardy
Copy link
Contributor

zebardy commented Dec 8, 2019

Just a heads up from my time digging into the code I think another issue that will need some thinking about to overcome is monitoring for Db changes. At the moment this is done by checking to see if the sqlite db file has changed. You might need to record latest update times in the db and query it out.

Glad you found the DatabaseService.php changes useful. It still needs tome tidying up as it still has the wrapper i inserted to monitor what calls were being made to the db.

@jeauxlb
Copy link

jeauxlb commented Dec 9, 2019

Can you just fake it and return a "yes, it's changed" every time? Is that practical from a usability/computational perspective? Not the most efficient obviously, but I'd just like to get it working.

@NikoGrano
Copy link

I was accually having a little sleep and thinking. Would it be impossible to rewrite this something like doctrine ORM aka, very high level abstraction?

Like in case of ORM, we would just need to rebuild the schema and migrations to be correct. Also in case of getting last modified time, I think we do not do anything with that information unless it is record specific.

I know this is lot of work, but it might pay back in long term and would help us achieve support for many other databases also.

@jeauxlb
Copy link

jeauxlb commented Dec 9, 2019

It's already using an ORM so that's exactly what my approach is. Some considerations:

  1. The schema is a collection of ~90+ SQL scripts, many of which have SQLite features that need conversion
  2. As mentioned above, the SQL statements in PHP need tweaking
  3. There are implications to code execution/DB interaction going from file-system to a connection-based server (like MySQL quitting when you establish too many connections, or like the model assumptions mentioned above that use filemtime)
  4. Probably other things
    All of these still involve a lot of troubleshooting as it's a matter of finding a problem and resolving it.

@NikoGrano
Copy link

Yeh, I see that.

However, like you said, I have seen plenty of direct SQL code in the source and many of that is SQLite, like you said. That is kind of a problem, instead of that it would make much more sense to have read-write models for tables/views. And the views could be implemented in level of PHP, so it would make database more independent and not so version/type specific.

I see your work as trying to do spaghetti top of spaghetti. Writing clear data models to define what table contains instead looking it from migrations folder would make more sense. Also in that case the data model would validate everything is setup correctly. In addition of data model, there can be this infrastructure type of approach having query and read separated into another model(s).

I also do understand your way to avoid massive rewriting trough the source.

If and when you can provide fork with your work I could start looking trough it and fixing issues while doing so. 🎖

@jeauxlb
Copy link

jeauxlb commented Dec 9, 2019

The errors started to get more involved with javascript, an area with which I'm less familiar, so I thought I'd take you up on your offer. The schema (default login) is in the zip file attached. My fork is available at https://github.com/jeauxlb/grocy
grocy.sql.zip
The code assumes a table called grocy exists (with the attached schema inside it), and has credentials grocy/grocy.

So far, known issues are:

  1. Need to specify a product group when adding a product
  2. Purchase interface (beyond adding a product) does not work (page does not progress when clicking 'OK')
  3. Migrations do not work
  4. Like many, many more: I haven't tested beyond 'purchase'/adding a product. But, it should at least run.

@zebardy
Copy link
Contributor

zebardy commented Dec 9, 2019

What are you doing to test your changes?

With a number of bits of work in progress for refactorings that impact much of the application some high level basic component tests or shared advice on testing would be helpful in detecting and understanding regressions easily.

@NikoGrano
Copy link

Well, currently nothing, I will be just testing it overall and checking for a bugs, which I could fix.

Currently I need to deal with Magento security flaws, so cannot have time for this yet...

@NikoGrano

This comment was marked as spam.

@SerialVelocity

This comment was marked as off-topic.

@berrnd

This comment was marked as off-topic.

@SerialVelocity

This comment was marked as off-topic.

@bendauphinee

This comment has been minimized.

@kriddles

This comment has been minimized.

@berrnd berrnd changed the title Feature Request: Mysql Database option Feature Request: Mysql/PostgreSQL Database option Oct 18, 2020
@pgy866

This comment has been minimized.

@berrnd

This comment has been minimized.

@pgy866

This comment has been minimized.

@berrnd

This comment has been minimized.

@martadinata666

This comment was marked as off-topic.

@zebardy

This comment was marked as off-topic.

@martadinata666

This comment was marked as off-topic.

@wtfzdotnet
Copy link

Worked on this last night and a couple of hours today, not entirely done but I've managed to get almost everything working. This in no way will be "backwards" compatible in it's current shape, but hey at least it works.

In my use-case I'm importing everything the supermarket has to offer, and just simply build recipes upon products that are available in the grocery store, makes my life much easier. The products overview page / stock overview page go bankrupt though when you try to pull up 21k products at once, so I'll still have to build in some type of pagination.

I have the chores_current view left to migrate, and then I should be all complete apart from splitting up heavy queries and modifying grocy a bit more. I'll share my fork sometime soon for anyone else who wants to help, or just set it up themselves.

Cheers!

@pasnox

This comment was marked as spam.

@berrnd

This comment was marked as off-topic.

@grocy grocy locked and limited conversation to collaborators Apr 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests