fix: implement connection tracking in metrics by mkleczek · Pull Request #4672 · PostgREST/postgrest

mkleczek · 2026-02-25T18:39:21Z

DISCLAIMER:
This commit was authored entirely by a human without the assistance of LLMs.

Right now metrics observation handler does not track database connections but updates a single Gauge based on HasqlPoolObs events.

This is problematic because Hasql pool reports various connection events in various states that make it impossible to predict the state change from the received event. The connection state machine is not simple and to precisely report the number of connections in various states, it is necessary to track their lifecycles.

Fixes #4622

steve-chavez · 2026-02-25T22:57:17Z

Fixes #4622

Fix should have an entry on the CHANGELOG

steve-chavez · 2026-04-08T18:23:33Z

        -- (there should be none but we need to verify that)
        threadDelay $ 1 * sec

+  it "Should track in use connections" $ do


Q: Does this reproduce the problem on #4622? If not, could we add a new repro test or is that too complicated?

Q: Does this reproduce the problem on #4622? If not, could we add a new repro test or is that too complicated?

As far as I understand it might be possible to reproduce once we have toxiproxy in place. The issue happens when there connectivity issues.

For that perhaps you could restart #4674? Should be easier I think now that we don't have slocat.

I also wonder if there's a simpler/focused way by just killing the connection like mentioned on #1766 (comment)

For a case like this, I wouldn't block on adding an exactly-reproducing test of the original issue: We really changed the entire approach to calculating this metric, from simply reacting to certain events to tracking each connection. I find it's unlikely that someone accidentally introduces a regression here.

That being said, I think we absolutely need some commentary in Metrics.hs about why we are doing the connection tracking and which problems this prevents. With such commentary, someone looking at this later will know what to look for when rewriting the whole thing again.

For that perhaps you could restart #4674? Should be easier I think now that we don't have slocat.

See #4833 #4836

That being said, I think we absolutely need some commentary in Metrics.hs

Done (see inline comment in Metrics.observationMetrics). I am not 100% sure if the comment is in the right place so happy to move it if you think it is necessary.

For a case like this, I wouldn't block on adding an exactly-reproducing test of the original issue: We really changed the entire approach to calculating this metric, from simply reacting to certain events to tracking each connection.

Yeah, I was thinking we should take this chance to add the required infra to test otherwise we'll end up with a "test debt" (like on #1766).

But I guess we still have #4670 as another chance to test an unreliable network.

So I won't block here if you approve here @wolfgangwalther

We really changed the entire approach to calculating this metric, from simply reacting to certain events to tracking each connection. I find it's unlikely that someone accidentally introduces a regression here.

In light of #4672 (comment), I think there is a chance that we'll rewrite this whole thing again when we have hasql-pool vendored. Thus, I agree with:

Yeah, I was thinking we should take this chance to add the required infra to test otherwise we'll end up with a "test debt" (like on #1766).

steve-chavez · 2026-04-08T18:24:48Z

@mkleczek Since this is a fix, let's change the commit, PR title and add a fix entry in the CHANGELOG.

steve-chavez · 2026-04-14T16:01:15Z

@mkleczek Could you please address the feedback here? I've repeated the same twice now 😩

mkleczek · 2026-04-14T19:44:55Z

@mkleczek Could you please address the feedback here? I've repeated the same twice now 😩

Done

wolfgangwalther · 2026-04-21T08:52:58Z

        -- (there should be none but we need to verify that)
        threadDelay $ 1 * sec

+  it "Should track in use connections" $ do


For a case like this, I wouldn't block on adding an exactly-reproducing test of the original issue: We really changed the entire approach to calculating this metric, from simply reacting to certain events to tracking each connection. I find it's unlikely that someone accidentally introduces a regression here.

That being said, I think we absolutely need some commentary in Metrics.hs about why we are doing the connection tracking and which problems this prevents. With such commentary, someone looking at this later will know what to look for when rewriting the whole thing again.

wolfgangwalther · 2026-04-21T20:09:16Z

+  -- Handle pool observations with connection tracking
+  -- this is necessary because it is not possible
+  -- to accurately maintain open/in use conneciton counts
+  -- statelessly based only on pool observation events.
+  -- The reason is that hasql-pool emits TerminatedConnectionStatus
+  -- both for connections successfully established and failed when connecting.
+  -- When receiving TerminatedConnectionStatus we have to find out
+  -- if we can decrement established connection count. To do that we have to track
+  -- established connections.


Thanks for the comment - this helps a lot.

Q: Would this reasoning be different once we vendor hasql, including hasql-pool, and could change the respective events? Would this make the fix here simpler, because we could keep using the same events?

Thanks for the comment - this helps a lot.

Q: Would this reasoning be different once we vendor hasql, including hasql-pool, and could change the respective events? Would this make the fix here simpler, because we could keep using the same events?

Yes.
Quite frankly - hasql-pool should expose the metrics itself because it already tracks this information anyway. Publishing events is the wrong abstraction here.

EDIT: I mean hasql-pool should expose functions providing the actual numbers. Exposing them as Prometheus metrics should be done similar to how ConnTrack is exposed in this PR.

Right now metrics observation handler does not track database connections but updates a single Gauge based on HasqlPoolObs events. This is problematic because Hasql pool reports various connection events in multiple phases. The connection state machine is not simple and to precisely report the number of connections in various states, it is necessary to track their lifecycles. This change adds a ConnTrack data structure and logic to track database connections lifecycles. At the moment it supports "connected" and "inUse" connection counts precisely. The "pgrst_db_pool_available" metric is implemented on top of ConnTrack instead of a simple Gauge.

This was referenced Feb 25, 2026

refactor(test): provide means to validate metrics and observations #4671

Merged

fix: Limit concurrent schema cache loads #4643

Open

steve-chavez reviewed Feb 25, 2026

View reviewed changes

Comment thread src/PostgREST/Metrics.hs Outdated

steve-chavez reviewed Feb 25, 2026

View reviewed changes

Comment thread src/PostgREST/Metrics.hs

mkleczek force-pushed the refactor/connection-tracking branch 5 times, most recently from 0f475e3 to 592e5a3 Compare March 10, 2026 14:53

mkleczek force-pushed the refactor/connection-tracking branch 2 times, most recently from 1f3b4bc to bce1f6e Compare March 13, 2026 06:49

mkleczek force-pushed the refactor/connection-tracking branch 8 times, most recently from 7383b59 to 9b3c004 Compare April 2, 2026 14:45

mkleczek force-pushed the refactor/connection-tracking branch from 9b3c004 to 6af7b30 Compare April 8, 2026 12:33

steve-chavez reviewed Apr 8, 2026

View reviewed changes

mkleczek force-pushed the refactor/connection-tracking branch 5 times, most recently from 27d9a1d to e438eed Compare April 14, 2026 07:02

mkleczek force-pushed the refactor/connection-tracking branch from e438eed to 41b439e Compare April 14, 2026 19:47

mkleczek requested a review from steve-chavez April 14, 2026 19:48

mkleczek changed the title ~~refactor: implement connection tracking in metrics~~ fix: implement connection tracking in metrics Apr 14, 2026

mkleczek force-pushed the refactor/connection-tracking branch 2 times, most recently from 3d14884 to decc1a0 Compare April 15, 2026 08:02

wolfgangwalther reviewed Apr 19, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

mkleczek force-pushed the refactor/connection-tracking branch 2 times, most recently from d541ff0 to c351c30 Compare April 20, 2026 08:56

steve-chavez reviewed Apr 20, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

mkleczek force-pushed the refactor/connection-tracking branch 2 times, most recently from c3aa110 to 43e0d89 Compare April 21, 2026 04:54

mkleczek requested review from steve-chavez and wolfgangwalther April 21, 2026 05:45

wolfgangwalther reviewed Apr 21, 2026

View reviewed changes

mkleczek force-pushed the refactor/connection-tracking branch 2 times, most recently from b0079b3 to b66bc15 Compare April 21, 2026 17:28

wolfgangwalther reviewed Apr 21, 2026

View reviewed changes

mkleczek force-pushed the refactor/connection-tracking branch 8 times, most recently from 0563abd to 423afeb Compare April 28, 2026 12:26

mkleczek mentioned this pull request Apr 28, 2026

test: Add reproducer of 4622 (negative connection count) #4855

Open

mkleczek force-pushed the refactor/connection-tracking branch from 423afeb to c18acfa Compare April 29, 2026 10:03

Uh oh!

Conversation

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

Uh oh!

Uh oh!

steve-chavez commented Feb 25, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-chavez commented Apr 8, 2026

Uh oh!

steve-chavez commented Apr 14, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mkleczek commented Feb 25, 2026 •

edited

Loading

mkleczek Apr 21, 2026 •

edited

Loading

mkleczek commented Apr 14, 2026 •

edited

Loading

mkleczek Apr 21, 2026 •

edited

Loading