Claude/update fork upstream xq c1 m - #4
Merged
Conversation
* Rewrite bug fixes - old cache key, more efficient update * Update pgdog/src/frontend/prepared_statements/mod.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update pgdog/src/frontend/client_request.rs Co-authored-by: Lev Kokotov <levkk@users.noreply.github.com> * Skip removing and readding cache item, just modify it in place * skip cloning name if passed by reference * skip cloning name if passed by reference * format * cache key fixes * Update cache removal test to handle close_unused(0) clearing --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Lev Kokotov <levkk@users.noreply.github.com>
) * Add query parameter support for Database URL configuration Enables configuring Database fields via URL query parameters in databases_from_urls(), allowing flexible configuration through environment variables without separate config files. Supported parameters: database_name, role, shard, user, password, pool_size, min_pool_size, pooler_mode, statement_timeout, idle_timeout, server_lifetime, read_only Example: PGDOG_DATABASE_URL_1=postgres://user:pass@host:port/db?database_name=realdb&role=replica&shard=3 * Load mirroring configuration from PGDOG_MIRRORING_N ENV vars
* Fix pgdogdev#48 * remove sha * Sleep before checking process * Use tls in CI
* swap to u16 on inbound and outbound paths * Add a test to check bounds of backend pg parameter limits * also update num_codes * parameterize u16 test to cover pgdog and raw postgresql * Unit test for parameter descriptions FromBytes/ToBytes * format
* Initial pass on TLS hot swap * wip tls testing * Ensure TLS acceptor cached * test demonstrating no additional tls acceptor rebuilds * Set up explicit TCP connector caching
* Correctly handle error state * Timeout server error * Add rollback timeout * Test and warning
* Initial AVG + expression parser AVG function implementation: Detects paired count+avg functions on the same expression Expression parser handles logic of "same expression" by assigning id through interning Adds "ghost" column using rewriting if no count is paired with avg removes "ghost" column from returned value of aggregates * rollback transactions correctly * stddev / variance * format * cover changed files with tests * format
* tweak codecov.yml * adding additional tests, for the admin module * format * prom stats tests * basic tests for some replication fns * scram and multitenant smoke tests * Update admin tests to add new pool value (healthy)
…o actually modify (pgdogdev#543)
* Add plugin unit/integration tests, optional for PR merge * install mold before plugin tests * try bundling * try to pass plugin unit tests * cant sudo bundle * try bundle * remove other bundler * use vendored bundler
* Support savepoints * Fix spelling
* initial draft internal explain annotations * clean up formatting logic in query parser * explain plan annotation integration test * explain annotation testing * enable explain annotations only when expanded_explain flag set * Enable expanded_explain in integration tests * Tweak explain recorder * better integration debugging * less strict assertion :(
* Dont rewrite anon prepared statements by default * Fix test
* Use Bytes for Bind fields * Fix warning * comment
Save `SET LOCAL` into Parameters so we can fetch them if we're looking for `search_path` for schema-based routing. Also, fix parameter encoding/decoding.
``` client "pgdog" startup parameters: application_name="psql", client_encoding="UTF8", database="pgdog", pgdog.role="primary", user="pgdog" [127.0.0.1:33622] ```
Log a reason for which a server connection was created. This helps debug configuration.
Log info about pool state when pool decides to create new connection. Additionally, avoid triggering pool creation check when pool is full: saves us a mutex lock and makes the pool, at capacity, a bit faster! A couple clippy fixes as well.
- fix: collision in server <-> client mapping in pool leaked connections by under-reporting the number of checked out connections. This is caused by "async" `Server::drop` which _eventually_ returns a connection back into the pool, but fast clients are already requesting a new one, overriding their server <-> client mapping. - fix: under counting banned hosts in unban all logic. - refactor: create a `ClientComms` for each client, instead of mutating a struct in-place - fix: use atomic integer to generate client IDs, reducing collision probability
pgdogdev#677 Prepared statements engine was acting as if `prepared_statements = "full"` was default, and rewriting `PREPARE` and `EXECUTE` commands (incorrectly, inside transactions) even though `prepared_statements = "extended"` is actually the default.
Parameters that contain double quotes were encoded incorrectly.
## Purpose This addresses some side effects of pgdogdev#565 in that when a connection falls out of sync (due to currently unknown series of events), the connection is detected as out of sync, but is checked back into the pool without any corrective action. ## Approach This updates server.rs to set its stats state to State::Error when we encounter a ProtocolOutOfSync error while reading from the server and decided whether to forward the message to the client. The result is that the connection will be disconnected when attempted to be checked back into the pool and the tainted connection will not be re-used.
I ran into a bug, where the `application_name` in the postgres connection parameters was not being correctly set on the server after the first connection. I tested with a local postgres container. Here is my `pgdog.toml`: ``` [general] host = "127.0.0.1" port = 6432 pooler_mode = "session" # Primary Database [[databases]] host = "127.0.0.1" name = "primary" port = 5433 database_name = "intempus" user = "intempus" password = "intempus" pool_size = 1 min_pool_size = 1 ``` I could reproduce the problem by starting pgdog and then connecting with psql, running a query to print the `application_name` in the postgres DB: ``` $ psql "host=127.0.0.1 port=6432 dbname=primary application_name=test1" -c "SELECT application_name,query from pg_stat_activity;" application_name | query ------------------+------------------------------------------------------ test1 | SELECT application_name,query from pg_stat_activity; $ psql "host=127.0.0.1 port=6432 dbname=primary application_name=test1" -c "SELECT application_name,query from pg_stat_activity;" application_name | query ------------------+------------------------------------------------------ PgDog | SELECT application_name,query from pg_stat_activity; $ psql "host=127.0.0.1 port=6432 dbname=primary application_name=test1" -c "SELECT application_name,query from pg_stat_activity;" application_name | query ------------------+------------------------------------------------------ PgDog | SELECT application_name,query from pg_stat_activity; ``` The `application_name` will always be `PgDog` after the 1st connection. If I change the `application_name` right after to something other than `test1`, it will reset and work for the 1st connection only again: ``` $ psql "host=127.0.0.1 port=6432 dbname=primary application_name=test2" -c "SELECT application_name,query from pg_stat_activity;" application_name | query ------------------+------------------------------------------------------ test2 | SELECT application_name,query from pg_stat_activity; $ psql "host=127.0.0.1 port=6432 dbname=primary application_name=test2" -c "SELECT application_name,query from pg_stat_activity;" application_name | query ------------------+------------------------------------------------------ PgDog | SELECT application_name,query from pg_stat_activity; $ psql "host=127.0.0.1 port=6432 dbname=primary application_name=test2" -c "SELECT application_name,query from pg_stat_activity;" application_name | query ------------------+------------------------------------------------------ PgDog | SELECT application_name,query from pg_stat_activity; ``` I tracked down the [problem to this comparison statement in servers.rs](https://github.com/pgdogdev/pgdog/blob/main/pgdog/src/backend/server.rs#L479). The `identical` function compares if the hashes in `params` and `client_params` on the server object are the same. On my 1st connection attempt the comparison correctly identifies that the parameters are not the same and goes into the if block. All subsequent connections never go into the if block, until the `application_name` is changed. Here are the values for `params` and `self.client_params`: ``` # 1st call client_params: Parameters { params: {}, transaction_params: {}, transaction_local_params: {}, hash: 0 } params: Parameters { params: {"application_name": String("test2"), "client_encoding": String("UTF8"), "database": String("primary"), "user": String("intempus")}, transaction_params: {}, transaction_local_params: {}, hash: 6714540884140617138 } # 2nd call client_params: Parameters { params: {}, transaction_params: {}, transaction_local_params: {}, hash: 6714540884140617138 } params: Parameters { params: {"application_name": String("test2"), "client_encoding": String("UTF8"), "database": String("primary"), "user": String("intempus")}, transaction_params: {}, transaction_local_params: {}, hash: 6714540884140617138 } ``` When the connection is closed and a `DISCARD ALL` query is sent, `self.client_params.clear()` [is called](https://github.com/pgdogdev/pgdog/blob/main/pgdog/src/backend/server.rs#L445), but that doesn't recompute the hash, so parameters will be cleared but the hash will stay the same value and result in wrong `identical` checks from then on. Ensuring the hash is recomputed when the parameters are cleared fixes the issue. Let me know if it sounds good or if it should be done using a different approach :)
- refactor: move rewrite engine into its own module under the parser and make it run independently - feat: support binary encoding for multi-tuple insert statements splitting - feat: multi-tuple inserts use whatever protocol the original insert was sent with instead of rewriting them into simple queries - feat: generate unique IDs with `pgdog.unique_id()` function call anywhere in a query - refactor: query AST is attached to the request, ensuring it's available to all parts of the parser/rewrite engine - fix: ambiguous shard key detection refactored into a priority heap, allowing the parser to evaluate multiple conditions and select the highest priority one; added granular debugging to check where a shard route came from - fix: sending `BEGIN` twice when starting transactions in sharded and load balanced clusters - feat: support `SET [LOCAL] pgdog.shard` and `SET [LOCAL] pgdog.sharding_key` in and out of transactions, following Postgres semantics - feat: add `rewrite` column to `SHOW PREARED` to show actual statements executed on the server - feat: keep rewritten statements in the prepared cache to avoid churn - refactor: much more granular errors for different not-connected states - refactor: change some methods that return boolean to use `is_` prefix - chore: add quite a bit of test coverage, especially around schema-based sharding - refactor: re-build parameter sharding hints for each request, avoiding stale state in the query engine - chore: attribute message input/output in trace logs to each server - chore: created `QueryParserTest` and `TestClient` structs to simplify testing those complex components - fix: parsing `BIGINT` sharding keys in simple queries that exceeded that `INTEGER` range (2.2B) - feat: handle `SET` commands completely inside the proxy, not sending them to a server even if using extended protocol - fix: query comments-based sharding hints being ignored when schema sharding was used - fix: dedup shard numbers in `Shard::Multi` binding
- chore: add `RESET` test for pgdogdev#682 - chore: tag release
…sharding (pgdogdev#684) Example: ``` BEGIN; SELECT 1; SET LOCAL work_mem TO '128MB'; COMMIT; ```
Fixes ``` WARNING: SET TRANSACTION can only be used in transaction blocks ``` and possibly the not in transaction warnings reported in pgdogdev#685
…ogdev#687) Handle saving/restoring parameters for connections that execute `SET` after a regular query inside a transaction, e.g.: ```sql BEGIN; SET LOCAL statement_timeout TO 1234; SELECT 1; SET work_mem TO '128MB'; COMMIT; ``` This would preserve the value of `work_mem` for that client for all subsequent transactions, as per Postgres spec.
Obviously this doesn't require people to use mise, but this provides a simple installation for people that do, and we can hardcode the versions of dev tools that pgdog relies on in case that's needed later. Obviously a totally optional one, but I set it up locally so figured I'd upstream.
- refactor: move sharding key update into its own module - BREAKING: sharding key updates require a transaction started by the user instead of starting one automatically - feat: support `RETURNING` clause in `UPDATE` that changes the sharding key - fix: `ErrorResponse` message incorrectly parsed `severity` field - feat: double-check we actually need to do a multi-step key update when the key isn't actually changed by reading the row from the database This is common in Rails: ```sql UPDATE orders SET user_id = $1, amount = $2, created_at = $3, /* [..every other model field..] */ WHERE id = $1 ``` The sharding key doesn't actually change (`user_id`), the ORM just puts it into the query anyway.
- fix: handle expressions in `UPDATE` statements that change the sharding key - chore: clippy ```sql UPDATE users SET id = $1, created_at = now() WHERE id = $2 ```
Make sure we escape any special characters in the parameter values. Will make sure SET queries always work as expected.
We forked [pg_query](https://github.com/pgdogdev/pg_query.rs) and replaced protobuf (de)serialization with native C/Rust bindings. This results in the following performance improvements: 1. Query parsing is 5x faster. 2. Query construction using the Posgres AST builder is 10x faster. We can generate queries on the fly with virtually no performance overhead. 3. `pg_bench` is reporting a 25% improvement in benchmarks using the simple protocol, which we don't cache. This is great for clients that don't support or use prepared statements (e.g. Rails!). This is configurable with the new `query_parser_engine` setting: ```toml [general] query_parser_engine = "pg_query_raw" # pg_query_protobuf to use the old protobuf-based path ``` While we continue to test this, we'll be using the old protobuf-based paths by default. This PR therefore won't affect any current deployments and adoption is completely optional. Things we need to optimize: 1. The raw C parser is very recursive. We're currently bumping up the Tokio worker stack size to 32MiB to make sure there is enough to parse complex statements, but that increases our memory requirements quite a bit. This is _per thread_, so if you have 4 threads, PgDog will allocate 32 x 4 = 132 MiB of stack memory on boot. For most modern systems that's not a problem, but still, the memory usage increases quite a bit. 2. The parser may not be complete. While we test it extensively, the number of Postgres AST nodes is high. ### Misc Upgraded rand to 0.9.2.
The shard calculating logic was using table-based sharding and erroring out when no sharded tables were configured. Also, added a test to comment parser just for good measure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.