perf: Purge JWT cache asynchronously in a separate thread

[mkleczek]

A follow-up to the JWT cache purging fix - looks like we still left expensive operation on a hot path affecting response times for request triggering cache miss.

A simple fix is to purge in a separate thread started upon a cache miss.

[steve-chavez]

It does look like throughput increases from the "load test summary" on https://github.com/PostgREST/postgrest/actions/runs/13062249392?pr=3889.

But doing forkIO on every request makes me uncomfortable, could this lead to more CPU consumption?

Ideally we would have a background thread that does this periodically and is outside of the hot path.

[mkleczek]

It does look like throughput increases from the "load test summary" on https://github.com/PostgREST/postgrest/actions/runs/13062249392?pr=3889.

But doing forkIO on every request makes me uncomfortable, could this lead to more CPU consumption?

It is no worse than synchronous purge but has better latency.

Ideally we would have a background thread that does this periodically and is outside of the hot path.

Periodical purge requires tuning (ie. adding a configuration parameter) to make sure cleanup happens often enough but not too often. Triggering purging upon cache miss solves this issue.
Current implementation is based on assumption that cache miss is rare (otherwise it is better to turn off JWT caching altogether) and does not cause too much stress.

[taimoorzaeem]

@mkleczek Could you perhaps wait for the next major if we switch to a different cache implementation, discussed in 3802#issuecomment?

[mkleczek]

It does look like throughput increases from the "load test summary" on https://github.com/PostgREST/postgrest/actions/runs/13062249392?pr=3889.

But doing forkIO on every request makes me uncomfortable, could this lead to more CPU consumption?

Ideally we would have a background thread that does this periodically and is outside of the hot path.

@steve-chavez @taimoorzaeem
How about we make sure there is at most one purging thread running concurrently?

It required some more changes and the patch is no longer one-liner but should address your concerns?

[taimoorzaeem]

@mkleczek Great attempt for this change. It might take us some time to decide where to go with this. Meanwhile, if you'd like this change urgently, just to save you some time, you can see how to build PostgREST from source here. That's the beauty of open source 🎉.

[steve-chavez]

@steve-chavez @taimoorzaeem
How about we make sure there is at most one purging thread running concurrently?

That sounds better, yes. If you want to do it, please go ahead. Although note that as @taimoorzaeem mentioned, we're going to switch to an LRU cache soon, if there's considerable complexitiy/effort in adding the purging thread, maybe it would be better to wait for v13.

[mkleczek]

@steve-chavez @taimoorzaeem
How about we make sure there is at most one purging thread running concurrently?

That sounds better, yes. If you want to do it, please go ahead. Although note that as @taimoorzaeem mentioned, we're going to switch to an LRU cache soon, if there's considerable complexitiy/effort in adding the purging thread, maybe it would be better to wait for v13.

It's already done - could you review the updated PR?

Still some linting issues reported - I could use some advice on how to find the reports or run it locally.

Not sure about the coverage of the patch - there is an uncovered path of skipping starting a new thread when one is running - not sure how to test it and if it is worth doing.

I would be really grateful if it could be merged as building patched version from source would be too much hassle for my organisation...

[steve-chavez]

Still some linting issues reported - I could use some advice on how to find the reports or run it locally.

You can run it locally with:

$ nix-shell
$ postgrest-lint && postgrest-style

[mkleczek]

Still some linting issues reported - I could use some advice on how to find the reports or run it locally.

You can run it locally with:

$ nix-shell
$ postgrest-lint && postgrest-style

Thanks @steve-chavez. Done.

I've also moved JwtCacheState to Auth/Types.hs introduced by @taimoorzaeem in the meantime.

Do you think this is mergeable now?