fix: Server startup issue for external postgres #38019
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
deploy/docker/fs/opt/appsmith/pg-utils.sh (4)
21-26: Add error handling for get_unix_socket_directory failureThe connection logic improvement looks good, but we should handle potential failures when getting the Unix socket directory.
local host_proxy if [[ "$PG_DB_HOST" == "localhost" || "$PG_DB_HOST" == "127.0.0.1" ]]; then - host_proxy=$(get_unix_socket_directory) + host_proxy=$(get_unix_socket_directory) || { + tlog "Failed to get Unix socket directory" + exit 2 + } else host_proxy=$PG_DB_HOST fi
Line range hint
155-156: Security: Avoid exposing database password in command lineUsing PGPASSWORD in the command line could expose credentials in process listings. Consider using a connection configuration file or PGPASSFILE instead.
- PGPASSWORD=$PG_DB_PASSWORD psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U "$PG_DB_USER" -d "$PG_DB_NAME" -c "CREATE SCHEMA IF NOT EXISTS appsmith;" + # Create temporary pgpass file + echo "*:*:*:$PG_DB_USER:$PG_DB_PASSWORD" > ~/.pgpass.tmp + chmod 600 ~/.pgpass.tmp + PGPASSFILE=~/.pgpass.tmp psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U "$PG_DB_USER" -d "$PG_DB_NAME" -c "CREATE SCHEMA IF NOT EXISTS appsmith;" + rm ~/.pgpass.tmp
Line range hint
158-159: Refactor: Extract pg_trgm extension creation to a functionThe pg_trgm extension creation is duplicated for local and remote connections. Consider extracting it to a separate function.
+create_pg_trgm_extension() { + local host=$1 + local port=$2 + local user=$3 + local db=$4 + local conn_type=$5 + + if [[ "$conn_type" == "local" ]]; then + local socket_dir=$(get_unix_socket_directory) + psql -h "$socket_dir" -p "$port" -U "$POSTGRES_ADMIN_USER" -d "$db" \ + -c "CREATE EXTENSION IF NOT EXISTS pg_trgm;" + else + PGPASSFILE=~/.pgpass.tmp psql -h "$host" -p "$port" -U "$user" -d "$db" \ + -c "CREATE EXTENSION IF NOT EXISTS pg_trgm;" + fi +}
Line range hint
161-167: Improve error handling consistencyThe schema creation success check should be specific to each path (local/remote) as they have different error conditions and requirements.
- # Check if the schema creation was successful - if [ $? -eq 0 ]; then - echo "Schema 'appsmith' created or already exists." - else - echo "Failed to create schema 'appsmith'." - exit 1 - fi + check_schema_exists() { + local schema_exists + if [[ "$PG_DB_HOST" == "localhost" || "$PG_DB_HOST" == "127.0.0.1" ]]; then + schema_exists=$(psql -h "$unix_socket_directory" -p "$PG_DB_PORT" -U "$POSTGRES_ADMIN_USER" \ + -d "$PG_DB_NAME" -tAc "SELECT 1 FROM information_schema.schemata WHERE schema_name='appsmith'") + else + schema_exists=$(PGPASSFILE=~/.pgpass.tmp psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" \ + -U "$PG_DB_USER" -d "$PG_DB_NAME" -tAc "SELECT 1 FROM information_schema.schemata WHERE schema_name='appsmith'") + fi + + if [ "$schema_exists" != "1" ]; then + echo "Failed to create schema 'appsmith'." + exit 1 + fi + echo "Schema 'appsmith' created or already exists." + } + + check_schema_exists
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
deploy/docker/fs/opt/appsmith/pg-utils.sh(1 hunks)
🔇 Additional comments (1)
deploy/docker/fs/opt/appsmith/pg-utils.sh (1)
Line range hint 1-1: Verify external PostgreSQL connectivity
The changes look good for handling external PostgreSQL connections. Let's verify the connectivity handling:
✅ Verification successful
Let me analyze the actual implementation of these files to ensure proper connection handling across all PostgreSQL-related scripts.
External PostgreSQL connectivity handling is properly implemented
The implementation correctly handles both local and remote PostgreSQL connections with proper error handling and security measures:
- Comprehensive connection string parsing
- Proper socket directory detection for local connections
- Robust retry mechanism with detailed error states
- Correct schema and permission handling for both local and remote setups
- Well-tested functionality as evidenced by test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any hardcoded PostgreSQL connection strings or host references
rg -l "postgresql://" --type sh
rg -l "localhost:5432|127.0.0.1:5432" --type sh
# Look for any other files that might need similar connection handling updates
fd -t f -e sh | xargs grep -l "psql\|pg_isready"
Length of output: 796
Script:
#!/bin/bash
# Check the content of pg-utils.sh and related test file
cat deploy/docker/fs/opt/appsmith/pg-utils.sh
echo "---"
cat deploy/docker/tests/test-pg-utils.sh
# Check if entrypoint.sh and pg-upgrade.sh have proper connection handling
cat deploy/docker/fs/opt/appsmith/entrypoint.sh | grep -A 5 -B 5 "psql\|pg_isready"
echo "---"
cat deploy/docker/fs/opt/appsmith/pg-upgrade.sh | grep -A 5 -B 5 "psql\|pg_isready"
Length of output: 12503
Description
We introduced the auth for embedded Postgres, but in pg-utils we missed on updating the pg_isready condition to resort to db host in case of external postgres url. This caused the pg_isready to never pass when user configure external postgres url as a persistence DB in Appsmith. This PR checks if the local postgres url is configured and accordingly modifies the pg_isready command to target the embedded host vs external host.
Why it did not get caught in our testing?
We are still investigating this but in local for some reason even if we opt for external Postgres DB embedded Postgres process is also initiating which runs the pg_isready against the embedded Postgres and startup goes through.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes