Skip to content

Extended-protocol fixes: Flush handling + prepared-statement client-cache consistency - #953

Open
ikeboy003 wants to merge 557 commits into
postgresml:mainfrom
YCWagerWise:fix/extended-protocol-flush-and-prepared-cache
Open

ikeboy003 wants to merge 557 commits into
postgresml:mainfrom
YCWagerWise:fix/extended-protocol-flush-and-prepared-cache

Conversation

@ikeboy003

Copy link
Copy Markdown

Four commits. Two distinct fixes that share a theme: pgcat losing extended-protocol state in ways the client cannot detect or recover from.

1. Flush ('H') handling — 3 commits

Drivers using describeFirst (e.g. postgres.js with prepare: false + params) send Flush between Parse/Describe and Bind/Execute. pgcat logged Unexpected code: H and dropped it, hanging the client.

  • Handle Flush ('H') extended-protocol message — drains buffered extended-protocol messages into the send buffer, forwards the Flush byte, keeps the server checked out for the follow-up Bind/Execute/Sync.
  • Read Flush responses without waiting for ReadyForQuery — Server::recv() only breaks on 'Z', which the server never sends after Flush. Adds recv_flush_response() that breaks on the actual terminal markers (RowDescription / NoData / ErrorResponse / BindComplete).
  • Flush response: drop double-copy parse and clone — perf: inspect framing byte without re-parsing length or copying the body for messages we only need to identify.

2. Prepared-statement client-cache desync — 1 commit

ensure_prepared_statement_is_on_server on PreparedStatementError:

  1. Removed the entry from the client's prepared_statements map
  2. Returned Ok(())

The client never received Close('S') so it still believed the statement was valid. The next Bind for that name missed in buffer_bind and pgcat dropped the entire TCP conn with ClientError(\"Prepared statement sN doesn't exist\").

For drivers that cache prepared statements per logical connection (tokio-postgres, asyncpg, pgx) and reuse them across many transactions, this is unrecoverable — the client has no way to know its view diverged from pgcat's.

Fix:

  • Keep the client cache intact — it mirrors what the client believes; dropping it without Close is a protocol-level lie.
  • Mark the offending server bad so the pool replaces it on next checkout.
  • Propagate PreparedStatementError so the caller fails this one op cleanly and retries on a fresh backend.

Test plan

  • cargo build clean
  • cargo fmt --check
  • postgres.js prepare: false + params: completes in ~500ms (was hanging)
  • Happy to add an integration test for Flush + describeFirst if you point me at the right harness

drdrsh and others added 30 commits January 19, 2023 05:18
We are seeing some Error reading message code from socket error messages, we want to get more context so this PR logs the actual error reported.
…ml#285)

* Removes message cloning operation required for query router

* fmt

* flakey?

* ?
This change:
- Adds server metrics to prometheus endpoint.
- Adds database metrics to prometheus endpoint.
- Adds pools metrics to prometheus endpoint.
- Change metrics name to have a prefix of (stats|pools|databases|servers).
…l#288)

* Refactors is_banned logic and forces healthcheck on unban

* typo

* Make is banned log debug

* addressing comments

* Comment
Bumps [toml](https://github.com/toml-rs/toml) from 0.5.10 to 0.5.11.
- [Release notes](https://github.com/toml-rs/toml/releases)
- [Commits](toml-rs/toml@toml-v0.5.10...toml-v0.5.11)

---
updated-dependencies:
- dependency-name: toml
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [async-trait](https://github.com/dtolnay/async-trait) from 0.1.61 to 0.1.63.
- [Release notes](https://github.com/dtolnay/async-trait/releases)
- [Commits](dtolnay/async-trait@0.1.61...0.1.63)

---
updated-dependencies:
- dependency-name: async-trait
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [toml](https://github.com/toml-rs/toml) from 0.5.11 to 0.6.0.
- [Release notes](https://github.com/toml-rs/toml/releases)
- [Commits](toml-rs/toml@toml-v0.5.11...toml-v0.6.0)

---
updated-dependencies:
- dependency-name: toml
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
* Dont require servers to be online to start pooler

* PAUSE/RESUME

* fix

* Refresh pool

* Fixes

* lint
Bumps [tokio](https://github.com/tokio-rs/tokio) from 1.24.2 to 1.25.0.
- [Release notes](https://github.com/tokio-rs/tokio/releases)
- [Commits](https://github.com/tokio-rs/tokio/commits/tokio-1.25.0)

---
updated-dependencies:
- dependency-name: tokio
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [toml](https://github.com/toml-rs/toml) from 0.6.0 to 0.7.0.
- [Release notes](https://github.com/toml-rs/toml/releases)
- [Commits](toml-rs/toml@toml-v0.6.0...toml-v0.7.0)

---
updated-dependencies:
- dependency-name: toml
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [bytes](https://github.com/tokio-rs/bytes) from 1.3.0 to 1.4.0.
- [Release notes](https://github.com/tokio-rs/bytes/releases)
- [Changelog](https://github.com/tokio-rs/bytes/blob/master/CHANGELOG.md)
- [Commits](tokio-rs/bytes@v1.3.0...v1.4.0)

---
updated-dependencies:
- dependency-name: bytes
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [hyper](https://github.com/hyperium/hyper) from 0.14.23 to 0.14.24.
- [Release notes](https://github.com/hyperium/hyper/releases)
- [Changelog](https://github.com/hyperium/hyper/blob/v0.14.24/CHANGELOG.md)
- [Commits](hyperium/hyper@v0.14.23...v0.14.24)

---
updated-dependencies:
- dependency-name: hyper
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [toml](https://github.com/toml-rs/toml) from 0.7.0 to 0.7.1.
- [Release notes](https://github.com/toml-rs/toml/releases)
- [Commits](toml-rs/toml@toml-v0.7.0...toml-v0.7.1)

---
updated-dependencies:
- dependency-name: toml
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [futures](https://github.com/rust-lang/futures-rs) from 0.3.25 to 0.3.26.
- [Release notes](https://github.com/rust-lang/futures-rs/releases)
- [Changelog](https://github.com/rust-lang/futures-rs/blob/master/CHANGELOG.md)
- [Commits](rust-lang/futures-rs@0.3.25...0.3.26)

---
updated-dependencies:
- dependency-name: futures
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [async-trait](https://github.com/dtolnay/async-trait) from 0.1.63 to 0.1.64.
- [Release notes](https://github.com/dtolnay/async-trait/releases)
- [Commits](dtolnay/async-trait@0.1.63...0.1.64)

---
updated-dependencies:
- dependency-name: async-trait
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Mistakenly logging username as poolname and poolname as username
We have encountered a case where PgCat pools were stuck following a database incident. Our best understanding at this point is that the PgCat -> Postgres connections died silently and because Tokio defaults to disabling keepalives, connections in the pool were marked as busy forever. Only when we deployed PgCat did we see recovery.

This PR introduces tcp_keepalives to PgCat. This sets the defaults to be

keepalives_idle: 5        # seconds
keepalives_interval: 5 # seconds
keepalives_count: 5    # a count
These settings can detect the death of an idle connection within 30 seconds of its death. Please note that the connection can remain idle forever (from an application perspective) as long as the keepalive packets are flowing so disconnection will only occur if the other end is not acknowledging keepalive packets (keepalive packet acks are handled by the OS, the application does not need to do anything). I plan to add tcp_user_timeout in a follow-up PR.
Code coverage logic was missing coverage from rust tests. This is now fixed.
Also, we weren't reaping spawned PgCat processes correctly which left zombie processes.
* Support EC and PKCS8 private keys

* Use iter instead of infinite loop in `load_keys` fn
warning: use of deprecated function `base64::decode`: Use Engine::decode
Bumps [once_cell](https://github.com/matklad/once_cell) from 1.17.0 to 1.17.1.
- [Release notes](https://github.com/matklad/once_cell/releases)
- [Changelog](https://github.com/matklad/once_cell/blob/master/CHANGELOG.md)
- [Commits](matklad/once_cell@v1.17.0...v1.17.1)

---
updated-dependencies:
- dependency-name: once_cell
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
What
Allows shard selection by the client to come in via comments like /* shard_id: 1 */ select * from foo;

Why
We're using a setup in Ruby that makes it tough or impossible to inject commands on the connection to set the shard before it gets to the "real" SQL being run. Instead we have an updated PG adapter that allows injection of comments before each executed SQL statement. We need this support in pgcat in order to keep some complex shard picking logic in Ruby code while using pgcat for connection management.

Local Testing
Run postgres and pgcat with the default options. Run psql < tests/sharding/query_routing_setup.sql to setup the database for the tests and run ./tests/pgbench/external_shard_test.sh as often as needed to exercise the shard setting comment test.
Instead of downloading Toxiproxy everytime we run CI, we bake it into the CI docker image
We have to build and push the docker image used in CI manually. This PR builds that image automatically and pushes it to Github docker repository.

Will start using that image in a follow PR
I am seeing Directory (/home/circleci/project) you are trying to checkout to is not empty and not a git repository error after I started using the new Dockerfile.ci image. My best guess is that this failure is because we download toxiproxy.deb file into the home directory which blocks git checkout.

This PR moves toxiproxy to /tmp/ to avoid this
I accidentally removed `psmisc` from the image and now the test builds are failing. I am adding it back in this PR
Connection to the CI databases is viewed by Postgres as coming from localhost. The pg_hba.conf file generated by the docker image uses trust for these connections, that's why we had no test coverage on SASL and md5 branches.

This PR fixes this issue. There was also an issue with under-reporting code coverage. This should be fixed now
dependabot Bot and others added 29 commits September 13, 2024 19:41
…tgresml#812)

Bumps [helm/chart-releaser-action](https://github.com/helm/chart-releaser-action) from 1.5.0 to 1.6.0.
- [Release notes](https://github.com/helm/chart-releaser-action/releases)
- [Commits](helm/chart-releaser-action@be16258...a917fd1)

---
updated-dependencies:
- dependency-name: helm/chart-releaser-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
postgresml#804)

Bumps [activesupport](https://github.com/rails/rails) from 7.0.4.1 to 7.0.7.1.
- [Release notes](https://github.com/rails/rails/releases)
- [Changelog](https://github.com/rails/rails/blob/v7.2.1/activesupport/CHANGELOG.md)
- [Commits](rails/rails@v7.0.4.1...v7.0.7.1)

---
updated-dependencies:
- dependency-name: activesupport
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Build is failing with this error

Downloading activerecord-3.2.14 revealed dependencies not in the API or the
lockfile (activesupport (= 3.2.14), activemodel (= 3.2.14), arel (~> 3.0.2),
tzinfo (~> 0.3.29)).
Either installing with `--full-index` or running `bundle update activerecord`
should fix the problem.

After ActiveSupport was updated.

This PR fixes that
… from K8s secret (postgresml#753)

* Make user min_pool_size configurable

* Set user server_lifetime only if specified

* Increment chart version

* Use default instea of or

* Allow enabling server_tls

* statement_timeout default value

* Allow pulling password from existing secret

---------
…gresml#822)

Currently, `connect_timeout` sounds like it should be for connections to
the Postgres server. It's actually used for obtaining a connection from
the pool.
End prometheus stats with a new line separator

According to the [OpenMetrics specification](https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#overall-structure), each line MUST end with `\n`. Previously, the last line was not ending with `\n`, so that strict parsers had issues reading the Prometheus stats.
* Update bb8 to 0.8.6

To get djc/bb8#186 and djc/bb8#189
which fix potential deadlocks (djc/bb8#154).

Also, this (djc/bb8#225) was needed to prevent a connection
leak which was conveniently spotted in our integration tests.

* Ignore ./.bundle (created by dev console)

---------
Add `unban_replicas_when_all_banned` to control unbanning replicas behavior.
postgresml#847)

Fix default_role being ignored when query_parser_enabled was false
)

Revert "Do not unban replicas if a primary is available (postgresml#843)"

This reverts commit 3d48265.
* Fix contact info for Helm chart
* chore(deps): bump sqlparser from 0.41.0 to 0.52.0

Bumps [sqlparser](https://github.com/apache/datafusion-sqlparser-rs) from 0.41.0 to 0.52.0.
- [Changelog](https://github.com/apache/datafusion-sqlparser-rs/blob/main/CHANGELOG.md)
- [Commits](https://github.com/apache/datafusion-sqlparser-rs/commits)

---
updated-dependencies:
- dependency-name: sqlparser
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* bump

* Update to latest sqlparser version

---------

Signed-off-by: dependabot[bot] <support@github.com>
In a high availability deployment of PgCat, it is possible that a client may land on a container of PgCat that is very busy with clients and as such the new client might be perpetually stuck in checkout failure loop because all connections are used by other clients. This is specially true in session mode pools with long-lived client connections (e.g. FDW connections).

One way to fix this issue is to close client connections after they encounter some number of checkout failure. This will force the client to hit the Network load balancer again, land on a different process/container, try to checkout a connection on the new process/container. if it fails, it is disconnected and tries with another one.

This mechanism is guaranteed to eventually land on a balanced state where all clients are able to find connections provided that the overall number of connections across all containers matches the number of clients.

I was able to reproduce this issue in a control environment and was able to show this PR is able to fix it.
postgres.js (and likely other drivers) sends Flush between
Parse/Describe and Bind/Execute when describeFirst is enabled
(the default for parameterized non-prepared queries). pgcat was
logging 'Unexpected code: H' and dropping the message, leaving the
client hung waiting for the ParameterDescription/RowDescription
response that never arrived.

Add a Flush arm that drains the buffered extended-protocol
messages into self.buffer (same as Sync), appends the Flush byte,
sends to the server, forwards the server response, and keeps the
server checked out so the subsequent Bind/Execute/Sync can
complete on the same backend.
The previous commit added a Flush arm in the client loop but reused
send_and_receive_loop, which calls Server::recv() — that recv loop
only terminates on ReadyForQuery ('Z'). Servers do NOT send
ReadyForQuery in response to Flush, so the read hung forever.

Add Server::recv_flush_response() that breaks on RowDescription,
NoData, ErrorResponse, or BindComplete (the terminal describe /
bind response markers). Wire it into the Flush arm via a direct
send + recv_flush_response call path.

Verified locally: postgres.js with prepare:false + params now
completes in ~500ms through pgcat (previously hung).
- recv_flush_response: skip body parse for non-S/non-1 codes (5–N byte
  protocol frames; framing byte is enough to dispatch).
- recv_flush_response: replace self.buffer.clone() with std::mem::take
  to avoid one memcpy+alloc per Flush response.
- Merge separate saw_terminal flag into single matches!() check on the
  framing byte.

Net effect on cloudrest describe-heavy workload: removes one full-buffer
clone and one redundant get_u8/get_i32/read_string pass per Flush. At
intra-DC RTT (~0.3ms) and 10k+ qps the saved CPU is the dominant win.
…re-prepare failure

When a checked-out backend rejected re-Parse of a previously-prepared
statement (e.g. after schema-change cleanup, DEALLOCATE, or transient
backend state), the old code removed the entry from the *client*
prepared_statements HashMap and returned Ok(). This left tokio-postgres
clients in an unrecoverable state: the client still believed the
statement was valid (it never received a Close), so its very next Bind
for that name missed in buffer_bind and the whole TCP conn dropped with
'Prepared statement sN doesn't exist'.

For long-lived ETL clients (pgraft's tokio-postgres prepares many shapes
via execute() and caches them per-conn by SQL), this killed every write
inflight on that conn and orphaned any open COPY-IN, leaving
AccessExclusiveLock on temp staging tables until session termination.

New behavior: keep the client cache intact (it mirrors the client's own
view), mark the misbehaving server bad so the pool replaces it, and
propagate PreparedStatementError so the caller can fail this one
operation and retry on a fresh backend.
@ikeboy003
ikeboy003 force-pushed the fix/extended-protocol-flush-and-prepared-cache branch from 2cd3799 to 1ff5d18 Compare October 8, 2026 04:06

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.