PostgreSQL support

[westito]

PostgresSQL experimental support

All tests pass! (except migration) 🎉

Missing features:
- Migrations, versioning (in progress)

Modifications:
- SQL type names changed for Postgres (SQLite compatible)

[westito]

Can we run Postgres server in the CI?

[simolus3]

Thanks a lot for this PR 🚀 I just had a quick look at this with some initial comments. I'll likely have more comments once I actually try this out :D

Can we run Postgres server in the CI?

Do you have experience with GitHub actions? I think you can edit the tasks in .github/workflows/main.yml to create a task with a postgres server. If you need help with this I can take care of this as well :)

[westito]

Postgres server working in CI, initial queries seems working, but the test queries timeout. I'll try to debug CI action locally.

[westito]

Postgres CI timeout solved! Problem was 'tearDown' is not runs syncroniouly and db got deadlock. I can't make it synchronous so I put the "deleteDatabase" func to the end of each test.

[PlugFox]

We need a good ORM for postgres on dart)
Thanks you @westito

[westito]

You're welcome! I think Drift is the best ORM persistence library on dart :)

[simolus3]

Sorry for the slow update, and thanks again for all your work ❤️ I have left some comments, but most of them are fairly minor.

[westito]

We have a small problem with 64bit integers. SQLite supports 64bit (In fact, integer size can't be set), therefore I use bigint/bigserial types in Postgres dialect. The problem is, Dart web can't handle integers this size (53bit only). It would be necessary to warn the user in the documentation. Other question is, what if Postgres user want to use 32bit (or 8bit) integers only (for reduce storage size)? May we can add an IntSize enum (default Int64) parameter into integer() column definition function.
I made a research about web 64bit int support. SqlJS supports bigint by a setting, but it is maps Sql.bigint to JS BigInt type (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt) what is not supported by Dart web right now. Dart says this JS BigInt not supported all browser right now and anyways, Dart has its own 'BigInt' class that is platform-independent. "Use that if you want real 64bit on web". It is not an option for native JS libs.
dart-lang/sdk#32399
dart-lang/sdk#45092
There is a workaround, maybe that helps. I give it a try later.

[westito]

Keywords is necessary for escaping. If user use "user" as column name, Drift won't escape it in generated queries and Postgres queries will fail. If I see well the issue

[simolus3]

Keywords is necessary for escaping. If user use "user" as column name, Drift won't escape it in generated queries and Postgres queries will fail.

Oh, now I get it. I think we could either encourage postgres users to (temporarily) disable new_sql_code_generation until we sort this out or we could add a build option to specify the dialect used. The later will be considerably more complicated, but I still think that we shouldn't add unnecessary keywords for sqlite3 users.

[westito]

I am also on the side of maintaining backward compatibility as much as possible. We could add a postgres_compatible builder option and later many other thing can be switched with too.

Oh I see the main problem:
"Inside of named queries, you can use variables just like you would expect with sql. We support regular variables (?), explicitly indexed variables (?123) and colon-named variables (:id). We don't support variables declared with @ or $. "
We can't use @ because it is a special identifier in Drift files.

Then, we can leave then '?' and ':' in Drift files. 'postgres_compatible' mode also enables new_sql_code_generation. Sqlparser parses custom SQLs with ':/?', but writes '@' as parameter sign in rewrited queries.

[westito]

/// If [name] is a reserved sql keyword, wraps it in double ticks. Otherwise
/// just returns the [name] directly.
String get escapedName {
    return isKeywordLexeme(name) ? '"$name"' : name;
}

Won't be easier to escape all names regardless of it is keyword or not?

[westito]

I started to make a compatible mode, but it requires to rewrite drift and drift_dev in many places. Must update all "escapeIfNeed" call.
I think adding Postgres keywords only in runtime drift will not cause problem. It is only used for construct runtime queries, not affects custom queries.

[westito]

Okay. Current solution:

• There is a "compatible mode" which with Sqlparser looks for postgres keywords too at escaping.
• In compatible mode Drift use '@' in generated queries as parameter mark. It is not affects the custom queries itself. '?' mark must still be used in Drift files
• I added postgres keywords to runtime drift. It is only affect runtime-generated queries (invisible to use). Custom constraints / queries won't be affected either. Current SQLite queries with postgres keywords will work as before (for ex: REFERENCES(user)). On Postgres this will fail obviously, but user have to refactor his queries if he want to use the Postgres extension.

[PlugFox]

Hello, when i can try alpha version of postgres support?
I'm at an early stage of my project with backend on the dart, it could be useful for me and you)

[westito]

Hello, when i can try alpha version of postgres support?
I'm at an early stage of my project with backend on the dart, it could be useful for me and you)

Sure!

Add these lines to pubspec.yaml

dependencies:
  drift: any
  drift_postgres: any

dev_dependencies:
  drift_dev: any

dependency_overrides:
  drift:
    git:
      url: https://github.com/westito/drift.git
      ref: pg_support
      path: drift

  drift_postgres:
    git:
      url: https://github.com/westito/drift.git
      ref: pg_support
      path: extras/drift_postgres

  drift_dev:
    git:
      url: https://github.com/westito/drift.git
      ref: pg_support
      path: drift_dev

  sqlparser:
    git:
      url: https://github.com/westito/drift.git
      ref: pg_support
      path: sqlparser

EDIT: Postgres extension merged

[PlugFox]

Sure!

OK, thanks. I'll try tomorrow with current commit.
If I have any problems, I will let you know)

dependency_overrides:
  drift:
    git:
      url: https://github.com/westito/drift.git
      ref: cffbd428629691790935693d94757f2bf2043473
      path: drift

  drift_postgres:
    git:
      url: https://github.com/westito/drift.git
      ref: cffbd428629691790935693d94757f2bf2043473
      path: extras/drift_postgres

  drift_dev:
    git:
      url: https://github.com/westito/drift.git
      ref: cffbd428629691790935693d94757f2bf2043473
      path: drift_dev

  sqlparser:
    git:
      url: https://github.com/westito/drift.git
      ref: cffbd428629691790935693d94757f2bf2043473
      path: sqlparser

[westito]

Almost forget! Add these builder options too:

targets:
  $default:
    builders:
      drift_dev:
        options:
          new_sql_code_generation: true
          compatible_mode_generation: true

[simolus3]

@westito I like your idea with the build option for compatibility 👍 . Is this ready from your side now? If so I'll take a deeper look tomorrow.

[westito]

I used "compatible mode" naming (could be "generic_sql"), because dialect builder option fixes that dialect in generated code. For ex. in my project the same generated code used on mobile and on backend too. The generated code must be dialect-independent.

[simolus3]

because dialect builder option fixes that dialect in generated code. For ex. in my project the same generated code used on mobile and on backend too

I understand where you're coming from, but that will be a hard trade off to make in the long term. If we want excellent support for both sqlite3 and postgres in drift, we need to know the exact dialect at compile-time for the accurate analysis and code generation.

In your setup (which sounds super awesome btw, I ❤️ Dart on the backend), I assume you have separate packages for your app and the backend, right? Because then you could introduce a shared package that stores the .drift files. You'd then have a database class in the backend and another one in the app package, they'd both import the shared drift files. However, you can choose different builder options for the two packages to reflect that they target different database engines. Would that work for you?

[westito]

I thinking on a solution, but I have a separate [core] package that contains all the data handling (DAO, Repos, stc.) and mobile and backend depend on that. Mobile/web/backend only gives the connection (Pg, Worker or NativeDatabase). If I generate two different .g.dart files, I would need write a lot of interfaces to decide between. This does make no sense. However, SQLite understands Postgres queries (at least we currently use), so database generated for postgres will work for SQLite too. Also, this dialect builder option only affects Drift files and custom queries. If not use these, there is only runtime generated queries that is depends on connection dialect.

[simolus3]

This does make no sense

Yeah I just had the same problem applying that to the integration tests :D

However, SQLite understands Postgres queries (at least we currently use), so database generated for postgres will work for SQLite too

Yeah, just setting dialect: postgres should work for sqlite3 as well so the build option is probably fine. I still think it's the better approach in the long-term, maybe we need support generating code for multiple dialects in the future.

[simolus3]

This looks good to me now 🎉 . Thanks a lot for all your effort here, I really appreciate it!!

[westito]

I'm glad I could help 😊🥳

[davidmartos96]

@westito Awesome job! I will give it a go with a shelf server app. I was using some adhoc query helpers until now 😄