graphile / crystal-pre-merge

Repository moved
https://github.com/graphile/crystal
39 stars 8 forks source link

Alternative node-postgres adaptor that uses `SET` rather than `SET LOCAL` #62

Open benjie opened 2 years ago

benjie commented 2 years ago

Our set_config(..., ..., true) is great because it unsets at the end of a transaction; however we currently do:

begin;
select set_config(el->>0, el->>1, true) from json_array_elements($1::json) el;
...
commit;

around each withPgClient call that uses settings.

We could instead use global settings:

select set_config(el->>0, el->>1, false) from json_array_elements($1::json) el;
...
reset all;

This would save us a single roundtrip.

We could save another roundtrip by removing the RESET ALL but a) we'd need to track every setting that had been made and be sure to clear those that the new $1 doesn't set, and b) this would prevent us sharing a PG pool between PostGraphile and another service (e.g. Worker) because the settings would leak between them. If we're happy with those tradeoffs, we could actually benefit more - it's reasonably likely that the same user will trigger another withPgClient call, maybe even just a tick later! We could see if we have a client for the same claims, and if so we could just reuse the client again:

-- withPgClient for user1
reset all;
select set_config(el->>0, el->>1, false) from json_array_elements($1::json) el;
... -- ACTUAL QUERIES WE NEED HERE

-- withPgClient for user1
... -- ACTUAL QUERIES WE NEED HERE

-- withPgClient for user1
... -- ACTUAL QUERIES WE NEED HERE

-- withPgClient for user2
reset all;
select set_config(el->>0, el->>1, false) from json_array_elements($1::json) el;
... -- ACTUAL QUERIES WE NEED HERE

Note how there's the initial 2 roundtrips, but then future calls to withPgClient don't need any extra roundtrips until a new set of claims is needed.

benjie commented 2 years ago

This was inspired by a conversation with @ericreisn on Twitter:

https://twitter.com/ericreisn/status/1563298359561289730

And by the amount of transaction management in this test:

https://github.com/benjie/postgraphile-private/blob/4ccf01f0b29b0984fd8ea62de31424e68d530220/postgraphile/postgraphile/__tests__/queries/v4/rbac.basic.sql