feat: Support all postgres version + use server postgres version in client tools #2

CuriousLearner · 2025-01-05T02:10:20Z

Fixes #1

ipmb · 2025-01-06T16:36:00Z

postgres/bin/dump-to-s3.sh

 set -euf -o pipefail

 cleanup() { rv=$?; if [ -f /tmp/db.dump ]; then shred -u /tmp/db.dump; fi; exit $rv; }
 trap cleanup EXIT

-NAME=${2:-$NAME}
-CONNECT_DB_URL="postgres://$USER@$HOST:$PORT/$NAME"
+# Extract database name from DATABASE_URL if not provided as an argument


Why was this change needed?

I thought that having one variable as opposed to several ones will be easier. Plus, we try to extract the database name directly from the URL if it's not passed as an argument.

I'm not following. Where are we using several variables?

@ipmb: The idea behind this change was to simplify things by combining multiple variables (like USER, HOST, PORT, etc.) into one CONNECT_DB_URL. This makes the code easier to manage and avoids dealing with too many individual variables.

The part where we extract the database name from DATABASE_URL is just a fallback in case it's not passed explicitly, so it ensures everything still works without needing extra inputs.

If something's unclear or there’s a better way to handle this, happy to discuss!

ipmb · 2025-01-06T16:36:12Z

postgres/Dockerfile

@@ -1,9 +1,32 @@
-FROM --platform=linux/amd64 python:3.12-alpine3.19
+FROM ubuntu:22.04


please use the latest debian slim

ipmb · 2025-01-06T16:36:52Z

postgres/bin/dump-to-s3.sh

+
+if ! command -v "$PG_DUMP" &>/dev/null; then
+  echo "Error: pg_dump for version $SERVER_VERSION is not installed." >&2
+  exit 1


rather than failing hard here, let's make this a warning and default to the latest version

ipmb · 2025-01-06T16:38:08Z

postgres/bin/dump-to-s3.sh

+CONNECT_DB_URL="${DATABASE_URL%/*}/$DBNAME"
+
+# Get PostgreSQL server version
+SERVER_VERSION=$(psql "$CONNECT_DB_URL" -tAc "SHOW server_version;" | cut -d '.' -f 1)


does it make sense to run this in the entrypoint?

Thanks! I moved this to the entrypoint.sh script.

ipmb · 2025-01-07T19:54:23Z

postgres/bin/entrypoint.sh

+  echo "  PORT=$PORT"
+  echo "  NAME=$NAME"
+  echo "  USER=$USER"
+


I'd prefer not to keep this DEBUG output in the final PR.

Sorry, this is still in progress, because CI was failing. I'll fix this up.

This is fixed now.

ipmb · 2025-01-07T19:55:34Z

postgres/bin/dump-to-s3.sh

 set -euf -o pipefail

 cleanup() { rv=$?; if [ -f /tmp/db.dump ]; then shred -u /tmp/db.dump; fi; exit $rv; }
 trap cleanup EXIT

-NAME=${2:-$NAME}
-CONNECT_DB_URL="postgres://$USER@$HOST:$PORT/$NAME"
+# Extract database name from DATABASE_URL if not provided as an argument


I'm not following. Where are we using several variables?

This reverts commit f7813c5.

ipmb

Looks good. Left a few minor comments. Can you also add docs on how to update when new PG versions are released?

ipmb · 2025-01-27T23:07:53Z

postgres/Dockerfile

+        ln -s /usr/lib/postgresql/$version/bin/pg_dump /usr/bin/pg_dump-$version && \
+        ln -s /usr/lib/postgresql/$version/bin/pg_restore /usr/bin/pg_restore-$version; \
+    done && \
+    # Use a virtual environment to install Python packages


No need for the virtualenv here. You can install on the system. There may be an apt installable version.

I think there was an issue when I was trying to install it system-wide (but I might be wrong since it has been some time I did this). I'll give it another try.

ipmb · 2025-01-27T23:09:15Z

postgres/bin/dump-to-s3.sh

+CONNECT_DB_URL="${DATABASE_URL%/*}/$DBNAME"
+
+# Use SERVER_VERSION set by the entrypoint
+PG_DUMP="pg_dump-$SERVER_VERSION"


Can you make this work if $SERVER_VERSION is not supplied? It can default to the latest version.

CuriousLearner · 2025-01-27T23:39:46Z

@ipmb Added the docs here: postgres/docs/support-newer-postgres-versions.md

ipmb · 2025-01-31T20:15:12Z

postgres/bin/entrypoint.sh

+
+  echo "Waiting for PostgreSQL server to be ready..."
+  until psql "$DATABASE_URL" -c '\q' 2>/dev/null || [ "$retries" -eq 0 ]; do
+    echo "PostgreSQL is unavailable - sleeping ($((retries--)) retries left)..."


remove " sleeping " please

ipmb · 2025-01-31T20:15:39Z

postgres/bin/entrypoint.sh

+  done
+
+  if [ "$retries" -eq 0 ]; then
+    echo "ERROR: PostgreSQL server did not become ready in time."


ERROR: PostgreSQL server did not respond.

ipmb · 2025-01-31T20:15:57Z

postgres/bin/entrypoint.sh

+    exit 1
+  fi
+
+  echo "PostgreSQL server is ready!"


Remove this one please

ipmb · 2025-01-31T20:17:30Z

postgres/docs/support-newer-postgres-versions.md

+   and running the script against a PostgreSQL 18 server.
+3. Ensure fallback compatibility:
+   Test the script with no `SERVER_VERSION` set to confirm it falls back to the latest version.
+4. Update `dump-to-s3.sh` script's `SERVER_VERSION` default value if the new version becomes the default:


It's referenced in load-to-s3.sh too

ipmb · 2025-01-31T20:17:52Z

postgres/docs/support-newer-postgres-versions.md

+   Verify that the new version works by setting `SERVER_VERSION` to the new version (e.g., `18`)
+   and running the script against a PostgreSQL 18 server.
+3. Ensure fallback compatibility:
+   Test the script with no `SERVER_VERSION` set to confirm it falls back to the latest version.


Can the automated tests do 2 & 3?

CuriousLearner added 2 commits January 5, 2025 07:33

feat: add all postgres client versions

e89bbd6

fix: Update base image to 22.04

83a37a8

CuriousLearner requested a review from ipmb January 5, 2025 02:10

ipmb requested changes Jan 6, 2025

View reviewed changes

CuriousLearner added 4 commits January 7, 2025 23:37

Address review comments

fdcc9e3

chore: fix Dockerfile + use venv

4d22320

fix path for aws

e3e6678

add debugging on db creds setup

e550024

ipmb requested changes Jan 7, 2025

View reviewed changes

CuriousLearner added 5 commits January 9, 2025 01:55

Fix mysql tests

03e8f8f

Modify github actions for postgres

f7813c5

Revert "Modify github actions for postgres"

ea4ff7f

This reverts commit f7813c5.

fix: Wait for DB to detect psql version

b9c6bd3

Add postgres init script for db auth

7f2192d

CuriousLearner requested a review from ipmb January 27, 2025 22:46

ipmb requested changes Jan 27, 2025

View reviewed changes

CuriousLearner added 3 commits January 28, 2025 04:57

Address review comments

ef84482

minor fix

e688af4

remove externally-managed

1f4f4e5

CuriousLearner requested a review from ipmb January 27, 2025 23:37

Update docs

89cddda

ipmb reviewed Jan 31, 2025

View reviewed changes

postgres/bin/entrypoint.sh

exit 1

fi

echo "PostgreSQL server is ready!"

Copy link

Member

ipmb Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this one please

ipmb reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support all postgres version + use server postgres version in client tools #2

feat: Support all postgres version + use server postgres version in client tools #2

CuriousLearner commented Jan 5, 2025

ipmb Jan 6, 2025

CuriousLearner Jan 7, 2025

ipmb Jan 7, 2025

CuriousLearner Jan 27, 2025

ipmb Jan 6, 2025

ipmb Jan 6, 2025

CuriousLearner Jan 7, 2025

ipmb Jan 6, 2025

CuriousLearner Jan 7, 2025

ipmb Jan 7, 2025

CuriousLearner Jan 7, 2025

CuriousLearner Jan 27, 2025

ipmb Jan 7, 2025

ipmb left a comment

ipmb Jan 27, 2025

CuriousLearner Jan 27, 2025

ipmb Jan 27, 2025

CuriousLearner commented Jan 27, 2025 •

edited

Loading

ipmb Jan 31, 2025

ipmb Jan 31, 2025

ipmb Jan 31, 2025

ipmb Jan 31, 2025

ipmb Jan 31, 2025

		@@ -1,9 +1,32 @@
		FROM --platform=linux/amd64 python:3.12-alpine3.19
		FROM ubuntu:22.04

feat: Support all postgres version + use server postgres version in client tools #2

Are you sure you want to change the base?

feat: Support all postgres version + use server postgres version in client tools #2

Conversation

CuriousLearner commented Jan 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousLearner commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousLearner commented Jan 27, 2025 •

edited

Loading