Skip to content

v14: fix: Do not clear the schema cache during retries - #5002

Closed
taimoorzaeem wants to merge 1 commit into
PostgREST:v14from
taimoorzaeem:v14
Closed

v14: fix: Do not clear the schema cache during retries#5002
taimoorzaeem wants to merge 1 commit into
PostgREST:v14from
taimoorzaeem:v14

Conversation

@taimoorzaeem

Copy link
Copy Markdown
Member

Backport #4869.

retryingSchemaCacheLoad should not clear existing schema cache upon failure - there is no reason to do that. If there is a communication issue with the database server or db is down, clients are going to get 502 anyway. If it was a glitch when loading the schema cache - the clients are going to use old (stale) schema cache for some time until next retry re-loads it successfully.
@wolfgangwalther

Copy link
Copy Markdown
Member

I'm not sure why this was labeled fix, but I don't think we should backport this. This just seems like a change from one intended behavior to another. Might not be a breaking change, but still.

@taimoorzaeem
taimoorzaeem marked this pull request as draft June 12, 2026 07:58
@steve-chavez

Copy link
Copy Markdown
Member

I'm not sure why this was labeled fix,

We should amend the changelog on #4869, I remember thinking it needed docs but was conflicted by the fix prefix.

@mkleczek

Copy link
Copy Markdown
Collaborator

I'm not sure why this was labeled fix,

We should amend the changelog on #4869, I remember thinking it needed docs but was conflicted by the fix prefix.

But it does fix #4873, doesn't it?

@wolfgangwalther

Copy link
Copy Markdown
Member

But it does fix #4873, doesn't it?

There's multiple angles to look at this:

  • Let's consider A statement timeout can void the schema cache #4873 a bug (which I don't really agree with), then this change does indeed fix that bug. But just because it fixes a bug, it doesn't mean that this only fixes a bug. You can fix a bug in many ways - and introducing a significant change in behavior, where both the previous and the new behavior were explicitly intended and we did not communicate it anywhere that we intended differently, is a "heavier" change than just fixing a bug.
  • Quite clearly the underlying discussion in that issue showed that this is just one tiny step in a much bigger change of mind on how to approach the schema cache and down-time. This is not an isolated change that just works nicely on its own and does not need any other considerations or future changes. It's part of something much bigger.

No matter whether you call it a fix or not - this is just not material to backport.

@wolfgangwalther

Copy link
Copy Markdown
Member

it needed docs but was conflicted by the fix prefix.

tbh, I'm not sure whether I follow the reasoning of "it's a fix, so no docs needed" ;)

@steve-chavez

steve-chavez commented Jun 30, 2026

Copy link
Copy Markdown
Member

tbh, I'm not sure whether I follow the reasoning of "it's a fix, so no docs needed" ;)

We never done that for regular fixes since they mean a "correction to match already documented behavior", only for critical fixes (like https://github.com/PostgREST/postgrest/releases/tag/v12.2.12) we've added docs; because they're partly an addition (new behavior). So far that has made sense.

We'll likely do the same for #5012.

Edit: added this clarification on 412c6a1

@steve-chavez

Copy link
Copy Markdown
Member

We should amend the changelog on #4869, I remember thinking it needed docs but was conflicted by the fix prefix.

Updated the docs on 6951e13. I only updated a note that was now wrong. It didn't warrant changing the fix to a feature because there wasn't much behavior change.

Quite clearly the underlying discussion in that issue showed that this is just one tiny step in a much bigger change of mind on how to approach the schema cache and down-time

Yeah I think that this is part of treating the "schema cache as a performance optimization", which makes a lot of sense considering some wrong decisions in the past (like passing it to SQL land #1794). This also further validates postgrest-openapi and #5031.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants