forked from kfzteile24/postgresql-proxy
-
Notifications
You must be signed in to change notification settings - Fork 0
fix(postgresql-proxy): prevent SSL COPY stalls by draining nonblocking reads #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
nik-localstack
wants to merge
2
commits into
master
Choose a base branch
from
unc-472-postgresql-proxy-drain-nonblocking-ssl-reads-to-prevent-copy
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| --container-architecture linux/arm64 | ||
| -P ubuntu-24.04=catthehacker/ubuntu:act-latest |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| name: Run Tests | ||
|
|
||
| on: | ||
| pull_request: | ||
| push: | ||
| branches: | ||
| - master | ||
|
|
||
| jobs: | ||
| tests: | ||
| runs-on: ubuntu-24.04 | ||
|
|
||
| services: | ||
| postgres: | ||
| image: postgres:16 | ||
| env: | ||
| POSTGRES_USER: postgres | ||
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_DB: postgres | ||
| ports: | ||
| - 5432:5432 | ||
| options: >- | ||
| --health-cmd "pg_isready -U postgres" | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 5 | ||
|
|
||
| env: | ||
| E2E_PG_HOST: 127.0.0.1 | ||
| E2E_PG_PORT: 5432 | ||
| E2E_PG_USER: postgres | ||
| E2E_PG_PASSWORD: postgres | ||
| E2E_PG_DB: postgres | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.13" | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y postgresql-client | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| pip install pytest pytest-timeout | ||
|
|
||
| - name: Run tests | ||
| run: | | ||
| python -m pytest -vv | ||
|
|
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,73 @@ | ||
| VENV_DIR ?= .venv | ||
| VENV_RUN = . $(VENV_DIR)/bin/activate | ||
| PIP_CMD ?= pip | ||
| PYTHON_CMD ?= python | ||
| TEST_DEPS ?= pytest pytest-timeout | ||
| LINT_DEPS ?= ruff | ||
|
|
||
| PG_TEST_CONTAINER ?= pg-proxy-local-tests | ||
| PG_TEST_IMAGE ?= postgres:16 | ||
| PG_TEST_PORT ?= 55432 | ||
| PG_TEST_USER ?= postgres | ||
| PG_TEST_PASSWORD ?= postgres | ||
| PG_TEST_DB ?= postgres | ||
| ACT_CMD ?= act | ||
| ACT_WORKFLOW ?= .github/workflows/tests.yml | ||
| ACT_JOB ?= tests | ||
| ACT_PULL ?= false | ||
| ACT_CONTAINER_ARCH ?= linux/arm64 | ||
|
|
||
| usage: ## Show this help | ||
| @fgrep -h "##" $(MAKEFILE_LIST) | fgrep -v fgrep | sed -e 's/\\$$//' | sed -e 's/##//' | ||
|
|
||
| install: ## Install dependencies in local virtualenv folder | ||
| (test `which virtualenv` || $(PIP_CMD) install --user virtualenv) && \ | ||
| (test `which virtualenv` || $(PIP_CMD) install virtualenv) && \ | ||
| (test -e $(VENV_DIR) || virtualenv $(VENV_OPTS) $(VENV_DIR)) && \ | ||
| ($(VENV_RUN) && $(PIP_CMD) install --upgrade pip) && \ | ||
| (test ! -e requirements.txt || ($(VENV_RUN); $(PIP_CMD) install -r requirements.txt)) | ||
|
|
||
| publish: ## Publish the library to the central PyPi repository | ||
| ($(VENV_RUN); pip install twine; python ./setup.py sdist && twine upload dist/*) | ||
|
|
||
| .PHONY: usage install clean publish test lint | ||
| install-test: install ## Install test dependencies in local virtualenv | ||
| ($(VENV_RUN); $(PIP_CMD) install $(TEST_DEPS)) | ||
|
|
||
| install-lint: install ## Install lint dependencies in local virtualenv | ||
| ($(VENV_RUN); $(PIP_CMD) install $(LINT_DEPS)) | ||
|
|
||
| lint: install-lint ## Format code with ruff | ||
| $(VENV_DIR)/bin/ruff format postgresql_proxy tests plugins | ||
|
|
||
| test: ## Start local PostgreSQL container and run all tests | ||
| @set -euo pipefail; \ | ||
| cleanup() { docker rm -f $(PG_TEST_CONTAINER) >/dev/null 2>&1 || true; }; \ | ||
| trap cleanup EXIT INT TERM; \ | ||
| docker rm -f $(PG_TEST_CONTAINER) >/dev/null 2>&1 || true; \ | ||
| docker run --name $(PG_TEST_CONTAINER) \ | ||
| -e POSTGRES_USER=$(PG_TEST_USER) \ | ||
| -e POSTGRES_PASSWORD=$(PG_TEST_PASSWORD) \ | ||
| -e POSTGRES_DB=$(PG_TEST_DB) \ | ||
| -p $(PG_TEST_PORT):5432 \ | ||
| -d $(PG_TEST_IMAGE) >/dev/null; \ | ||
| for i in $$(seq 1 45); do \ | ||
| if docker exec $(PG_TEST_CONTAINER) pg_isready -U $(PG_TEST_USER) >/dev/null 2>&1; then \ | ||
| echo "PostgreSQL ready on 127.0.0.1:$(PG_TEST_PORT)"; \ | ||
| break; \ | ||
| fi; \ | ||
| sleep 1; \ | ||
| done; \ | ||
| if ! docker exec $(PG_TEST_CONTAINER) pg_isready -U $(PG_TEST_USER) >/dev/null 2>&1; then \ | ||
| echo "PostgreSQL did not become ready in time"; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| E2E_PG_HOST=127.0.0.1 \ | ||
| E2E_PG_PORT=$(PG_TEST_PORT) \ | ||
| E2E_PG_USER=$(PG_TEST_USER) \ | ||
| E2E_PG_PASSWORD=$(PG_TEST_PASSWORD) \ | ||
| E2E_PG_DB=$(PG_TEST_DB) \ | ||
| $(VENV_DIR)/bin/$(PYTHON_CMD) -m pytest -vv | ||
|
|
||
| test-act: ## Run the CI test workflow locally with act | ||
| $(ACT_CMD) -W $(ACT_WORKFLOW) -j $(ACT_JOB) --pull=$(ACT_PULL) --container-architecture $(ACT_CONTAINER_ARCH) | ||
|
|
||
| .PHONY: usage install install-test install-lint clean publish test test-act lint |
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
This file was deleted.
Oops, something went wrong.
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| # Testing Guide | ||
|
|
||
| All tests in this repo require a real PostgreSQL server and are organized at the top level: | ||
|
|
||
| - `test_proxy.py`: proxy behavior tests (connection, SSL, hang regressions) | ||
| - `test_plugins.py`: plugin integration tests (HLL rewrite behavior) | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| - Python `3.13` (same version as CI) | ||
| - Docker (for local disposable PostgreSQL) | ||
| - `psql` (`postgresql-client`) | ||
| - `openssl` (SSL tests generate a temporary self-signed cert/key at runtime) | ||
| - `act` (optional, for local GitHub Actions runs) | ||
|
|
||
| Install Python deps in the project virtualenv: | ||
|
|
||
| ```bash | ||
| make install-test | ||
| ``` | ||
|
|
||
| ## Which command should I use? | ||
|
|
||
| - Fastest full local run with disposable Postgres: `make test` | ||
| - Run only proxy tests (using your own Postgres): `python -m pytest tests/test_proxy.py -vv` | ||
| - Run only plugin tests: `python -m pytest tests/test_plugins.py -vv` | ||
| - Run exact CI workflow locally: `make test-act` | ||
|
|
||
| ## 1) Full local suite (recommended) | ||
|
|
||
| `make test` starts a temporary PostgreSQL container, waits for readiness, sets DB env vars, then runs: | ||
|
|
||
| ```bash | ||
| python -m pytest -vv | ||
| ``` | ||
|
|
||
| Use it when you want one command that matches normal contributor workflow. | ||
|
|
||
| ```bash | ||
| make test | ||
| ``` | ||
|
|
||
| ## 2) DB-backed proxy tests against an existing PostgreSQL | ||
|
|
||
| If you already have PostgreSQL running, set connection env vars and run only proxy tests: | ||
|
|
||
| ```bash | ||
| export E2E_PG_HOST=127.0.0.1 | ||
| export E2E_PG_PORT=5432 | ||
| export E2E_PG_USER=postgres | ||
| export E2E_PG_PASSWORD=postgres | ||
| export E2E_PG_DB=postgres | ||
| python -m pytest tests/test_proxy.py -vv | ||
| ``` | ||
|
|
||
| If PostgreSQL is not reachable, tests fail fast at startup. | ||
|
|
||
| ## 3) Plugin integration tests | ||
|
|
||
| ```bash | ||
| python -m pytest tests/test_plugins.py -vv | ||
| ``` | ||
|
|
||
| Requires PostgreSQL to be running with the `E2E_PG_*` env vars set (see section 2). | ||
|
|
||
| ## 4) Run the GitHub workflow locally (`act`) | ||
|
|
||
| ```bash | ||
| make test-act | ||
| ``` | ||
|
|
||
| Useful overrides: | ||
|
|
||
| ```bash | ||
| # Refresh workflow images | ||
| make test-act ACT_PULL=true | ||
|
|
||
| # Match GitHub x86_64 runner architecture (slower on Apple Silicon) | ||
| make test-act ACT_CONTAINER_ARCH=linux/amd64 | ||
| ``` |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import os | ||
|
|
||
| import psycopg2 | ||
| import pytest | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| def postgres_settings(): | ||
| """PostgreSQL connection settings from environment or defaults.""" | ||
| return { | ||
| "host": os.environ.get("E2E_PG_HOST", "127.0.0.1"), | ||
| "port": int(os.environ.get("E2E_PG_PORT", "5432")), | ||
| "user": os.environ.get("E2E_PG_USER", "postgres"), | ||
| "password": os.environ.get("E2E_PG_PASSWORD", "postgres"), | ||
| "dbname": os.environ.get("E2E_PG_DB", "postgres"), | ||
| } | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session", autouse=True) | ||
| def ensure_postgres_available(postgres_settings): | ||
| """Ensure PostgreSQL backend is available before running any tests.""" | ||
| try: | ||
| with psycopg2.connect( | ||
| connect_timeout=3, sslmode="disable", **postgres_settings | ||
| ) as conn: | ||
| with conn.cursor() as cur: | ||
| cur.execute("SELECT 1") | ||
| assert cur.fetchone() == (1,) | ||
| except Exception as err: # pragma: no cover - environment dependent | ||
| pytest.fail( | ||
| f"PostgreSQL backend is required for tests but is not reachable: {err}" | ||
| ) | ||
|
|
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know this is from the code I shared last week, but now that I read it, I don't think the min comparison is really necessary since the recv method already works that it grabs what is present until value provided in arg. We can probably safely achieve the same result with
extra = sock.recv(4096).