Replace Gunicorn with Granian to improve pgAdmin 4 performance#9786
Replace Gunicorn with Granian to improve pgAdmin 4 performance#9786Smartappli wants to merge 10 commits into
Conversation
Updated granian package version from 2.2.5 to 2.7.2 in Dockerfile.
…anian Replace Gunicorn with Granian in Docker runtime
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace Gunicorn with Granian in the container: update Docker build to install Granian and stop copying Gunicorn config, remove the Gunicorn config module, adapt the entrypoint to emit Granian CLI args (bind, TLS, access logging, threads/workers), and tighten CSRF test request context environ overrides. ChangesGranian runtime switch
Sequence Diagram(s)sequenceDiagram
participant Entrypoint as Entrypoint Script
participant Granian as /venv/bin/granian
participant App as run_pgadmin:app (WSGI)
participant Client as Client (HTTP)
Entrypoint->>Granian: build CLI args (workers 1, --blocking-threads, ${BIND_ARGS}, ${TLS_ARGS}, ${ACCESS_LOG_ARGS})\nexec granian run_pgadmin:app
Granian->>App: load WSGI app
Client->>Granian: request (TCP host:port or UDS)
Granian->>App: forward request (WSGI call)
App-->>Granian: response
Granian-->>Client: send response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/docker/entrypoint.sh (2)
204-204: Environment variable naming: Consider documenting backward compatibility.The script retains
GUNICORN_THREADSandGUNICORN_ACCESS_LOGFILEenvironment variable names for backward compatibility, which is good. However, users may be confused by Gunicorn-prefixed variables when running Granian.Consider adding a comment explaining this is for backward compatibility, or adding support for new
GRANIAN_*aliases in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/docker/entrypoint.sh` at line 204, Add a short inline comment above the exec line explaining that GUNICORN_* vars (e.g., GUNICORN_THREADS and GUNICORN_ACCESS_LOGFILE) are kept for backward compatibility when running Granian, and optionally add support for GRanian-prefixed aliases by reading GRANIAN_THREADS and GRANIAN_ACCESS_LOGFILE (fallback to GUNICORN_* if unset) before composing ACCESS_LOG_ARGS, TLS_ARGS, and BIND_ARGS so users can use either naming convention.
141-143: Stale comment references Gunicorn.The comment on line 141 still mentions "Gunicorn" but the server has been changed to Granian.
📝 Suggested fix
- # Initialize DB before starting Gunicorn + # Initialize DB before starting Granian # Importing pgadmin4 (from this script) is enough /venv/bin/python3 run_pgadmin.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/docker/entrypoint.sh` around lines 141 - 143, Update the stale comment that mentions "Gunicorn" to reference the current server "Granian" instead; locate the comment above the run_pgadmin.py invocation (the line commenting DB initialization) and change the wording from "Initialize DB before starting Gunicorn" to something like "Initialize DB before starting Granian" so the comment matches the actual server being used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/docker/entrypoint.sh`:
- Around line 192-196: The current branch only enables access logs when
GUNICORN_ACCESS_LOGFILE equals "-" which breaks support for file paths; update
the logic around the GUNICORN_ACCESS_LOGFILE check so it handles three cases:
"-" -> set ACCESS_LOG_ARGS to enable logging to stdout (e.g., "--access-log"),
empty/unset -> set ACCESS_LOG_ARGS to disable access logging (e.g.,
"--no-access-log"), and any other non-empty value -> treat it as a file path by
setting ACCESS_LOG_ARGS to the flag that enables logging to a file (e.g.,
including the file path) and emit a warning via the existing logger/echo
indicating a custom file path is being used; reference the
GUNICORN_ACCESS_LOGFILE env var and ACCESS_LOG_ARGS variable when making this
change.
---
Nitpick comments:
In `@pkg/docker/entrypoint.sh`:
- Line 204: Add a short inline comment above the exec line explaining that
GUNICORN_* vars (e.g., GUNICORN_THREADS and GUNICORN_ACCESS_LOGFILE) are kept
for backward compatibility when running Granian, and optionally add support for
GRanian-prefixed aliases by reading GRANIAN_THREADS and GRANIAN_ACCESS_LOGFILE
(fallback to GUNICORN_* if unset) before composing ACCESS_LOG_ARGS, TLS_ARGS,
and BIND_ARGS so users can use either naming convention.
- Around line 141-143: Update the stale comment that mentions "Gunicorn" to
reference the current server "Granian" instead; locate the comment above the
run_pgadmin.py invocation (the line commenting DB initialization) and change the
wording from "Initialize DB before starting Gunicorn" to something like
"Initialize DB before starting Granian" so the comment matches the actual server
being used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60c8a8bb-2fcb-494b-8259-8f4f362199d9
📒 Files selected for processing (3)
Dockerfilepkg/docker/entrypoint.shpkg/docker/gunicorn_config.py
💤 Files with no reviewable changes (1)
- pkg/docker/gunicorn_config.py
…rusted-host Fix CSRF test context host overrides
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/docker/entrypoint.sh`:
- Around line 201-203: The TLS branch that sets TLS_ARGS when PGADMIN_ENABLE_TLS
is present currently only adds --ssl-keyfile/--ssl-certificate and relies on
defaults that may raise the minimum protocol to TLS1.3; update entrypoint.sh so
TLS_ARGS also explicitly sets the minimum protocol (e.g., add --ssl-protocol-min
tls1.2) or read a new env var (e.g., PGADMIN_SSL_MIN_PROTOCOL) to allow opt-in
upgrades, ensuring the change is applied where TLS_ARGS is constructed and used.
- Around line 206-207: The entrypoint currently sets Granian's thread pool via
--blocking-threads using the GUNICORN_THREADS env var but omits --backpressure,
so connection admission uses Granian's default; update the exec invocation that
runs granian (the line invoking /venv/bin/granian with --interface, --workers,
--blocking-threads and existing ${ACCESS_LOG_ARGS} ${TLS_ARGS} ${BIND_ARGS}) to
add an explicit --backpressure flag wired to an environment variable (e.g.
${GUNICORN_BACKPRESSURE:-<sensible-default>}) so deployment concurrency tuning
is respected and overload admission is predictable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d180b36-538e-4cb8-9cc8-b3be3b260adc
📒 Files selected for processing (1)
pkg/docker/entrypoint.sh
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/docker/entrypoint.sh`:
- Line 286: The exec line uses unquoted string variables (BIND_ARGS,
ACCESS_LOG_ARGS, TLS_ARGS) which can be word-split or globbed; change the code
that builds these variables to Bash arrays (e.g., declare -a BIND_ARGS
ACCESS_LOG_ARGS TLS_ARGS or use read -r -a) and push each flag/token into those
arrays, then call exec /venv/bin/granian ... --blocking-threads
"${GUNICORN_THREADS:-25}" "${ACCESS_LOG_ARGS[@]}" "${TLS_ARGS[@]}"
"${BIND_ARGS[@]}" run_pgadmin:app so each argument is preserved and not subject
to word-splitting or globbing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c1f1c41-95fa-4bee-be8d-ce400fafcb20
📒 Files selected for processing (2)
Dockerfilepkg/docker/entrypoint.sh
| fi | ||
|
|
||
| # Keep the existing environment variables for backward compatibility. | ||
| exec /venv/bin/granian --interface wsgi --workers 1 --blocking-threads "${GUNICORN_THREADS:-25}" ${ACCESS_LOG_ARGS} ${TLS_ARGS} ${BIND_ARGS} run_pgadmin:app |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the entrypoint.sh file around line 286
if [ -f "pkg/docker/entrypoint.sh" ]; then
wc -l pkg/docker/entrypoint.sh
fiRepository: pgadmin-org/pgadmin4
Length of output: 93
🏁 Script executed:
# Check the file and view the relevant section
cat -n pkg/docker/entrypoint.sh | sed -n '280,295p'Repository: pgadmin-org/pgadmin4
Length of output: 488
🏁 Script executed:
# Also look at the variable assignments mentioned in the patch
cat -n pkg/docker/entrypoint.sh | grep -A 2 "BIND_ARGS\|ACCESS_LOG_ARGS\|TLS_ARGS" | head -40Repository: pgadmin-org/pgadmin4
Length of output: 1095
Convert variable assignments to Bash arrays to prevent unintended word-splitting and globbing.
Line 286 expands BIND_ARGS, ACCESS_LOG_ARGS, and TLS_ARGS as unquoted strings. If environment variables like PGADMIN_LISTEN_ADDRESS or PGADMIN_LISTEN_PORT contain spaces or special characters, the exec command will incorrectly split tokens. Use Bash arrays and expand with "${arr[@]}" instead.
🔧 Suggested hardening patch
-if [ -n "${PGADMIN_ENABLE_SOCK}" ]; then
- BIND_ARGS="--uds /run/pgadmin/pgadmin.sock"
+if [ -n "${PGADMIN_ENABLE_SOCK}" ]; then
+ BIND_ARGS=(--uds /run/pgadmin/pgadmin.sock)
else
- BIND_ARGS="--host ${PGADMIN_LISTEN_ADDRESS:-[::]} --port ${PGADMIN_LISTEN_PORT:-80}"
+ BIND_ARGS=(--host "${PGADMIN_LISTEN_ADDRESS:-[::]}" --port "${PGADMIN_LISTEN_PORT:-80}")
if [ -n "${PGADMIN_ENABLE_TLS}" ]; then
- BIND_ARGS="--host ${PGADMIN_LISTEN_ADDRESS:-[::]} --port ${PGADMIN_LISTEN_PORT:-443}"
+ BIND_ARGS=(--host "${PGADMIN_LISTEN_ADDRESS:-[::]}" --port "${PGADMIN_LISTEN_PORT:-443}")
fi
fi
if [ "${GUNICORN_ACCESS_LOGFILE:--}" = "-" ]; then
- ACCESS_LOG_ARGS="--access-log"
+ ACCESS_LOG_ARGS=(--access-log)
elif [ -n "${GUNICORN_ACCESS_LOGFILE}" ]; then
echo "Warning: GUNICORN_ACCESS_LOGFILE file paths are not supported with Granian. Access logging disabled." >&2
- ACCESS_LOG_ARGS="--no-access-log"
+ ACCESS_LOG_ARGS=(--no-access-log)
else
- ACCESS_LOG_ARGS="--no-access-log"
+ ACCESS_LOG_ARGS=(--no-access-log)
fi
-TLS_ARGS=""
+TLS_ARGS=()
if [ -n "${PGADMIN_ENABLE_TLS}" ]; then
- TLS_ARGS="--ssl-keyfile /certs/server.key --ssl-certificate /certs/server.cert"
+ TLS_ARGS=(--ssl-keyfile /certs/server.key --ssl-certificate /certs/server.cert)
fi
# Keep the existing environment variables for backward compatibility.
-exec /venv/bin/granian --interface wsgi --workers 1 --blocking-threads "${GUNICORN_THREADS:-25}" ${ACCESS_LOG_ARGS} ${TLS_ARGS} ${BIND_ARGS} run_pgadmin:app
+exec /venv/bin/granian --interface wsgi --workers 1 --blocking-threads "${GUNICORN_THREADS:-25}" "${ACCESS_LOG_ARGS[@]}" "${TLS_ARGS[@]}" "${BIND_ARGS[@]}" run_pgadmin:app📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exec /venv/bin/granian --interface wsgi --workers 1 --blocking-threads "${GUNICORN_THREADS:-25}" ${ACCESS_LOG_ARGS} ${TLS_ARGS} ${BIND_ARGS} run_pgadmin:app | |
| if [ -n "${PGADMIN_ENABLE_SOCK}" ]; then | |
| BIND_ARGS=(--uds /run/pgadmin/pgadmin.sock) | |
| else | |
| BIND_ARGS=(--host "${PGADMIN_LISTEN_ADDRESS:-[::]}" --port "${PGADMIN_LISTEN_PORT:-80}") | |
| if [ -n "${PGADMIN_ENABLE_TLS}" ]; then | |
| BIND_ARGS=(--host "${PGADMIN_LISTEN_ADDRESS:-[::]}" --port "${PGADMIN_LISTEN_PORT:-443}") | |
| fi | |
| fi | |
| if [ "${GUNICORN_ACCESS_LOGFILE:--}" = "-" ]; then | |
| ACCESS_LOG_ARGS=(--access-log) | |
| elif [ -n "${GUNICORN_ACCESS_LOGFILE}" ]; then | |
| echo "Warning: GUNICORN_ACCESS_LOGFILE file paths are not supported with Granian. Access logging disabled." >&2 | |
| ACCESS_LOG_ARGS=(--no-access-log) | |
| else | |
| ACCESS_LOG_ARGS=(--no-access-log) | |
| fi | |
| TLS_ARGS=() | |
| if [ -n "${PGADMIN_ENABLE_TLS}" ]; then | |
| TLS_ARGS=(--ssl-keyfile /certs/server.key --ssl-certificate /certs/server.cert) | |
| fi | |
| # Keep the existing environment variables for backward compatibility. | |
| exec /venv/bin/granian --interface wsgi --workers 1 --blocking-threads "${GUNICORN_THREADS:-25}" "${ACCESS_LOG_ARGS[@]}" "${TLS_ARGS[@]}" "${BIND_ARGS[@]}" run_pgadmin:app |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 286-286: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 286-286: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/docker/entrypoint.sh` at line 286, The exec line uses unquoted string
variables (BIND_ARGS, ACCESS_LOG_ARGS, TLS_ARGS) which can be word-split or
globbed; change the code that builds these variables to Bash arrays (e.g.,
declare -a BIND_ARGS ACCESS_LOG_ARGS TLS_ARGS or use read -r -a) and push each
flag/token into those arrays, then call exec /venv/bin/granian ...
--blocking-threads "${GUNICORN_THREADS:-25}" "${ACCESS_LOG_ARGS[@]}"
"${TLS_ARGS[@]}" "${BIND_ARGS[@]}" run_pgadmin:app so each argument is preserved
and not subject to word-splitting or globbing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/docker/entrypoint.sh (1)
262-291:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent word-splitting by converting to Bash arrays.
Lines 262-291 build
BIND_ARGS,ACCESS_LOG_ARGS, andTLS_ARGSas strings, then expand them unquoted on line 291. If environment variables likePGADMIN_LISTEN_ADDRESScontain spaces or shell metacharacters, word-splitting will break the command. A previous review requested converting these to arrays, but the issue remains unaddressed.🛡️ Proposed fix using Bash arrays
if [ -n "${PGADMIN_ENABLE_SOCK}" ]; then - BIND_ARGS="--uds /run/pgadmin/pgadmin.sock" + BIND_ARGS=(--uds /run/pgadmin/pgadmin.sock) else - BIND_ARGS="--host ${PGADMIN_LISTEN_ADDRESS:-[::]} --port ${PGADMIN_LISTEN_PORT:-80}" + BIND_ARGS=(--host "${PGADMIN_LISTEN_ADDRESS:-[::]}" --port "${PGADMIN_LISTEN_PORT:-80}") if [ -n "${PGADMIN_ENABLE_TLS}" ]; then - BIND_ARGS="--host ${PGADMIN_LISTEN_ADDRESS:-[::]} --port ${PGADMIN_LISTEN_PORT:-443}" + BIND_ARGS=(--host "${PGADMIN_LISTEN_ADDRESS:-[::]}" --port "${PGADMIN_LISTEN_PORT:-443}") fi fi if [ "${GUNICORN_ACCESS_LOGFILE:--}" = "-" ]; then - ACCESS_LOG_ARGS="--access-log" + ACCESS_LOG_ARGS=(--access-log) elif [ -n "${GUNICORN_ACCESS_LOGFILE}" ]; then echo "Warning: GUNICORN_ACCESS_LOGFILE file paths are not supported with Granian. Access logging disabled." >&2 - ACCESS_LOG_ARGS="--no-access-log" + ACCESS_LOG_ARGS=(--no-access-log) else - ACCESS_LOG_ARGS="--no-access-log" + ACCESS_LOG_ARGS=(--no-access-log) fi -TLS_ARGS="" +TLS_ARGS=() if [ -n "${PGADMIN_ENABLE_TLS}" ]; then - TLS_ARGS="--ssl-keyfile /certs/server.key --ssl-certificate /certs/server.cert --ssl-protocol-min ${GRANIAN_SSL_PROTOCOL_MIN:-tls1.2}" + TLS_ARGS=(--ssl-keyfile /certs/server.key --ssl-certificate /certs/server.cert --ssl-protocol-min "${GRANIAN_SSL_PROTOCOL_MIN:-tls1.2}") fi -fi # Keep the existing environment variables for backward compatibility. GRANIAN_BACKPRESSURE="${GRANIAN_BACKPRESSURE:-${GUNICORN_THREADS:-25}}" exec /venv/bin/granian --interface wsgi --workers 1 \ --blocking-threads "${GUNICORN_THREADS:-25}" \ --backpressure "${GRANIAN_BACKPRESSURE}" \ - ${ACCESS_LOG_ARGS} ${TLS_ARGS} ${BIND_ARGS} run_pgadmin:app + "${ACCESS_LOG_ARGS[@]}" "${TLS_ARGS[@]}" "${BIND_ARGS[@]}" run_pgadmin:app🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/docker/entrypoint.sh` around lines 262 - 291, The current script builds BIND_ARGS, ACCESS_LOG_ARGS and TLS_ARGS as plain strings and expands them unquoted in the exec invocation (exec ... ${ACCESS_LOG_ARGS} ${TLS_ARGS} ${BIND_ARGS} run_pgadmin:app), which allows word-splitting; change each to a Bash array (e.g., BIND_ARGS=() / BIND_ARGS+=(--host "${PGADMIN_LISTEN_ADDRESS:-[::]}") / BIND_ARGS+=(--port "${PGADMIN_LISTEN_PORT:-80}") ), similarly build ACCESS_LOG_ARGS=() and TLS_ARGS=() using quoted variable expansions for any values, and finally call exec with the arrays expanded as "${ACCESS_LOG_ARGS[@]}" "${TLS_ARGS[@]}" "${BIND_ARGS[@]}" so arguments containing spaces or metacharacters are preserved; ensure you also remove or fix the stray extra fi near TLS_ARGS if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/docker/entrypoint.sh`:
- Line 284: There is an orphaned "fi" at the end of the script that has no
matching "if", which breaks shell parsing; remove the extra "fi" (the one after
the TLS_ARGS block) so the BIND_ARGS, ACCESS_LOG_ARGS and TLS_ARGS if/fi groups
are balanced, ensuring no additional if/fi pairs are introduced or altered in
functions or blocks like the TLS_ARGS handling.
---
Duplicate comments:
In `@pkg/docker/entrypoint.sh`:
- Around line 262-291: The current script builds BIND_ARGS, ACCESS_LOG_ARGS and
TLS_ARGS as plain strings and expands them unquoted in the exec invocation (exec
... ${ACCESS_LOG_ARGS} ${TLS_ARGS} ${BIND_ARGS} run_pgadmin:app), which allows
word-splitting; change each to a Bash array (e.g., BIND_ARGS=() /
BIND_ARGS+=(--host "${PGADMIN_LISTEN_ADDRESS:-[::]}") / BIND_ARGS+=(--port
"${PGADMIN_LISTEN_PORT:-80}") ), similarly build ACCESS_LOG_ARGS=() and
TLS_ARGS=() using quoted variable expansions for any values, and finally call
exec with the arrays expanded as "${ACCESS_LOG_ARGS[@]}" "${TLS_ARGS[@]}"
"${BIND_ARGS[@]}" so arguments containing spaces or metacharacters are
preserved; ensure you also remove or fix the stray extra fi near TLS_ARGS if
present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b7fa7df-935d-43b9-9746-c77b64868cca
📒 Files selected for processing (1)
pkg/docker/entrypoint.sh
This change proposes replacing Gunicorn with Granian as the application server for pgAdmin 4 in order to improve overall performance, scalability, and efficiency.
Granian offers a modern Rust-based runtime with strong ASGI/WSGI support, lower overhead, and better concurrency handling compared to Gunicorn. For pgAdmin 4, this could lead to:
The goal of this change is to modernize the deployment stack while maintaining compatibility with the existing pgAdmin 4 application architecture.
Further benchmarking and validation may be required to confirm gains across different deployment environments.
Summary by CodeRabbit
Chores
Tests