perf: no caching for prefer timezone

[steve-chavez]

Fixes #5100 and #4751.

This essentially removes the timezones from the schema cache. Please read the latest comments on #5100 to fully understand the implementation.

To discuss

Edit: We'll keep the revert as we're not going to have a lenient variant as discussed below.

Here I've also removed the db-timezone-enabled config by reverting d6c338d. But we might want to keep it given that the query in the handling=lenient case is kinda expensive (ref). Some users might not want clients forcing that query.

Or perhaps we can have a config similar to the one for Prefer:tx, only allowing the strict timezone handling.

[wolfgangwalther]

This is not a fix, but a performance improvement.

[taimoorzaeem]

Oh, I think you missed removing the changelog entry from the unreleased section:

postgrest/CHANGELOG.md

Line 14 in e820cc7

- Add config `db-timezone-enabled` for optional querying of timezones by @taimoorzaeem in #4751

[steve-chavez]

This is not a fix, but a performance improvement.

Ah but the incorrect reloading part we discussed on #4751 (comment) seems like a bug to me. If someone did modify their timezones the cache would not be reloaded. For #5100 it would be purely perf but I dunno which one wins here.

[wolfgangwalther]

incorrect reloading part we discussed on #4751 (comment) seems like a bug to me. If someone did modify their timezones the cache would not be reloaded.

"automatic reloading" is not a feature that PostgREST provides. It's part of the documentation for how to set it up. The bug is not in PostgREST - it's just that this recommendation does not cover this aspect. You could argue that this recommendation is wrong - but we never call wrong things in the docs "bugs" or changes to the docs "fix".

One approach to resolve this issue would be to correct the docs as I mentioned earlier. You instead opted to change the code as part of a performance improvement, which makes the issue obsolete. Still not a bug, nor a fix. Especially the PR title / commit message is just wrong: The caching of timezones was always correct, there was not a problem with it.

[steve-chavez]

Changed the commit prefix to perf. Also added tests and docs for the numeric offset timezones on separate commits. Offsets are a new feature but I didn't knew how to separate them from the perf commit.

[taimoorzaeem]

Just did a rg -n "dbTimezone" in project root, looks like we forgot to remove this:

postgrest/docs/references/admin_server.rst

Line 77 in 6304265

"dbTimezones": ["..."]

and this:

postgrest/test/io/test_cli.py

Lines 240 to 251 in 6304265

	"dbTimezones",
	],
	)
	def test_schema_cache_snapshot(baseenv, key, snapshot_yaml):
	"Dump of schema cache should match snapshot."
	schema_cache = yaml.load(cli(["--dump-schema"], env=baseenv), Loader=yaml.Loader)
	formatted = yaml.dump(
	schema_cache[key],
	encoding="utf8",
	allow_unicode=True,
	Dumper=yaml.SafeDumper if key == "dbTimezones" else ExtraNewLinesDumper,

[taimoorzaeem]

LGTM 👍

[wolfgangwalther]

Needs a rebase to resolve conflicts, too.

[steve-chavez]

Addressed all the feedback.

[wolfgangwalther]

Thank you! ❤️