fix: Start listening after schema cache load

[mkleczek]

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.

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

[steve-chavez]

Previous discussion on the motivation of the change on #4703 (comment).

[steve-chavez]

@mkleczek As per #4703 (comment), this would clearly benefit the case of SO_REUSEPORT given 2 PostgREST instances running.

But let's consider the scenario of a single instance managed by systemd behind a proxy:

• The service restarts (for any reason, could be manual restart because somehow the schema cache failed reloading).
• Right now our startup is fast (milliseconds) and we start responding with 503s. During this time clients will get the 503s with a clear error message that says we're "Retrying.." plus a Retry-After header.

With this change, now we'll not respond and clients will get a connection refused with no error message. And this state can last multiple seconds now that we wait for the scache to load.

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?

[mkleczek]

So under this scenario, it looks like this new behavior is worse?

Not really.

From the point of view of the client there is not much gain from these 503 errors comparing to some connect timeout or similar. The client has to handle connection issues anyway because there are many more cases when they can happen (for example the whole server might have crashed). In case of reverse proxies in front of PostgREST (ie. always) - the client will get some 50x anyway.
The only reasonable way for the client to handle network issues is to retry. Well behaving clients will use some exponential backoff with jitter retry policy to avoid overwhelming freshly started server (ie. to avoid thundering herd).
Retry-After is not very useful because it is not reliable. What's worse: if all clients retry according to this header then... boom - thundering herd - I would even say Retry-After is more harmful than good.

[steve-chavez]

What's worse: if all clients retry according to this header then... boom - thundering herd - I would even say Retry-After is more harmful than good.

Thought about removing the Retry-After, but its docs say:

[...] Retry-After indicates the minimum time that the user agent is asked to wait

So it's a minimum, not exact time. I think it should be fine to be clear about this on the docs and recommend jitter.

[steve-chavez]

@mkleczek The direction here is good, make sure to address the comments and then we can merge this.

[mkleczek]

I am marking this PR as draft to address concerns related to handling schema cache loading errors during start-up.

It seems the right course of action cannot be any of these two extremes:

• always return 503 during schema cache loading on startup
• start listening only after successful schema cache load

The first one forces clients to handle normal conditions as errors.
The second one makes the clients unaware of errors that might happen during schema cache loading which makes diagnostics more difficult.

It seems like the best (ultimate?) startup sequence should be:

1. Start admin server.
2. Try to load schema cache once.
3. Start listening on main socket
4. If there was an error in 2, enter retry loop.

That way we achieve both:

• happy path (ie. successful startup sequence) does not cause any error responses
• errors are properly reported to clients

The above requires wider refactoring - today the whole schema cache loading loop is implemented in a single function without any means to introspect the state of the loading process. Clients can only trigger asynchronous schema load and wait for it to finish.
It makes it related to #4856, which in turn is a prerequisite to implement loading the schema cache using listener connection to fix #4842.

@steve-chavez WDYT?

[wolfgangwalther]

• Start admin server.
• Try to load schema cache once.
• Start listening on main socket
• If there was an error in 2, enter retry loop.

I wrote up two different proposals but threw them away, because I always came to the conclusion that this is the sensible thing to do.

So 👍

[steve-chavez]

It seems like the best (ultimate?) startup sequence should be:

Looks much better. Also 👍 from me.

[laurenceisla]

2. Try to load schema cache once.
3. Start listening on main socket

So between these two steps, we'd still return the connection error, however after that we'd retry and get the 503. I agree with this.

@steve-chavez Not sure if it was discussed elsewhere, but this would mean that the proposal to wait until the schema cache is loaded on startup is no longer desired, right?

[steve-chavez]

@laurenceisla The waiting is being discussed on #4873 (comment). #4129 won't be solved here.

[mkleczek]

It seems like the best (ultimate?) startup sequence should be:

1. Start admin server.
2. Try to load schema cache once.
3. Start listening on main socket
4. If there was an error in 2, enter retry loop.

That way we achieve both:

• happy path (ie. successful startup sequence) does not cause any error responses
• errors are properly reported to clients

Updated the code to implemented the above.

[steve-chavez]

It seems like the best (ultimate?) startup sequence should be:

1. Start admin server.
2. Try to load schema cache once.
3. Start listening on main socket
4. If there was an error in 2, enter retry loop.

That way we achieve both:

• happy path (ie. successful startup sequence) does not cause any error responses
• errors are properly reported to clients
  The first one forces clients to handle normal conditions as errors.

@mkleczek While the happy path is devoid of errors, the "usual path" always has some db connections errors (while the db is coming up, this happens on docker compose), should we account for a number of retries maybe before giving 503?

Otherwise, I'm wondering if there's value in merging this independently (separate from #4703), since under a connection error we'll now force a client to handle both upstream connection refused and 503 instead of just 503.

If we agree it's not an improvement on its own, maybe we should merge it together in #4703 (which is of course great on its own). That way we avoid a change in behavior here, since on #4703 this change will be guarded by the reuse port config.

Thoughts?

[steve-chavez]

So from the point of view of the clients (they don't know when Postgrest was started), we have 3 alternatives:

1. connection refused -> 503 -> normal
2. connection refused -> blocked/time out -> normal
3. connection refused -> normal

I am not sure what value clients get from the first two options comparing to the third one.
#4703 (comment)

I remember on #4703 (comment) the third option sounded great and IIRC it was the main motivation for this PR, but under real world conditions we've seen connection refused could last forever (e.g. if the schema cache never loads due to statement_timeout), so in practice we'll always devolve to option 1.

[mkleczek]

So from the point of view of the clients (they don't know when Postgrest was started), we have 3 alternatives:

1. connection refused -> 503 -> normal
2. connection refused -> blocked/time out -> normal
3. connection refused -> normal

I am not sure what value clients get from the first two options comparing to the third one.
#4703 (comment)

I remember on #4703 (comment) the third option sounded great and IIRC it was the main motivation for this PR, but under real world conditions we've seen connection refused could last forever (e.g. if the schema cache never loads due to statement_timeout), so in practice we'll always devolve to option 1.

The problem is with SO_REUSEPORT - in this case we strictly want the new instance not to start listening until it can serve requests. But we cannot really detect if we are started as a "replacement" instance or a "fresh" instance.

The are several scenarios we have to consider, I guess:

1. Fresh start of PostgREST before database is available - that can happen when both are started as systemd services that don't have proper dependencies set between them. PostgREST starts first and gets errors when it tries to connect to the db.
2. Fresh start of PostgREST when database is available and all goes ok (ie. happy path).
3. SO_REUSEPORT start - eg. zero downtime upgrade.

I am now starting to think that the best strategy is the original idea of this PR: do not listen until schema cache is loaded (even in case of errors) - it handles scenarios 2 and 3 properly and in scenario 1 it makes clients receive connection refused until both db and PostgREST are ready. Which I would say is fine - diagnostics is a little more difficult (because clients always get connection refused) but not that much - logs and admin server should provide enough information.

WDYT?

[steve-chavez]

The problem is with SO_REUSEPORT - in this case we strictly want the new instance not to start listening until it can serve requests. But we cannot really detect if we are started as a "replacement" instance or a "fresh" instance.

Yes, that's why I mentioned above that this behavior of connection refused would only make sense with #4703, since there it can be enabled with the server-reuseport config.

Which I would say is fine - diagnostics is a little more difficult (because clients always get connection refused) but not that much - logs and admin server should provide enough information.

Yes, this would only make things harder for non reuseport cases (since connection refused can last long). There's no benefit on changing the behavior for the default case. We would cause a breaking change unnecessarily.

[mkleczek]

Yes, this would only make things harder for non reuseport cases (since connection refused can last long). There's no benefit on changing the behavior for the default case. We would cause a breaking change unnecessarily.

The problem with what we have currently is that clients get errors during startup even if all is fine. That's confusing and IMHO wrong. I'd say that gives us a choice:

1. Change it as it is right now in this PR (ie. wait for first schema cache load to finish, then listen)
2. Wait for schema cache to load successfully.

The first option stays compatible with what we have now (and does not improve anything in cases when initial schema cache load fails). The second option seems cleaner to me but it is not clear cut.

[wolfgangwalther]

Needs a rebase after 1a6ba20.

[mkleczek]

Needs a rebase after 1a6ba20.

The reason I didn't do refactoring first was to avoid hard to resolve conflicts. I'd be grateful if we collaborated more on PRs to make our job easier instead of harder.

[mkleczek]

Needs a rebase after 1a6ba20.

The reason I didn't do refactoring first was to avoid hard to resolve conflicts. I'd be grateful if we collaborated more on PRs to make our job easier instead of harder.

Rebased

[wolfgangwalther]

The reason I didn't do refactoring first was to avoid hard to resolve conflicts.

Same reasoning here - but with an eye on our future selves, when we need to maintain things. It's much more likely we'd like to revert this fix compared to the refactor. If we do the refactor first, then the fix, it's easy to revert. If we do it the other way around, we'd then need to be very careful at that time.

btw rebasing your changeset over it should not have been hard. It should have been as easy as:

• Start the rebase on main
• When hitting conflicts, checkout the conflicted file from the final commit in refactor: Simplify App.initServerSocket #4951

The result after the two commits is the same, so that part is really easy. The harder to resolve conflict, which included actually looking at the code, was the one that I did when I cherry-picked it. That's why I did it and didn't force it onto you.

[steve-chavez]

Looked at all the change in tests, they look fine.

All is left is resolving https://github.com/PostgREST/postgrest/pull/4880/changes#r3306654090.