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

Get off of slonik fork #1119

Open
lognaturel opened this issue Mar 15, 2024 · 1 comment
Open

Get off of slonik fork #1119

lognaturel opened this issue Mar 15, 2024 · 1 comment

Comments

@lognaturel
Copy link
Member

We are currently using a fork of slonik: alxndrsn/slonik@b0299fa

This is because of some stream-related issues.

At least one related patch has been accepted to upstream node-postgres: brianc/node-postgres#2836

History of postgres clients used in Central:

  • Started out with knex: chaining based query builder but wasn’t great to work with
    • Streaming-related bugs
  • Swapped to slonik because it seemed closer to desired interface
    • Tests ran 30% slower than knex (not clear on production impact)
    • Experienced several streaming-related bugs
    • Single maintainer has come and gone in availability
  • July 2022, started considering postgres.js
    • Lots of energy behind it
    • Uses a similar tag template literal interface as Slonik
    • issa discovered after finished Slonik work
    • Would have switched to it, but at the time, it didn’t support composing SQL fragments ("partials"), now introduced
    • Transactions weren't initially working, at least in tests
    • Initial PR: remove slonik, insert postgres.js #564
    • Lots of tiny differences from Slonik. For the most part, it’s an improvement. A lot of things that we did by hand before, porsager postgres has an answer.
    • porsager postgres has some weird quirks of its own though, e.g., no join utilities.
    • So far it’s a code reduction. No queries so far have needed to change. Only core infrastructure has changed so far.
    • Streaming issues - Caught errors within transactions still thrown as uncaught porsager/postgres#455; would need to add support for multiple streams

A barrier to upgrading slonik has been several breaking changes since the version we were on. It seems there's likely a more deliberate approach to breaking changes now: gajus/slonik#450 (comment)

There is now a bridge to use slonik on top of postgres.js instead of node-postgres: https://github.com/gajus/postgres-bridge

@matthew-white
Copy link
Member

matthew-white commented Mar 18, 2024

Great to have these notes centralized here! Here are a few from me:

  • I encountered a variety of errors while trying out the initial PR for postgres.js. It could be that they all trace back to the issues with streaming, but that doesn't feel certain to me. Some of the errors were thrown in the postgres.js code, while others were thrown in our code (though could still stem from postgres.js).
  • One thing that's changed in Slonik is how SSL is configured. That's mostly a good thing, because it could allow us to offer additional configuration options to our users. We know that some users have SSL configured. I wanted to make a note of this change because I think it'd be easy to miss among the other Slonik changes. One piece of awkwardness is that whatever SSL configuration we offer for Slonik will need to be available in Knex as well, since we still use Knex for migrations. Backend issue about SSL configuration: Additional SSL configuration for database #433

@lognaturel lognaturel transferred this issue from getodk/central Mar 27, 2024
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

No branches or pull requests

2 participants