refactor: App control flow refactoring by mkleczek · Pull Request #4969 · PostgREST/postgrest

mkleczek · 2026-06-01T04:56:11Z

Make MainTx.mainTx and Query.mainQuery receive DbPlan instead of ActionPlan so that they do not have to handle unrelated NoDb cases.
Removed "NoDbResult" constructor from "DbResult" datatype and factored out noDbActionResponse from actionResponse to clearly separate "DbResult" and "InfoPlan" handling.

Server timing is now handled in a way that does not pollute the control flow. New MonadTiming typeclass with two implementations was introduced - one for StateT Timings m that times operations and remembers timing as state, and another for IdentityT m that does not do any measurements. The selection of monad is guarded by configServerTimingsEnabled. GHC with -O2 is capable to specialize code so that withTiming calls are completely discarded for disabled case.

Moved authentication back to App.postgrestResponse so that all steps that require timing are encapsulated. In #4884 it was moved level up the call chain which, while achieving the goal of removing auth middleware, made the control flow complicated and difficult to follow. Thanks to introduction of WriterT based handling of auth role in #4968, it is now possible to return to have all request handling pipeline steps in App.postgrestResponse.

This PR is a follow up on #4968.

wolfgangwalther · 2026-06-05T21:02:30Z

This PR is a follow up on #4969 and is structured into multiple commits that introduce smaller changes.

I'm confused: This is a self-reference and there is only one commit in here.

mkleczek · 2026-06-06T04:27:43Z

I'm confused: This is a self-reference and there is only one commit in here.

Yes, sorry - during reworking of this PR the description got outdated.

steve-chavez · 2026-06-08T16:18:24Z

Should this be ready for review?

wolfgangwalther · 2026-06-12T07:45:47Z

The coverage job says:

postgrest-15-inplace: src/PostgREST/Auth.hs:30:1: getAuthResult

Found dead code: Check file list above.

mkleczek · 2026-06-12T14:07:05Z

The coverage job says:

postgrest-15-inplace: src/PostgREST/Auth.hs:30:1: getAuthResult

Found dead code: Check file list above.

I'm aware of this but hesitating what to do.

The situation is that Auth module got very erratic and inlining getAuthResult into postgrestResponse (2 lines) left it unused. I don't think there is a need to maintain so "thin" Auth module.
I consider two approaches:

Removing Auth module, leaving Auth.Jwt and Auth.JwtCache
Removing Auth.Jwt module and splitting the code in it between Auth and Auth.JwtCache

The first one is simpler and leaves all functions related to JWT parsing and claims validation in Auth.JWT. That means that two modules depend on it: App and Auth.JwtCache. The first one uses claims validation functions and the second - token parsing and verification (but not claims validation).

The second one splits JWT handling into two separate groups: token parsing and signature verification and claims validation. The first group would land in Auth.JwtCache as these functions are used in JWT cache. The second group would land in Auth module (as it validates claims after looking them up in the JWT cache).

The second option is nice because it clearly allows encapsulating different responsibilities and thanks to it - GHC can easily inline respective functions easily, because the functions stay internal to the modules they are used in.

@wolfgangwalther @steve-chavez @taimoorzaeem WDYT?

steve-chavez · 2026-06-12T16:04:47Z

Looks like there are performance improvements from this refactoring.

Nice! I can confirm that by reading the loadtest results. Also now we do have a reduction in LOCs! 🎉

@wolfgangwalther Any opposition towards changing the loadtest columns from v14.13|head|main to main|head|v14.13? It's much easier to read the change from left to right than right to left.

The second option is nice because it clearly allows encapsulating different responsibilities and thanks to it - GHC can easily inline respective functions easily, because the functions stay internal to the modules they are used in.

@mkleczek I'd go with the second option if the inlining does bring us more performance.

wolfgangwalther · 2026-06-12T16:33:39Z

Any opposition towards changing the loadtest columns from v14.13|head|main to main|head|v14.13? It's much easier to read the change from left to right than right to left.

Not opposed if you can make it work. I never liked it that way, but couldn't figure out how to affect the order of the output.

steve-chavez · 2026-06-12T21:22:11Z

Q: Why this shows on the loadtest:

404 GET /actoxs?actor=eq.1 48.9 48.5 44.6 ❌ 8.7 %
https://github.com/PostgREST/postgrest/actions/runs/27436623108?pr=4969

Did we really lose perf for that case?

wolfgangwalther · 2026-06-12T21:30:45Z

Q: Why this shows on the loadtest:

404 GET /actoxs?actor=eq.1 48.9 48.5 44.6 ❌ 8.7 %
https://github.com/PostgREST/postgrest/actions/runs/27436623108?pr=4969

Did we really lose perf for that case?

This is for the 90th percentile. That data is inherently more noisy. There is no reason to assume that there is a performance loss specific for that case, which is only visible on that percentile.

wolfgangwalther · 2026-06-13T17:15:09Z

Has this refactor become a performance improvement now? Can these two be separated in any way or is that not easily possible?

mkleczek · 2026-06-14T06:01:39Z

Has this refactor become a performance improvement now? Can these two be separated in any way or is that not easily possible?

I wouldn't consider it as performance improvement PR. Any performance gains are a byproduct of polishing the PR - original one introduced performance regressions and after updating the code we have (modest) perf improvements.

But the goal of this PR stays the same: make the request handling control flow explicit and easier to follow.

I've split the changes into three separate commits to make the change history cleaner.

wolfgangwalther · 2026-06-14T08:10:19Z

original one introduced performance regressions

Ah, I missed that. Right, agree then.

taimoorzaeem · 2026-06-15T18:46:01Z

+class MonadTiming m where
+  withTiming' :: forall name a. KnownSymbol name => m a -> m a
+  renderTimings :: m [HTTP.Header]
+
+withTiming :: forall s m a. (KnownSymbol s, MonadTiming m) => m a -> m a
+withTiming = withTiming' @m @s
+
+instance Monad m => MonadTiming (IdentityT m) where
+  withTiming' = identity
+  renderTimings = pure mempty
+
+instance MonadIO m => MonadTiming (StateT [Timing] m) where
+  withTiming' @name f = do
+          (t, result) <- timeItT f
+          modify ((BS.pack $ symbolVal (Proxy @name), t) :)
+          pure result
+  {-# INLINE withTiming' #-}
+  renderTimings = (foldMap (pure . serverTimingHeader) . nonEmpty) . reverse <$> get


Can we add a comment here to explain why these are needed? You have explained it the commit message, but it would be good to add that here too, I think.

Guess we should remove this INLINE here too?

Guess we should remove this INLINE here too?

I'm afraid not - without explicit INLINE GHC did not inline withTiming' for some reason, which in turn stopped it from performing more important optimizations: constant folding of symbolVal handling.

Will check again since I verified it before #5008.

wolfgangwalther · 2026-06-16T19:36:08Z

Just a note: I looked at this multiple times, but this exceeds my mental capacity - there are too many new concepts in here, still unknown to me, that I have to learn each time, and looking at this at once is hard. I do understand the overall idea to be roughly that:

instead of going "is timing enabled?" at each step along the way, we do it once.
we have two separate code-paths one with timing, one without.
to avoid copying the whole thing mostly 1:1 in two separate code-paths and having to maintain that twice, we use a polymorphic function and let GHC specialize two functions from it. GHC does the copying, we don't have to.
Both cases are faster that way, because they don't need to repeatedly jump around these conditions.

Ok, cool. And then come the language extensions... :)

mkleczek · 2026-06-16T19:47:38Z

Just a note: I looked at this multiple times, but this exceeds my mental capacity - there are too many new concepts in here, still unknown to me, that I have to learn each time, and looking at this at once is hard. I do understand the overall idea to be roughly that:

instead of going "is timing enabled?" at each step along the way, we do it once.

[...]
Well said, but...

It's not only about specialization/performance.
First of all I wanted to move timing "to the background" of main control flow, eliminate visual noise that way and have:

authResult <- withTiming @"auth" ...
...
apiReq <- withTiming @"request" ...
plan <- withTiming @"plan" ...
toWaiResponse =<< case plan of
  ...

Ok, cool. And then come the language extensions... :)

They are really to enable passing timing names as type level literals. Thanks to it in the "no-timing" case we can have no-op withTiming = identity (ie. there is no runtime argument passed to withTiming).

Make MainTx.mainTx and Query.mainQuery receive DbPlan instead of ActionPlan so that they do not have to handle unrelated NoDb cases. Removed "NoDbResult" constructor from "DbResult" datatype and factored out noDbActionResponse from actionResponse to clearly separate "DbResult" and "InfoPlan" handling. Server timing is now handled in a way that does not pollute the control flow. New MonadTiming typeclass with two implementations was introduced - one for `StateT Timings m` that times operations and remembers timing as state, and another for `IdentityT m` that does not do any measurements. The selection of monad is guarded by `configServerTimingsEnabled`. GHC with `-O2` is capable to specialize code so that `withTiming` calls are completely discarded for disabled case. Moved authentication back to App.postgrestResponse so that all steps that require timing are encapsulated. In 5359769 it was moved level up the call chain which, while achieving the goal of removing auth middleware, made the control flow complicated and difficult to follow. Thanks to introduction of WriterT based handling of auth role in d663421, it is now possible to return to have all request handling pipeline steps in App.postgrestResponse.

This change splits JWT handling into two separate groups: token parsing/signature verification and claims validation. The first group was moved to Auth.JwtCache as these functions are used in JWT cache. The second group was moved to Auth module (as it validates claims after looking them up in the JWT cache). Auth.JWT module was removed as no longer needed. Thanks to this change JWT handling became more cohesive and made it possible for GHC to aggressively inline and specialize JWT related functions.

getAuthResult suggests authentication result is already available and it just needs to be read. But in reality this function performs full JWT validation (ie. parsing, signature verification, claims validation). Renaming it to authenticate so that the name is no longer misleading.

taimoorzaeem · 2026-06-21T08:45:55Z

+class MonadTiming m where
+  withTiming' :: forall name a. KnownSymbol name => m a -> m a
+  renderTimings :: m [HTTP.Header]
+
+withTiming :: forall s m a. (KnownSymbol s, MonadTiming m) => m a -> m a
+withTiming = withTiming' @m @s
+
+-- Dummy MonadTiming instance.
+-- Used when configServerTimingEnabled is False
+-- GHC specialization will optimize away all calls to withTiming and to renderTimings,
+-- hence hot path does not execute any unnecessary code and does not cause unnecessary allocations.
+instance Monad m => MonadTiming (IdentityT m) where
+  withTiming' = identity
+  renderTimings = pure mempty
+
+-- MonadTiming instance saving timings in StateT [Timing].
+-- Used when configServerTimingEnabled is True
+instance MonadIO m => MonadTiming (StateT [Timing] m) where
+  withTiming' @name f = do


How about we move this to TimeIt.hs module? It seems like it is polluting this App.hs. WDYT?

mkleczek force-pushed the refactor/app-control-flow branch 22 times, most recently from 9350465 to 42da614 Compare June 2, 2026 07:13

taimoorzaeem self-requested a review June 8, 2026 10:03

mkleczek force-pushed the refactor/app-control-flow branch 2 times, most recently from b6d2c8b to e238fc2 Compare June 8, 2026 20:06

mkleczek marked this pull request as ready for review June 8, 2026 20:08

steve-chavez reviewed Jun 8, 2026

View reviewed changes

Comment thread src/PostgREST/Plan.hs

mkleczek force-pushed the refactor/app-control-flow branch 3 times, most recently from 00f2217 to 9732728 Compare June 12, 2026 05:04

mkleczek force-pushed the refactor/app-control-flow branch from 9732728 to c060e88 Compare June 12, 2026 18:58

mkleczek force-pushed the refactor/app-control-flow branch from c060e88 to e3b825c Compare June 13, 2026 04:45

mkleczek force-pushed the refactor/app-control-flow branch from 4e04f96 to 012b811 Compare June 15, 2026 14:55

mkleczek requested a review from taimoorzaeem June 15, 2026 18:27

taimoorzaeem reviewed Jun 15, 2026

View reviewed changes

mkleczek force-pushed the refactor/app-control-flow branch 2 times, most recently from f1ea79d to 38e7b57 Compare June 15, 2026 19:21

taimoorzaeem reviewed Jun 15, 2026

View reviewed changes

Comment thread src/PostgREST/Auth.hs Outdated

mkleczek force-pushed the refactor/app-control-flow branch from 38e7b57 to ab8310a Compare June 15, 2026 19:59

mkleczek force-pushed the refactor/app-control-flow branch from ab8310a to 44f1ac6 Compare June 17, 2026 14:55

mkleczek added 3 commits June 18, 2026 07:45

mkleczek force-pushed the refactor/app-control-flow branch from 44f1ac6 to 82bdbb0 Compare June 18, 2026 05:46

mkleczek marked this pull request as draft June 18, 2026 06:24

taimoorzaeem reviewed Jun 21, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

mkleczek commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Jun 5, 2026

Uh oh!

mkleczek commented Jun 6, 2026

Uh oh!

steve-chavez commented Jun 8, 2026

Uh oh!

Uh oh!

wolfgangwalther commented Jun 12, 2026

Uh oh!

mkleczek commented Jun 12, 2026

Uh oh!

steve-chavez commented Jun 12, 2026

Uh oh!

wolfgangwalther commented Jun 12, 2026

Uh oh!

steve-chavez commented Jun 12, 2026

Uh oh!

wolfgangwalther commented Jun 12, 2026

Uh oh!

wolfgangwalther commented Jun 13, 2026

Uh oh!

mkleczek commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Jun 14, 2026

Uh oh!

taimoorzaeem Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

mkleczek Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

steve-chavez Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

mkleczek Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wolfgangwalther commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkleczek commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taimoorzaeem Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

mkleczek commented Jun 1, 2026 •

edited

Loading

mkleczek commented Jun 14, 2026 •

edited

Loading

wolfgangwalther commented Jun 16, 2026 •

edited

Loading

mkleczek commented Jun 16, 2026 •

edited

Loading