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

feat(PostgreSQL DB Storage): Added support for Postgres. Changes incl… #635

Merged
merged 2 commits into from
Nov 14, 2019
Merged

Conversation

srekapalli
Copy link
Contributor

@srekapalli srekapalli commented Nov 13, 2019

  • Added changesets for postgres
  • Master changeset will include changesets required for postgres which get conditionally executed based on the dbms type.
  • Modified an existing changeset ( for adding indexes ) to conditionally execute for MySQL only.
  • Adjusted JOOQ api to work with both MySQL and PostgreSQL.

/cc @cfieber @robzienert @ajordens

…ude liquibase changesets to target postgres with test running against both MySQL & Postgres in test containers
Comment on lines +1 to +7
databaseChangeLog:
- changeSet:
preConditions:
onFail: CONTINUE
dbms:
type: postgresql
id: create-indexes-postgresql
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changeset is similar to the MySQL one except that PgSQL indexes are globally scoped and we can't have same index name for every table and this changeset is applying that rule.

Copy link
Member

Choose a reason for hiding this comment

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

What happens with the schemas defined by MySQL with the duplicate index names? Does pg still create the indexes but they're wrong, or what? I would expect that if the indexes conflict, it would generate an error and fail to apply the changeset, so we'd never land here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PG will fail to execute the changeset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you meant by so we'd never land here , but our original changeset that we had will fail for postgres and that's the main reason I had to add preconditions in there.

@@ -0,0 +1,89 @@
databaseChangeLog:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new changeset only applied for Postgres. CHAR types are converted to bpchar (blank pad characters) in Postgres and they are not recommended datatypes + JOOQ doesn't play well with padded column values. So this change set modifies the column data type to VARCHAR.

Copy link
Member

Choose a reason for hiding this comment

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

I did a quick look around but came up short - could you link where it's documented that char / bpchar is not recommended? I'd like to get more learned on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@cfieber cfieber left a comment

Choose a reason for hiding this comment

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

minor suggestion but otherwise LGTM

front50-sql/README.md Outdated Show resolved Hide resolved
front50-sql/README.md Outdated Show resolved Hide resolved
front50-sql/README.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants