fix: Start listening after schema cache load#4880
fix: Start listening after schema cache load#4880mkleczek wants to merge 1 commit intoPostgREST:mainfrom
Conversation
mkleczek
force-pushed
the
push-tynrmqwlwwus
branch
from
ca17636 to
5bb48c5
Compare
| - Restore Listener query shape so it can be found in pg_stat_activity by @mkleczek in #4857 #4859 | ||
| - The LISTEN channel now automatically recovers when it stops working due to a PostgreSQL bug @laurenceisla in #3147 | ||
| - Fix misleading "Functions" name on schema cache summary in startup logs by @taimoorzaeem in #4821 | ||
| - Do not listen on main socket until schema cache is loaded by @mkleczek in #4880 |
There was a problem hiding this comment.
Should also have a ### Changed entry, let's put it as breaking change to be extra safe.
| status | not isMainAppReachable = HTTP.status500 | ||
| | isPending = HTTP.status503 | ||
| status | isPending = HTTP.status503 | ||
| | not isMainAppReachable = HTTP.status500 |
There was a problem hiding this comment.
Looks like this change is not needed? Results in same behavior?
There was a problem hiding this comment.
Looks like this change is not needed? Results in same behavior?
I think it is needed: we want to return 503 during schema cache loading when main app is still not reachable.
| mainSocket <- initServerSocket conf | ||
| atomicWriteIORef mainSocketRef $ Just mainSocket |
There was a problem hiding this comment.
Q: Can this change help lift the limitation we have with the --ready check? (#4269 (comment)) i.e. can the admin server know the address that is resolved for the API server when using special hosts? (like !4)
|
This should close #4129. Previous discussion on the motivation of the change on #4703 (comment). |
|
@mkleczek As per #4703 (comment), this would clearly benefit the case of But let's consider the scenario of a single instance managed by systemd behind a proxy:
With this change, now we'll not respond and clients will get a So under this scenario, it looks like this new behavior is worse? Thinking more what we need is zero-downtime restarts, which I guess is easier under this new behavior since we could rely on systemd socket activation? |
Not really. From the point of view of the client there is not much gain from these |
| response = postgrest.session.get("/projects") | ||
| assert response.status_code == 503 | ||
| with pytest.raises(requests.ConnectionError): | ||
| postgrest.session.get("/projects") |
There was a problem hiding this comment.
With this change, the test description does not fit anymore.
But anyway, with the change in this PR, the test case does not make sense anymore at all, I think. A connection error does not even make it to PostgREST, so testing its non-effect is useless. I'd remove that test-case as well, if we decide to go on with this PR.
There was a problem hiding this comment.
Not really sure about that - we want to show that schema cache is loaded once during startup.
We also verify the behavior during schema cache load (before it was 503 now it is connection error).
I also added another assertion that once schema cache is loaded we return 200 from /projects request.
Test description updated - it was indeed wrong.
This change ensures PostgREST starts listening on a server socket only after it loaded the schema cache and is ready to handle requests. It is no longer going to return 503 errors during startup until the schema cache is loaded.
mkleczek
force-pushed
the
push-tynrmqwlwwus
branch
from
5bb48c5 to
99554cb
Compare
3 participants
DISCLAIMER:
This commit was authored entirely by a human without the assistance of LLMs.
This change ensures PostgREST starts listening on a server socket only after it loaded the schema cache and is ready to handle requests. It is no longer going to return 503 errors during startup until the schema cache is loaded.