Migrate MLS DB To SQLC #380

neekolas · 2024-04-25T03:01:06Z

tl;dr

Bun has been bothering me for a while, so I've been working on a migration that will get rid of our ORM altogether and just use boring SQL queries for everything.

sqlc is a very slick tool to generate code based on plain SQL queries using placeholders for arguments. It's not perfect...I had to do some gymnastics to make a few of the query types work.

But the fact that there is no runtime other than the standard SQL driver and some generated code outweighs its limitations IMO. There's no fancy ORM library to worry about mangling your queries, and the learning curve is basically just "how well do you know SQL".

What's wrong with Bun?

No support for serializable transactions
The SQL driver is not as well maintained as PGX
High learning curve to build complex queries, even if you know SQL well
Relations system is not very powerful and ends up doing N+1 queries a lot of the time.
Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema.

Things that suck right now with sqlc

I can't find a good way to have dynamic ORDER BY expressions. So I literally have separate queries for ASC and DESC versions. It's not the end of the world, but it's very frustrating. There's an issue to fix it, and some hacky workarounds using CASE statements, but it's not great.
Making the filters play nice with json_populate_recordset is a bit of a pain. Switching to the pgx driver helped, since I think there was a bug in Bun's pgdriver.

Migration plan

We use Bun in a lot of places and for a lot of things today.

It powers the authz database and all the migrations there
It powers the migrations for the message database (but not the queries)
It powers the mls database and all the queries in the mlsstore.

The priority right now is to remove it from the mlsstore. We will still use it for migrations (sqlc can read Bun migrations just fine).

This involves replacing the bun pgdriver with pgx (done in this PR) and replacing all the Bun ORM queries with sqlc queries. I have most of the queries written, but I'll split up the actual migration over several PRs. This can be done incrementally, but once the process is complete we can delete the Bun models.

We aren't using any of the fancy sqlc cloud features and have no plans to.

What knucklehead brought Bun into our codebase?

Ummmm. 😬. That was me.

…o nm/sqlc-experiment

neekolas · 2024-04-25T03:01:20Z

Add InsertLog Query #383
Add GetInboxIDs and use validation service in PublishIdentityUpdates #382
Migrate first store methods to sqlc #381
Migrate MLS DB To SQLC #380 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @neekolas and the rest of your teammates on Graphite

…o nm/sqlc-experiment

neekolas · 2024-04-25T15:43:05Z

pkg/mls/store/queries.sql

+ORDER BY sequence_id ASC FOR UPDATE;
+
+-- name: GetInboxLogFiltered :many
+SELECT a.* FROM inbox_log AS a


This is probably the fanciest query we have.

We need to take in an array of (inbox_id, sequence_id) pairs and get the results of each inbox that are greater than the sequence_id. We are currently doing this in one query per inbox, but this change makes it all happen in one query total by joining against a virtual table.

null::inbox_filter is a custom SQL data type (not a table) that we decode the JSON serialized filters into

neekolas · 2024-04-25T15:46:58Z

pkg/mls/store/queries/queries.sql.go

@@ -0,0 +1,472 @@
+// Code generated by sqlc. DO NOT EDIT.


This file is entirely generated based the the queries.sql

37ng

Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema.

How does sqlc manage db schema? From pkg/mls/store/queries/models.go I see that it's generating go structs based on migration history?

neekolas · 2024-04-25T19:27:43Z

Merge activity

Apr 25, 3:27 PM EDT: @neekolas started a stack merge that includes this pull request via Graphite.
Apr 25, 3:28 PM EDT: @neekolas merged this pull request with Graphite.

Bun has been bothering me for a while, so I've been working on a migration that will get rid of our ORM altogether and just use boring SQL queries for everything. [`sqlc`](https://sqlc.dev/) is a very slick tool to generate code based on plain SQL queries using placeholders for arguments. It's not perfect...I had to do some gymnastics to make a few of the query types work. But the fact that there is no runtime other than the standard SQL driver and some generated code outweighs its limitations IMO. There's no fancy ORM library to worry about mangling your queries, and the learning curve is basically just "how well do you know SQL". - No support for serializable transactions - The SQL driver is not as well maintained as PGX - High learning curve to build complex queries, even if you know SQL well - Relations system is not very powerful and ends up doing N+1 queries a lot of the time. - Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema. - I can't find a good way to have dynamic ORDER BY expressions. So I literally have separate queries for ASC and DESC versions. It's not the end of the world, but it's very frustrating. There's an [issue to fix it](sqlc-dev/sqlc#2061), and some hacky workarounds using CASE statements, but it's not great. - Making the filters play nice with `json_populate_recordset` is a bit of a pain. Switching to the `pgx` driver helped, since I think there was a bug in Bun's pgdriver. We use Bun in a lot of places and for a lot of things today. - It powers the `authz` database and all the migrations there - It powers the migrations for the `message` database (but not the queries) - It powers the `mls` database and all the queries in the `mlsstore`. The priority right now is to remove it from the `mlsstore`. We will still use it for migrations (`sqlc` can read Bun migrations just fine). This involves replacing the bun `pgdriver` with `pgx` (done in this PR) and replacing all the Bun ORM queries with `sqlc` queries. I have most of the queries written, but I'll split up the actual migration over several PRs. This can be done incrementally, but once the process is complete we can delete the Bun models. We aren't using any of the fancy `sqlc` cloud features and have no plans to. Ummmm. 😬. That was me.

…382) * Initial commit * Set up sqlc * Initial commit * Set up sqlc * Migrate first store methods to sqlc * Migrate first store methods to sqlc * Migrate first store methods to sqlc * Add GetAddressLog SQL * Add GetAddressLog SQL * Merge branch '04-24-migrate_first_store_methods_to_sqlc' of github.com:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc * Add GetAddressLog SQL * Initial commit * Set up sqlc * Initial commit * Set up sqlc * Add InsertLog Query (#383) * Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests * Migrate first store methods to sqlc * Migrate MLS DB To SQLC (#380) Bun has been bothering me for a while, so I've been working on a migration that will get rid of our ORM altogether and just use boring SQL queries for everything. [`sqlc`](https://sqlc.dev/) is a very slick tool to generate code based on plain SQL queries using placeholders for arguments. It's not perfect...I had to do some gymnastics to make a few of the query types work. But the fact that there is no runtime other than the standard SQL driver and some generated code outweighs its limitations IMO. There's no fancy ORM library to worry about mangling your queries, and the learning curve is basically just "how well do you know SQL". - No support for serializable transactions - The SQL driver is not as well maintained as PGX - High learning curve to build complex queries, even if you know SQL well - Relations system is not very powerful and ends up doing N+1 queries a lot of the time. - Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema. - I can't find a good way to have dynamic ORDER BY expressions. So I literally have separate queries for ASC and DESC versions. It's not the end of the world, but it's very frustrating. There's an [issue to fix it](sqlc-dev/sqlc#2061), and some hacky workarounds using CASE statements, but it's not great. - Making the filters play nice with `json_populate_recordset` is a bit of a pain. Switching to the `pgx` driver helped, since I think there was a bug in Bun's pgdriver. We use Bun in a lot of places and for a lot of things today. - It powers the `authz` database and all the migrations there - It powers the migrations for the `message` database (but not the queries) - It powers the `mls` database and all the queries in the `mlsstore`. The priority right now is to remove it from the `mlsstore`. We will still use it for migrations (`sqlc` can read Bun migrations just fine). This involves replacing the bun `pgdriver` with `pgx` (done in this PR) and replacing all the Bun ORM queries with `sqlc` queries. I have most of the queries written, but I'll split up the actual migration over several PRs. This can be done incrementally, but once the process is complete we can delete the Bun models. We aren't using any of the fancy `sqlc` cloud features and have no plans to. Ummmm. 😬. That was me. * Migrate first store methods to sqlc * Add InsertLog Query (#383) * Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests * Add GetAddressLog SQL * Merge branch '04-24-migrate_first_store_methods_to_sqlc' of github.com:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc * Add InsertLog Query (#383) * Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests * Add InsertLog Query (#383) * Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests --------- Co-authored-by: Andrew Plaza <[email protected]>

neekolas · 2024-04-26T22:54:26Z

How does sqlc manage db schema? From pkg/mls/store/queries/models.go I see that it's generating go structs based on migration history?

That's right. You just point it at your migrations folder and it reads all of them to assemble the schema. You can also point it at a live database if you want, but we don't have that configured since the migration folder seems to work just fine.

37ng · 2024-04-26T23:29:41Z

That's right. You just point it at your migrations folder and it reads all of them to assemble the schema. You can also point it at a live database if you want, but we don't have that configured since the migration folder seems to work just fine.

@neekolas So it's reversed Bun? Bun generates schema from defined go structs/tags.

neekolas added 5 commits April 24, 2024 07:59

Initial commit

831b4e6

Set up sqlc

a99c43b

Initial commit

86b7131

Set up sqlc

2fa85f6

Merge branch 'nm/sqlc-experiment' of github.com:xmtp/xmtp-node-go int…

1a44dc9

…o nm/sqlc-experiment

neekolas mentioned this pull request Apr 25, 2024

Migrate first store methods to sqlc #381

Merged

neekolas changed the title ~~Initial commit~~ Migrate MLS DB To SQLC Apr 25, 2024

neekolas added 4 commits April 24, 2024 20:13

Merge branch 'nm/sqlc-experiment' of github.com:xmtp/xmtp-node-go int…

3d192f3

…o nm/sqlc-experiment

Merge branch 'nm/sqlc-experiment' of github.com:xmtp/xmtp-node-go int…

7d9d984

…o nm/sqlc-experiment

Merge branch 'nm/sqlc-experiment' of github.com:xmtp/xmtp-node-go int…

8b434fd

…o nm/sqlc-experiment

Merge branch 'nm/sqlc-experiment' of github.com:xmtp/xmtp-node-go int…

5a658b5

…o nm/sqlc-experiment

neekolas mentioned this pull request Apr 25, 2024

Add GetInboxIDs and use validation service in PublishIdentityUpdates #382

Merged

neekolas commented Apr 25, 2024

View reviewed changes

neekolas marked this pull request as ready for review April 25, 2024 15:55

neekolas requested a review from a team April 25, 2024 16:02

insipx mentioned this pull request Apr 25, 2024

Add InsertLog Query #383

Merged

37ng approved these changes Apr 25, 2024

View reviewed changes

insipx approved these changes Apr 25, 2024

View reviewed changes

neekolas merged commit ab5ea92 into main Apr 25, 2024
4 checks passed

neekolas deleted the nm/sqlc-experiment branch April 25, 2024 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate MLS DB To SQLC #380

Migrate MLS DB To SQLC #380

neekolas commented Apr 25, 2024 •

edited

Loading

neekolas commented Apr 25, 2024 •

edited by insipx

Loading

neekolas Apr 25, 2024 •

edited

Loading

neekolas Apr 25, 2024

neekolas Apr 25, 2024

37ng left a comment •

edited

Loading

neekolas commented Apr 25, 2024 •

edited

Loading

neekolas commented Apr 26, 2024

37ng commented Apr 26, 2024

Migrate MLS DB To SQLC #380

Migrate MLS DB To SQLC #380

Conversation

neekolas commented Apr 25, 2024 • edited Loading

tl;dr

What's wrong with Bun?

Things that suck right now with sqlc

Migration plan

What knucklehead brought Bun into our codebase?

neekolas commented Apr 25, 2024 • edited by insipx Loading

neekolas Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

neekolas Apr 25, 2024

Choose a reason for hiding this comment

neekolas Apr 25, 2024

Choose a reason for hiding this comment

37ng left a comment • edited Loading

Choose a reason for hiding this comment

neekolas commented Apr 25, 2024 • edited Loading

Merge activity

neekolas commented Apr 26, 2024

37ng commented Apr 26, 2024

neekolas commented Apr 25, 2024 •

edited

Loading

neekolas commented Apr 25, 2024 •

edited by insipx

Loading

neekolas Apr 25, 2024 •

edited

Loading

37ng left a comment •

edited

Loading

neekolas commented Apr 25, 2024 •

edited

Loading