cerebruminc / yates

Use Postgres RLS with Prisma
50 stars 3 forks source link

[discussion] Reduce startup time #90

Open jason-curtis opened 6 months ago

jason-curtis commented 6 months ago

Spinning this discussion off from https://github.com/prisma/prisma/issues/12735#issuecomment-2001893619 .

Yates provides a straightforward way to add RLS to Prisma-Postgres setups, but there is overhead at startup:

This compute and check process adds some overhead to the server startup time, on our production system at Cerebrum, we have 148* default abilities and 133 custom abilities and it takes ~2.5s to get through the setup process for Yates.

In serverless configurations, 2.5s on startup of each compute instance can be substantial. Are there ways that we can reduce the startup time? Maybe by moving the DB role validation to a script that can be run asyncronously as part of a build/deploy process?

LucianBuzzo commented 6 months ago

@jason-curtis I'm open to offering DB setup as a separate process, allowing a migration step to be run separately. Something like npx yates migrate, and a flag in the setup function that skips the migration code. I would also like to see if the setup code can be optimized for reduced startup time, as I think there is low hanging fruit that will improve performance without modifying the interface. In a serverless architecture, what's your threshold for an "acceptable" startup time? Obviously the faster the better, but having a target would be good.

jason-curtis commented 6 months ago

Something like npx yates migrate, and a flag in the setup function that skips the migration code.

Love this! Maybe npx yates sync? Looks like createRoles is already organized in a way that would make this easy to factor out https://github.com/cerebruminc/yates/blob/master/src/index.ts#L558 👍

I would also like to see if the setup code can be optimized for reduced startup time, as I think there is low hanging fruit that will improve performance without modifying the interface.

That would be great! Like maybe only generate the roles that are actually used, rather than all of the generic CRUD roles?

target

Hmm, off the cuff, 100ms would be an easy yes. I'm assuming here that the vast majority of the startup time is mostly due to interaction with the DB, and that the npx yates migrate approach would cut it way down. It would be good to know how much the startup time is compute (/might scale with the number of models) vs. db time.

LucianBuzzo commented 5 months ago

@jason-curtis With the latest v3.5.0 release, subsequent startup time is ~100ms for our big schema (309 abilities), and I think there is still some more performance that can be squeezed out of the system. Initial bootstrap is still about the same time (assuming you have never run Yates before), but I've been able to make a lot of improvements in how abilities are checked to see if we need to update/create them. This means that once the initial abilities are created, the startup time is really fast. If you make changes to abilities later down the line, only the affected abilities will be changed in the DB, meaning that you won't see much slowdown there either.

LucianBuzzo commented 5 months ago

That would be great! Like maybe only generate the roles that are actually used, rather than all of the generic CRUD roles?

This is an area where we can get some more speedup I think

hongkongkiwi commented 5 months ago

I would really love to have support for creating policies in a migration step instead. One of the things I like about Prisma is that it generates a client statically that can be pulled in based on the schema.

Doing live DB modifications at startup seems to bypass that logic completely and could make migrations more difficult.

It would be great to have an option so that yates can just perform checks based on the schema but not actually perform any updates. Then be able to have a separate migrate step that can be run.

sebmellen commented 4 months ago

It would be great to have an option so that yates can just perform checks based on the schema but not actually perform any updates. Then be able to have a separate migrate step that can be run.

@hongkongkiwi this is not something that we currently need for our use-case, so we won't be able to dedicate time to it. However, if you can open a PR for this, we'd love to review it and merge it in. Thank you!