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 (client): support for PG on the client #1038

Merged
merged 156 commits into from
May 2, 2024

Conversation

kevin-dp
Copy link
Contributor

@kevin-dp kevin-dp commented Mar 7, 2024

This PR adds support for Postgres on the client:

  • adds DB drivers for Postgres in NodeJS and in Tauri
  • modifies the initial Electric schema (meta tables)
  • modifies the queries executed by the BundleMigrator
  • modifies the triggers to also support PG
  • modifies the Satellite process to also support PG
  • modifies the CLI to bundle both SQLite and PG migrations
  • modifies the DAL to transform JS values to PG when writing and the other way around when reading

To test this PR you will need to:

  • use a local build of Electric from alco/vax-1659-postgresql-dialect
  • npm pack the generator
  • use the packed generator in the ts-client
  • npm pack the ts-client
  • use the packed version of ts-client in the example app (and use node-postgres or tauri-postgres driver)

Limitations:

  • Floating-point numbers that are out of range cannot be inserted (PG throws an error, in contrast to SQLite which silently converts them to +Inf/-Inf (for too big values) or 0 (for too small values))
  • Not sure if PGlite version currently supports infinity values and/or NaN for floating-point numbers. Our triggers do some casting of these values, need to test this with some e2e tests or actual tauri application to see if it works or not.

A couple of things to note regarding the implementation:

  • Had to change a trigger in both SQLite and PG for storing the JSON of a row's PK:
    • In PG, jsonb adds whitespace and re-orders keys. Whereas, SQLite has no whitespace and preserves the ordering of keys. So in PG we use json instead of jsonb and strip any whitespace but that has the side effect of also removing null values. Hence, we also remove null values in SQLite.
  • Postgres requires migrations coming from Electric to define all FKs as DEFERRABLE
  • We no longer defer foreign key checks before applying an incoming transaction as PG does not like deferred constraints when trying to apply some DDL. This happens because PG wants to run all trigger events before applying DDL but they are deferred so they are only allowed to run when committing so PG is stuck. So if a TX contains DML and DDL then when it encounters the DDL we get errors like this: cannot ALTER TABLE "parent" because it has pending trigger events

Remains to be done (perhaps in a follow-up PR):

  • e2e tests for PG client

For reviewers:

  • The crux of this PR consists of modifying the TS-client to abstract away dialect-specific SQL queries behind a query builder abstraction:
    • src/migrators/query-builder/builder.ts defines an abstract QueryBuilder class that every dialect must implement
    • src/migrators/query-builder/sqliteBuilder.ts and ``src/migrators/query-builder/pgBuilder.ts` implement the abstract class for SQLite and Postgres respectively
  • When electrifying a database, the dialect can (optionally) be provided in the config object. It defaults to 'SQLite'. Postgres drivers' electrify function pass 'Postgres' dialect through this config object. The DAL checks the dialect in order to apply the right conversions from JS values to SQLite/PG values and back.
  • The main changes are in src/migrators/bundle.ts, src/migrators/schema.ts, and src/migrators/triggers.ts to use the dialect-specific query builder object.
  • Likewise, the satellite process (src/satellite/process.ts) has been changed to use the dialect-specific query builder for constructing its specialised queries.
  • Another important change consists of supporting data types such as booleans, dates, json, etc.
    • In SQLite such values were converted to strings and stored as strings (since SQLite has no real support for them).
    • Postgres has support for those data types so we want to leverage that.
    • Again, we abstracted away the data type conversions behind a Converter interface (src/client/conversions/converter.ts).
      • Then we created 2 specific converters, 1 for SQLite src/client/conversions/sqlite.ts and 1 for PG src/client/conversions/postgres.ts
  • Similar to the value conversions, we also had to change serialisation/deserialisation of values in the satellite client. Currently it was encoding SQLite values on the wire and decoding values from the wire into SQLite values. We abstracted those encoders/decoders and created 2 instances of each, one for SQLite and one for PG (cf. src/util/encoders).

Copy link

linear bot commented Mar 7, 2024

@kevin-dp kevin-dp marked this pull request as ready for review March 11, 2024 13:51
@kevin-dp kevin-dp changed the title (WIP) feat (client): support for PG on the client feat (client): support for PG on the client Mar 11, 2024
@kevin-dp kevin-dp force-pushed the kevindp/vax-1638-pg-generator branch 3 times, most recently from 9987b45 to 21d114b Compare March 14, 2024 13:39
@kevin-dp kevin-dp force-pushed the kevindp/vax-1638-pg-generator branch 2 times, most recently from 570b24f to 37bb5b9 Compare April 8, 2024 13:58
@msfstef msfstef force-pushed the kevindp/vax-1638-pg-generator branch from 37bb5b9 to 95f3314 Compare April 9, 2024 08:05
@kevin-dp kevin-dp force-pushed the kevindp/vax-1638-pg-generator branch 6 times, most recently from d17b56b to 337ee9d Compare April 23, 2024 11:28
Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Amazing work! Left a few minor comments, might do a second pass/review later!

clients/typescript/src/client/conversions/converter.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/conversions/postgres.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/conversions/postgres.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/model/builder.ts Outdated Show resolved Hide resolved
clients/typescript/src/migrators/triggers.ts Outdated Show resolved Hide resolved
clients/typescript/src/satellite/client.ts Outdated Show resolved Hide resolved
clients/typescript/src/satellite/client.ts Outdated Show resolved Hide resolved
@kevin-dp kevin-dp force-pushed the kevindp/vax-1638-pg-generator branch from 8a93815 to 3dcf5a6 Compare April 23, 2024 14:53
@kevin-dp kevin-dp requested a review from msfstef April 24, 2024 07:44
@kevin-dp
Copy link
Contributor Author

e2e test 03.25_node_satellite_can_resume_replication_after_server_restart.lux consistently fails on CI but always passes on my machine locally. Not sure why it fails on CI.

Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Did a second pass, looking good!

clients/typescript/src/satellite/client.ts Outdated Show resolved Hide resolved
clients/typescript/src/satellite/process.ts Show resolved Hide resolved
@kevin-dp
Copy link
Contributor Author

e2e test 03.25_node_satellite_can_resume_replication_after_server_restart.lux consistently fails on CI but always passes on my machine locally. Not sure why it fails on CI.

When client reconnects, Electric restores some information from the DB we then checked that that information was gone from the DB. However, CI is slow and Electric did not yet restore the information by the time we checked that the information was gone from the DB. Now added an explicit match in the e2e tests to make sure we only check the DB state after that information has been restored by Electric.

@kevin-dp kevin-dp force-pushed the kevindp/vax-1638-pg-generator branch 3 times, most recently from 6ddf542 to 4b861d7 Compare April 25, 2024 12:42
@kevin-dp kevin-dp force-pushed the kevindp/vax-1638-pg-generator branch 2 times, most recently from ba47f26 to 360e85e Compare April 29, 2024 13:49
clients/typescript/package.json Outdated Show resolved Hide resolved
clients/typescript/src/util/tablename.ts Outdated Show resolved Hide resolved
clients/typescript/src/satellite/process.ts Outdated Show resolved Hide resolved
clients/typescript/src/satellite/process.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@icehaunter icehaunter left a comment

Choose a reason for hiding this comment

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

Last batch of comments. Honestly mostly nitpicks, although I really want package.json and deferFKs addressed before we proceed. Feel free to ignore/address the rest as you see fit.

Very good work overall, speaks to a reasonable code organisation that this change was actually impressively reviewable and contained

clients/typescript/src/migrators/schema.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/conversions/input.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/model/builder.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/model/builder.ts Outdated Show resolved Hide resolved
@alco
Copy link
Member

alco commented May 1, 2024

@kevin-dp Don't forget to add a changeset.

@kevin-dp kevin-dp requested a review from icehaunter May 2, 2024 09:52
Copy link
Contributor

@icehaunter icehaunter left a comment

Choose a reason for hiding this comment

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

Thanks for addressing feedback! Final function deletion and LGTM

clients/typescript/src/client/conversions/input.ts Outdated Show resolved Hide resolved
@samwillis samwillis merged commit 450a65b into main May 2, 2024
27 of 28 checks passed
@samwillis samwillis deleted the kevindp/vax-1638-pg-generator branch May 2, 2024 15:34
samwillis added a commit that referenced this pull request May 2, 2024
Enables using Linearlite with PGlite or wa-sqlite by setting the
`ELECTRIC_CLIENT_DB` env var to either `wa-sqlite` or `pglite`. This can
be done in the `.env` file or any other standard way. It will be
embedded into the build so a build will be for one or the other.

To be merged after #1038 with the `electric-sql` rep either pointing at
a canary build or a published version.
samwillis added a commit that referenced this pull request May 14, 2024
Preview: https://deploy-preview-128--electric-sql-website.netlify.app

node-postgres driver docs may need editing if a change is made due to
this:
#1038 (comment)
alco pushed a commit that referenced this pull request May 19, 2024
Preview: https://deploy-preview-128--electric-sql-website.netlify.app

node-postgres driver docs may need editing if a change is made due to
this:
#1038 (comment)
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.

6 participants