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

Use native postgres boolean type instead of integers #2116

Merged
merged 19 commits into from
Nov 8, 2022

Conversation

abitofevrything
Copy link
Contributor

Makes it so that databases connected to postgresql will use the boolean type for boolean columns, instead of integers mapped to booleans as they are in sqlite.

These changes were made to be non-breaking but could use some cleanup in the next major release - mainly changing the SqlTypes methods to get the current dialect from somewhere else than an optional positional parameter.

Closes #2113

@abitofevrything abitofevrything marked this pull request as draft October 29, 2022 20:19
@abitofevrything
Copy link
Contributor Author

Converted to draft as this PR has issues with backwards compatibility - databases generated before this PR would not be compatible with the contents of this PR.

I'll work on allowing both integer and boolean columns to represent booleans in postgres, and users can migrate simply by changing the column type.

@simolus3
Copy link
Owner

simolus3 commented Nov 2, 2022

databases generated before this PR would not be compatible with the contents of this PR.

You mean the generated database code, right? I think this is fine as long we keep the dependency constraints between drift and drift_dev consistent. The current contract is that we can make "breaking" changes in minor drift versions as long as the generator with the same minor version will generate correct code. It means that users will have to re-run build_runner build after upgrading their dependencies, but at least drift and drift_dev don't get out-of-sync.

@abitofevrything
Copy link
Contributor Author

No, I mean the actual database - the column type from before this PR would still be integer which is incompatible.

Since postgres accepts 1 and 0 as Boolean literals, it would be possible to be backwards compatible if it weren't for the postgres library only accepting bool for Boolean arguments and int for integer arguments in queries - other than that, reading from the db and constants embedded in the query as literals are compatible with both column types, so arguments are the only real blocker.

@abitofevrything
Copy link
Contributor Author

abitofevrything commented Nov 3, 2022

It would be easy for users to migrate though, simply ALTER TABLE tbl_name ALTER COLUMN old_integer_boolean_column DROP CHECK SET TYPE boolean USING integer::boolean;, or something along those lines (wrote that from memory).

@simolus3
Copy link
Owner

simolus3 commented Nov 3, 2022

Oh right, of course. Given that our support for postgres is still somewhat experimental, I think that shouldn't be a problem either. I think it's better to just do this right now instead of adding (and testing and maintaining) a build option just to get the broken behavior.

@AlexKenbo
Copy link

It would be easy for users to migrate though, simply ALTER TABLE tbl_name ALTER COLUMN old_integer_boolean_column DROP CHECK SET TYPE boolean USING integer::boolean;, or something along those lines (wrote that from memory).

Can you tell me if we are just starting out, can we use your commite right away?
We have been writing backends in Dart for 1.5 years, but we don't work with SQL directly. We work via Hasura :)

Now the application has grown and we need more control on the backend and we don't want to leave Dart for Java.

We already had a nice experience with Moor. We want to try it with Postgres as well.
We're in business and we want to move in this month, hopefully all will be good.

And ideally we will join in on the package.

I'd like your opinion @simolus3 - why is this still experimental and what might we encounter?
We already have experience writing complex queries and embedded SQL procedures.

@abitofevrything
Copy link
Contributor Author

Yes, this PR is in a working state. You just need to migrate existing databases, both the actual database contents and the generated database file to use it though.

I'll reopen it later, as if backwards compatibility isn't a huge concern then there is no reason for this to be kept as a draft.

@abitofevrything abitofevrything marked this pull request as ready for review November 3, 2022 21:22
@abitofevrything
Copy link
Contributor Author

After a bit of experimenting, this is the SQL migration query for old integer columns to the new boolean columns:

ALTER TABLE tbl_name DROP CONSTRAINT col_constraint, ALTER COLUMN col_name TYPE boolean USING col_name::boolean;

Where:

  • tbl_name is the name of the table containing the column
  • col_constraint is the name of the old CHECK value IN (0, 1) constraint (can be found with \d+ from psql, generally looks like $tblName_$colName_check)
  • col_name is the name of the column to change.

Run that query in a migration for every boolean column in your database, and that's all the migration done.

Copy link
Owner

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

I like the idea and the approach! My only concern is the increased code size, but I think we can fix that.

drift/lib/src/runtime/types/mapping.dart Outdated Show resolved Hide resolved
drift/test/generated/todos.g.dart Outdated Show resolved Hide resolved
drift_dev/lib/src/writer/tables/table_writer.dart Outdated Show resolved Hide resolved
Copy link
Owner

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@simolus3 simolus3 merged commit 22d30bd into simolus3:develop Nov 8, 2022
@simolus3
Copy link
Owner

@abitofevrything To reduce the size of the generated code, dialects for which we generate code now need to be enabled (in drift 2.11.0). If you only need postgres support, you can use dialect: postgres in your build.yaml. If you need database code that works on sqlite3 and on postgres as well, you can use this setup. Without that option, the postgres-specific types and constraints would be removed from generated code in the latest drift version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when using boolean columns in postgresql when used as a boolean expression
3 participants