-
Notifications
You must be signed in to change notification settings - Fork 16
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
Check how the application works with postgresql #267
Comments
Here is what to do to load a sqlite dump in a postgresql database:
(I added |
@FrFerry I have only one error during the pgloader moment:
Result: the corpus table is empty. Yet I can create fresh new corpora and manage them as will, so nothing apparently broke atm. |
Well ... indeed we have more than 64 characters. These kind of restriction wasn't check by sqlite ? In my opinion we have 2 solutions here:
What do you think ? |
We should remove the limit, or set it to a higher count, such as 254 |
I pushed the modification on the pull request. |
Even though it now seems to be working, I would not merge the PR before the tests are fixed (too many failures related to sql constraints) or investigated. Might be discussed/worked in an other issue though. |
Hi, I'm taking over for @FrFerry for this ticket. @FrFerry told me that the tests are mainly broken because of constraints that SQLite did not check but PostgreSQL does, but that these are not related to the behavior of the application. He proposed to not migrate the tests to PostgreSQL and instead to continue to use SQLite for the tests. What is your position on this? |
I am currently trying to install the application using PostgreSQL - however, I cannot find the information on which version we have agreed on (if any) ? I'd go for PostgreSQL 14 unless you have some restrictions on your side ? |
Please go for it. AFAIK, we should have no issues with PSQL 14 and I am not sure that there would be huge breaking change with 13 :) |
I've done my tests with the v13.6 so far and its ok but please, proceed with v14 :) |
OK, thanks for your reply! |
Just to clarify this point, when I run the tests on #280 they fail
when you say the tests need to be fixed, do you mean the tests running using SQLite? Do you want us to port the tests as well to PostgreSQL? |
Hi @carinedengler, you should really check the current version of the dev branch, as it includes fixes for the current sqlite-only version of the application. Some tests were known for having a chance of failing (80% of the time), this seems to have fixed it. This also raises requirements version for security reasons. |
I checked the dev branch out this morning and rebased the postgresql branch on it, the 3 failing tests (using SQLite) I have are related to the changes made for the PostgreSQL port. The question is therefore whether you would want me to fix the tests so they can be run on SQLite after the port to PostgreSQL or whether you would want me to port the SQLite tests to PostgreSQL ? |
The change I am talking about seems to have been merged after your last commits ( #281 ), are you sure you are up to date ? I pinged you on the PR with the current PR fixing those tests. |
I must have fetched/rebased before you committed your latest changes, I should be up to date now
should I correct the tests for SQLite or for PostgreSQL ? or both ? |
The application should run on both SQLite and PostgreSQL, as it is still used locally by some researchers. CI Tests should also actually have a version for SQLite and one for PostgreSQL |
OK, thanks for the clarification |
when creating a new development instance with |
I think the default in dev mode should be SQLite, but we could have a new dev settings as Dev-Postgresql to facilitate work with this one |
Could you give an example how you would use that? Since db.engine.dialect.name can be used to retrieve the DBMS currently in use, and DEV_DATABASE_URL allows for passing both SQLite and PostgreSQL accesses, it seems to me that introducing another setting is not necessary. |
I think I would simply have a new config object in the config.py file that could be used from with the manage CLI |
So I'd go with the information retrieval you talk about and the DATABASE URI parameter, yes :) |
OK, I get your idea now 👍 |
the tests should now be fixed for SQLite - I won't have time before the end of the week (maybe even start next week) to have a look at the PostgreSQL tests, but introducing a switch for the tests for SQLite or PostgreSQL and fixing the PostgreSQL tests will be the next items on the list |
* feat!: change the code to support postgresql instead of sqlite * increase corpus name size * feat: automatically create PostgreSQL database when running manage.py db-create * fix: fix psycopg2-binary version * feat: use switch for DBMS-specific calls * fix: keep naming schema from SQLite development database * fix: adapt validation to new field size * fix: use dynamic list length instead of hard-coded value * fix: fix incorrect number of tokens in new corpus * feat: add separate PostgreSQL test config * refactor: use id attribute instead of hard-coded ID * feat: create test database if it doesn't exist * fix: PostgreSQL does not autoincrement if fixed id is given * fix: PostgreSQL's unique constraint message is different from SQLite's * fix: in PostgreSQL, all fields must be part of GROUP BY clause * ci: add PostgreSQL tests to CI * fix: close connection to PostgreSQL database in tearDown * fix: create database if it does not exist --------- Co-authored-by: François Ferry <[email protected]> Co-authored-by: Carine Dengler <[email protected]> Co-authored-by: Thibault Clérice <[email protected]>
Is your feature request related to a problem? Please describe.
The current app shows limits with SQLite: it's slow, and does not benefit from a fast SQL engine when having different users doing multiple writes.
Describe the solution you'd like
The text was updated successfully, but these errors were encountered: