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(js-connectors): use standard provider names ("mysql", "postgres", ...) #4141

Merged
merged 8 commits into from
Aug 29, 2023

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Aug 16, 2023

@jkomyno jkomyno marked this pull request as ready for review August 16, 2023 13:26
@jkomyno jkomyno requested a review from a team as a code owner August 16, 2023 13:26
@jkomyno jkomyno requested a review from tomhoule August 16, 2023 13:26
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 16, 2023

CodSpeed Performance Report

Merging #4141 will not alter performance

Comparing feat/allow-std-provider-names (b49048d) with main (08cc777)

Summary

✅ 11 untouched benchmarks

@jkomyno jkomyno added this to the 5.2.0 milestone Aug 16, 2023
@tomhoule
Copy link
Contributor

I am trying to understand the rationale and I'm not sure I completely get the advantages.

It slows down testing new connectors (such as a new pg JS connector, see Slack).

The lower amount of effort will only apply strictly for connectors we can transparently treat as mysql or postgres — so the pg connector, but for example not a hypothetical d1 or mssql connector. That's pretty narrow.

Switching from e.g. the Neon JS connector to the pg JS connectors requires changing both the values passed to the PrismaClient constructor, as well as the Prisma schema's datasource provider name.

I'm not familiar with the reasoning behind the current prisma client api (and I gather it is being iterated on) so I don't have an opinion here. Only that it doesn't sound extremely painful.

It requires workarounds for testing JS Connectors in prisma/prisma.

Also not familiar with this one, I'll let you weigh the tradeoff, but we should think about how we want to test this ultimately. Adding one axis of conditionals to engine initialization has clear downsides, we've worked hard to reduce that in the past.

It requires creating a new static connector struct in Rust for every new JS Connector we want to support. Examples: neon.rs.

This is the main point for me. With this PR, the query engine knows it has a JS connector on the other end (from the newly introduced connector mode), but it loses the information about which connector (neon vs. pg, for example). Not differentiating between these two may work for a proof of concept, but my hunch is that we will to have that information in the QE. Before this PR, we do have it, and we would lose it. Not saying that it's the only two alternatives, we could imagine something similar to the LSP initialization request for the JS connector to report more info to the query engine, and runtime dispatch on that for example.

Beyond the QE, this PR removes the mechanism we would need to have different PSL validations for different node driver based connectors.

Posting this with the caveat that I'm still not clear on where we are going with node drivers at the moment, how much tech debt we are willing to take on and what the scope of the current changes is, but assuming this is going to end up being shipped.

wdyt?

@jkomyno
Copy link
Contributor Author

jkomyno commented Aug 17, 2023

Not differentiating between these two may work for a proof of concept, but my hunch is that we will to have that information in the QE

I wonder, why would the QE need to know we're using the pg JS connector or the neon JS connector?

The following holds:

  • The QE needs to know the flavour ("postgres" | "mysql") of the database used by the JS connector, in order to select the correct graph building strategy. The flavour is provided by every JS Connector already.
  • The PSL for Neon or PlanetScale doesn't vary compared to postgres and mysql. Previously, JS connectors were forced to implement relationMode = "prisma" only, but that was not the right call, as confirmed by Jan.
  • The Schema Engine also needs to know the same flavour ("postgres" | "mysql"), for using the Rust drivers when the user runs prisma db and prisma migrate commands.
  • Data-type conversions for JS connectors are defined only once in Rust, and applied to every JS connector. Every JS connector's data is (de)serialised uniformly, and any divergence is handled in TypeScript-side, so that Rust can offer a single unified type conversion strategy: https://github.com/prisma/team-orm/issues/257.

Happy to correct my point of view, in case I've missed anything here ☺️

@tomhoule
Copy link
Contributor

I wonder, why would the QE need to know we're using the pg JS connector or the neon JS connector?

I don't have concrete examples in mind, but my assumption would be that planetscale / neon have or will have features (extending the schema or client) or constraints (restricting the schema or client) in their server or their serverless clients that are different from stock mysql / postgres. That's equivalent to the question of whether these two should be separate connectors or not, regardless of how the connectors are implemented.

The relevance of this to your bullet points would only be the first sentence in your second bullet point, where I would disagree that this will continue to be true. In the first and third bullet points, either the current solution in main or the one from this PR works, no arguing. For the fourth, I wasn't thinking of that part of the connector at all, but more about user-facing things: client and schema APIs.

@jkomyno
Copy link
Contributor Author

jkomyno commented Aug 17, 2023

@tomhoule Ok, then let's keep this around as a reference PR, without merging it / reviewing it further until we have the tools for making a more informed decision. I'll prepare a separate PR that just allows using relationMode = "foreignKeys" for neon and planetscale, which should be enough for unblocking Joel's testing experiments in prisma/prisma for now.

@aqrln aqrln modified the milestones: 5.2.0, 5.3.0 Aug 22, 2023
@jkomyno jkomyno requested a review from tomhoule August 28, 2023 12:45
Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

Since we're going in the direction (DX initiative) of keeping the exact same connectors and relying on matching behaviour from the different driver adapters, we can do this. Approved 👍

@jkomyno jkomyno merged commit c021fae into main Aug 29, 2023
54 checks passed
@jkomyno jkomyno deleted the feat/allow-std-provider-names branch August 29, 2023 15:05
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.

None yet

3 participants