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

Fix postgres issue when updating #584

Merged
merged 6 commits into from
Jan 12, 2018

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jan 12, 2018

Postgres could not be used in 12. It installed the tables with wrong names
so we need to ignore it on the update steps. The wrong tables will be
dropped and new tables will be created with further migrations.
But we shouldn't try to migrate data from the old and broken layout.

Fix #578
Fix #543

Postgres could not be used in 12. It installed the tables with wrong names
so we need to ignore it on the update steps. The wrong tables will be
dropped and new tables will be created with further migrations.
But we shouldn't try to migrate data from the old and broken layout.

Signed-off-by: Joas Schilling <coding@schilljs.com>
Actually it's not postgres having the problem, but migrations.
Postgres allows camelCased columns, but they need to be quoted.
They were quoted from the old database.xml file but this does
not work anymore in migrations. But that means it worked on
postgres in the past, it just didn't work in 2.9~ to install
since migrations where merged. So we are reverting the check
for postgres and only allow updating from 2.0.0 or later.

This makes sure that when newly installing the migration code
is not run.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@enoch85
Copy link
Member

enoch85 commented Jan 12, 2018

So this means that old DB enteries will be purged? I'd like an as clean DB as possible.

The issue is that migrations always generate lowercase columns only.
So all columns that were created with database.xml are okay and cC.
But all intermediate columns the migrations create need to be
referenced as lowercase columns in the queries.

In the future we won't have this problem anymore because we switched
to under_score column names for 3.0

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

Actually its a bit different:

Actually it's not postgres having the problem, but migrations.
Postgres allows camelCased columns, but they need to be quoted.
They were quoted from the old database.xml file but this does
not work anymore in migrations. But that means it worked on
postgres in the past, it just didn't work in 2.9~ to install
since migrations where merged. So we are reverting the check
for postgres and only allow updating from 2.0.0 or later.

This makes sure that when newly installing the migration code
is not run.

Make sure Postgres uses lowercase for the migration generated columns

The issue is that migrations always generate lowercase columns only.
So all columns that were created with database.xml are okay and cC.
But all intermediate columns the migrations create need to be
referenced as lowercase columns in the queries.

In the future we won't have this problem anymore because we switched
to under_score column names for 3.0


@enoch85 I have no idea what you are talking about

So this means that old DB enteries will be purged?

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Copy link
Member

@Ivansss Ivansss left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@nickvergessen nickvergessen merged commit 70ce12f into master Jan 12, 2018
@nickvergessen nickvergessen deleted the bugfix/578/postgres-update-issue branch January 12, 2018 16:02
@enoch85
Copy link
Member

enoch85 commented Jan 12, 2018

Nvm, I'm glad it wasn't a PostgreSQL issue.

marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants