Add replication capabilities

[atodorov]

No description provided.

[atodorov]

Example #1, keeping reference of AI prompts:

Notes:

• 2 relevant comments (different directories)
• one irrelevant b/c no branch-protection rules configured (agent doesn't have perms to inspect), nor required on the PR (clearly visible, no perms needed):

[atodorov]

Example #2:

resulting in:

---

AI forgot to mark comments as resolved:

NOTE: GitHub marks them as "Outdated" when the code/line changes not as resolved!

[atodorov]

Example #3 - same review asking Kimi K3 (performed after the 2nd commit which introduces exec):

resulting in the review immediately below

[atodorov]

Left a few inline comments. Overall the approach works, but there are a couple of correctness concerns worth addressing before this merges: (1) the replication slot name is derived from POSTGRES_REPLICATION_USER, which will collide if two replicas use the same user; (2) pg_basebackup runs before docker-entrypoint.sh gets a chance to configure the instance, so the replica inherits whatever defaults pg_basebackup writes but the initdb.d scripts never run on the replica's data dir; (3) the test hard-codes two sleep 120 waits which will make CI slow and flaky. Details inline.

[atodorov]

Using $POSTGRES_REPLICATION_USER as the replication slot name means the slot name is the same for every replica that connects with this user. If you ever spin up a second replica of postgres_17 (or tear one down and bring up a new one while the old slot still exists on the primary), pg_basebackup --create-slot will fail with "replication slot already exists", and worse, the old slot will keep retaining WAL on the primary even after the replica is gone. Consider using a dedicated slot-name variable (e.g. POSTGRES_REPLICATION_SLOT) defaulting to something unique per replica (hostname or container name), and document that operators must drop stale slots on the primary.

[atodorov]

Because this block runs before docker-entrypoint.sh, pg_basebackup populates $PGDATA directly and the standard entrypoint's initdb logic will see an existing PG_VERSION and skip docker-entrypoint-initdb.d entirely. That's probably what you want for a replica (the data comes from the primary), but it also means postgresql.conf/pg_hba.conf tuning done in initdb scripts (like 00_enable_ssl.sh) never runs against the replica's freshly-synced config. Since --write-recovery-conf only appends replication settings, the replica ends up with whatever config was baked into the primary backup. Worth a comment here making that explicit, or confirming that the base image's config is sufficient for the replica to actually serve SSL connections.

[atodorov]

sslmode=require here hard-fails if the primary doesn't have SSL configured. For a replication connection that's fine in this repo (SSL is forced), but the connection string is built by directly interpolating $POSTGRES_REPLICATION_PASSWORD into a URI — if the password ever contains @, : or / this will silently produce a malformed DSN. Consider passing credentials via PGPASSWORD and --host/--username flags instead of embedding them in the URL.

[atodorov]

Same slot-name collision concern as in 17/entrypoint.sh — slot is named after the replication user, so multiple replicas sharing the user will conflict.

[atodorov]

Two fixed sleep 120 calls add ~4 minutes to every CI run and will still be flaky on a slow runner (pg_basebackup of a freshly-initialised Kiwi TCMS DB is usually fast, but not guaranteed). Poll instead: loop on docker logs replica_17 | grep -q 'completed initial wal sync' (or check pg_isready / SELECT pg_is_in_recovery() inside the container) with a timeout, so the test proceeds as soon as the replica is actually ready.

[atodorov]

The replicas are started with docker run against the compose-created network, but they never get cleaned up if an earlier phase fails — rlPhaseStartCleanup only runs on the happy path in some beakerlib configs. Consider registering the replica containers for cleanup the same way the compose stack is, or at least docker rm -f replica_17 replica_18 defensively at the start of the phase in case a previous run left them behind.

[atodorov]

Both replication users are created with the same hard-coded password replicate-me, and the file is mounted into the primary unconditionally. Anyone who can reach the primary's port 5432 now has replication credentials baked into the image's compose setup. At minimum, source these from environment variables (e.g. POSTGRES_REPLICATION_PASSWORD_RPL_USR_17) so production deployments aren't stuck with a password that's committed to git; and consider scoping the users' pg_hba.conf entries to the replica subnet rather than relying on the default.

[atodorov]

Pinning the network name to postgres_default makes the test's --network=postgres_default work, but it also means this compose project can no longer be run twice side-by-side on the same host (both will try to claim the same network) and it overrides compose's project-name isolation. Since the test already knows the compose project, it could discover the network dynamically (docker network ls --filter name=_default) or the replicas could be added to the compose file under a profile instead — either avoids hard-coding the name.

[atodorov]

Example #4 , same review with Claude Opus 5:

---

results in review immediately below; not inline like others; took a lot more time & $
!!! cloned the repo + the base repo ; missed the assert portion in tests!

[atodorov]

Reviewed at 6b95d40. The overall shape is right: wrap the upstream entrypoint, bootstrap with pg_basebackup --write-recovery-conf, and exec docker-entrypoint.sh so signals still work. A few things I think are blocking before this leaves draft, then some smaller ones.

Blocking

1. entrypoint.sh has no set -e, so a failed base backup still starts Postgres

17/entrypoint.sh:1-34 runs without set -euo pipefail and never checks the exit status of pg_basebackup. If the backup fails halfway (primary not up yet, auth failure, network blip), the script prints INFO: completed initial wal sync regardless and falls through to exec docker-entrypoint.sh. At that point $PGDATA contains a partial data directory, and upstream's docker_setup_env decides what to do based purely on whether $PGDATA/PG_VERSION exists. Either outcome is bad: a half-copied cluster that Postgres tries to start, or a re-initdb on top of backup debris.

I'd make the failure explicit:

if ! pg_basebackup ... ; then
    echo "ERROR: initial wal sync failed"
    exit 3
fi

Adding set -euo pipefail at the top is worth it too, but note it interacts with item 2.

2. The env var guards read unset variables

17/entrypoint.sh:9, :13, :18 dereference $POSTGRES_REPLICATION_USER, $POSTGRES_REPLICATION_PASSWORD and $POSTGRES_PRIMARY_HOST bare. That is fine today only because there is no set -u; the moment you add it, every non-replica container dies at line 9. Use ${POSTGRES_REPLICATION_USER:-} and friends, which also lets you turn on set -u safely.

3. The replication password gets written into postgresql.auto.conf in cleartext

--write-recovery-conf copies the connection string it was given into primary_conninfo, password included. So $PGDATA/postgresql.auto.conf on every replica ends up holding password=..., and it is also visible in ps inside the container while the backup runs. Passing it out of band avoids both:

export PGPASSWORD="$POSTGRES_REPLICATION_PASSWORD"
pg_basebackup --dbname "postgres://$POSTGRES_REPLICATION_USER@$POSTGRES_PRIMARY_HOST/postgres?sslmode=require" ...

pg_basebackup will pick up PGPASSWORD, and it will not end up in primary_conninfo. The replica then needs PGPASSWORD or a .pgpass/PGPASSFILE at runtime for the walreceiver to reconnect, so this trades one problem for a bit of config, but cleartext credentials sitting in the data directory are the worse half of that trade.

4. --create-slot makes re-bootstrapping a replica fail

17/entrypoint.sh:28 unconditionally creates a slot named after the replication user. Slots live on the primary and survive the replica. So: replica volume is wiped or the replica is recreated from scratch, the slot is still there, pg_basebackup --create-slot fails with "replication slot already exists", and (given item 1) the container proceeds to start anyway. This is the ordinary disaster-recovery path, not an edge case.

Options: drop --create-slot and provision the slot separately, or tolerate the existing slot instead of hard-failing on it.

Related, and worth at least a comment in the file: a slot whose replica never comes back pins WAL on the primary until the disk fills. max_slot_wal_keep_size on the primary is the usual guard.

Also, keying the slot name off POSTGRES_REPLICATION_USER means one slot per user, so two replicas sharing a user silently collide. A separate POSTGRES_REPLICATION_SLOT (defaulting to the user) would decouple that.

Test coverage

5. "Start replication containers" cannot fail

testing/test_docker.sh:40-48 runs docker run -d, sleeps, and dumps logs. docker run -d returns 0 as long as the container was created; it says nothing about whether the container is still alive 120s later. docker logs is unasserted output. So this phase passes with a replica that exited immediately.

Worth asserting on something real, e.g. rlAssertGrep "completed initial wal sync" against the captured log, the container still being up, and pg_is_in_recovery() returning true on the replica. Checking pg_stat_replication on the primary would confirm streaming actually established, which is the property the whole PR is about.

6. 240 seconds of unconditional sleep

Two sleep 120 calls. On a nearly empty database the base backup finishes in a second or two, and if it is going to fail it usually fails fast. A poll loop on pg_isready (or on the log line from item 5) with a timeout gets you a faster suite and a better failure message than "we waited two minutes and something is off".

7. Cleanup fails when the test aborts early

testing/test_docker.sh:78-82 runs docker kill / docker rm unguarded. If the run dies before the replicas exist, all four commands fail, the cleanup phase is marked FAIL, and runner.sh:34 turns that into exit 22 — masking the original failure. docker rm -f replica_17 || : covers both steps and is safe when the container was never created.

8. Hardcoded postgres-postgres_17:latest

That tag is derived from the Compose project name, which is derived from the directory name. Anyone checking out into a directory not named postgres gets an image-not-found. docker compose images -q postgres_17 resolves it, or pin image: explicitly in docker-compose.yml and reference that.

9. Replica startup is sequenced before the restart/stop/kill phases

The replicas come up at line 40 and the content assertion is at line 69, with compose restart, stop/start and kill/start in between. So the assertion implicitly depends on replicas surviving three primary outages and re-establishing streaming. That is arguably a nice thing to test, but right now it is untested-by-accident rather than deliberate: nothing distinguishes "replication never worked" from "replication broke on reconnect". Consider asserting content once right after bootstrap, then again after the restart cycle.

Smaller things

10. 17/entrypoint.sh and 18/entrypoint.sh are byte-identical. I diffed them, no differences. Two copies that must not drift. The repo already does this with 00_enable_ssl.sh, so it is consistent with what is there, but the maintenance cost doubles with each added file. A shared directory plus a build context at the repo root, or a symlink, would collapse it.

11. 18/Dockerfile is missing the copyright header added to 17/Dockerfile:1-4.

12. mkdir -p/chmod run before the env validation. 17/entrypoint.sh:10-11 create and chmod $PGDATA before checking that the password and host are even set, so the error paths leave a directory behind. Moving the two guards above the mkdir is tidier. Also note upstream's docker_create_db_directories does this same work anyway.

13. pg_basebackup runs as root. The entrypoint starts as root and does not drop privileges, so the copied cluster is root-owned; it works out because upstream's docker_create_db_directories chowns to postgres when uid is 0. That is a load-bearing detail from someone else's script. gosu postgres pg_basebackup ... makes it not matter.

14. networks.default.name: postgres_default (docker-compose.yml:105-107) pins the network name globally so the test can --network=postgres_default. It also removes per-project network isolation, so two checkouts of this repo cannot run side by side. Acceptable for a test rig, just noting the tradeoff.

15. The 01_replication_users.sh mount on postgres_18 looks redundant. postgres_18 extends postgres_17, and Compose merges volumes keyed on the target path, so the mount is inherited. Same for the existing db18_data line, which is there precisely to override by target path.

16. Renaming the workflow jobs breaks required status checks. docker → docker-build and test → end-to-end in .github/workflows/testing.yml. Better names, but if branch protection on main requires the old names, it will block merges until the rules are updated.

17. The three new env vars are undocumented. There is no README in the repo, so there is nowhere obvious to put them, but POSTGRES_REPLICATION_USER / POSTGRES_REPLICATION_PASSWORD / POSTGRES_PRIMARY_HOST and the "set these and the container becomes a replica" behavior need to be written down somewhere for anyone consuming these images.

18. Pre-existing, but adjacent: hostssl replication all all scram-sha-256 in 00_enable_ssl.sh lets any authenticated role attempt replication. Now that there is a dedicated replication user, narrowing that rule to it costs nothing.