edgedb-go

edgedb-go copied to clipboard

While benchmarking the client I noticed that every connection release triggered parsing of the system_config server setting to get the idle connection timeout. This in turn requires building a brand new decoder for the systemConfig, doh!

There is also a theoretical issue with a transparent server upgrade. The connection that receives the system_config blob might use a different protocol version than the connection that reads the blob again to parse it.

I've switched all of the parsing logic into the write code path for settings. To simplify the read call sites, I've also integrated the defaults handling into the ServerSettings interface. The zero value of ServerSettings is ready to use.

~Now that we use individual struct fields for the values, I've removed the locking for reads/writes -- it was required for concurrent read/write access on the map only.~ Apparently we still need it to make the race detector happy. It is technically fine to concurrently read/write the settings as we are not interested in any sequence or deem any of the old/new value correct, also the written values are not dependent on the read values in another goroutine. TL;DR r/w locking has been restored to keep this simple.

The handling of DSN query parameters and Options.ServerSettings map entries is stricter now. It is returning an error for an invalid value on a known key (e.g. [^1] or [^2]). The shared test cases send some unknown keys, so I could not flag all of these as errors. It's probably for the best to flag invalid usage of the client at boot time instead of silently ignoring a (typoed) field. If you agree on that, would you prefer adjusting the shared test cases to send reasonable query parameters instead of param, a, b; or have the Golang client ignore these cases?

As a bonus: There is a edgedb.Duration.ToStdDuration helper now, which converts from microseconds to nanoseconds.

[^1]: suggested_pool_concurrency=! -> edgedb.ConfigurationError: invalid edgedb.Options: invalid ServerSettings: invalid suggested_pool_concurrency: strconv.Atoi: parsing "!": invalid syntax [^2]: system_config={} -> edgedb.ConfigurationError: invalid edgedb.Options: invalid ServerSettings: invalid system_config: too few bytes

I'm not confident I understand all the intended use cases for the server settings so I'm hesitant to make big changes here. @1st1 or @elprans can you give us some direction on how this should work?

The handling of DSN query parameters and Options.ServerSettings map entries is stricter now. It is returning an error for an invalid value on a known key

This should probably be proposed in the https://github.com/edgedb/shared-client-testcases/ repo first. We want to keep client behavior as similar as possible across languages.

The shared test cases send some unknown keys, so I could not flag all of these as errors. It's probably for the best to flag invalid usage of the client at boot time instead of silently ignoring a (typoed) field. If you agree on that, would you prefer adjusting the shared test cases to send reasonable query parameters instead of param, a, b; or have the Golang client ignore these cases?

I'm guessing here but it seems like if the shared-client-testcases has arbitrary keys it's probably on purpose.

As far as optimizations go it seems fine to move the session_idle_timeout and other frequently used value parsing so that it happens when the message is received from the server and then store the typed value on the session.