Compose the PostgreSQL component instead of including the shared tree #7

Closed

marcos-mendez wants to merge 1 commit from feat/postgresql-as-a-unit into main

marcos-mendez commented

2026-09-27 15:22:13 -03:00

(Migrated from github.com)

The recipe stops including mk/turnkey/pgsql.mk, stops naming the server packages, and composes keel-linux/unit-postgresql as unit.d/postgresql (its pull request 1). Decision 0013 is why: a database is a component, not a parent layer, so LAMP and LAPP can share one apache-php layer.

listen_addresses = '::1,127.0.0.1' moves with the server, into the component. The line was written here last night, when this layer was found binding the IPv4 loopback alone because Debian's /etc/hosts never maps ::1 to localhost. It belongs to whoever installs the server: LAPP will carry the component without carrying this recipe. What stays here is the check that the built image has the line, and the boot test still proves what the cluster binds on a running machine.

The comparison, on the build host

Four builds of bt-layer postgresql --parent core, all at SOURCE_DATE_EPOCH=1700000000, fab 1.1.1+keel2, parent core 7acf2c53, 116 to 126 seconds each:

build	tree	packages	files	symlinks
control	main	430	37,439	3,802
unit	this branch plus `unit.d/postgresql`	430	37,439	3,802
control, second run	main again, the noise floor	430	37,439	3,802
control plus this changelog entry	main plus `changelog` only	430	37,439	3,802

The noise floor is 13 of 37,439 regular files: 6 under /var/lib/postgresql/**, plus /etc/webmin/postgresql/config, /var/webmin/module.infos.cache, /var/log/alternatives.log, /var/log/webmin/webmin.log, /var/cache/ldconfig/aux-cache and the snakeoil key and certificate. All install-time state.

Against the fourth build, which carries this changelog entry and nothing else of this branch:

package list identical, all 430 entries, name and version
symlinks identical, all 3,802, path and target
14 differing regular files against a 13-file noise floor, and exactly one path outside it

That one path is /etc/postgresql/17/main/postgresql.conf, and the difference is one line, the trailing comment of the line the fix writes:

- listen_addresses = '::1,127.0.0.1'		# set by conf.d/main of keel-postgresql: see the comment there
+ listen_addresses = '::1,127.0.0.1'		# set by the postgresql component: see the comment in its conf script

The value is byte identical, pg_hba.conf is identical, and the comment now names the file that writes it. That is the fix surviving the move, measured rather than asserted.

The changelog entry alone accounts for the package line turnkey-postgresql-19.0 2 against 3 and for three files (/usr/share/doc/turnkey-postgresql-19.0/changelog, its md5sums, /var/lib/dpkg/status), which is why the fourth build exists.

The manifest records units postgresql@1.0.0 and build_units postgresql@1.0.0.

Nothing was published: the staged postgresql layer, d5220c56, and its manifest were restored from the backup taken before the first build.

Test plan:

four builds and the comparison above
tests/coverage.sh unchanged by this branch
appliance / build-and-boot, which fetches the published layer

The recipe stops including `mk/turnkey/pgsql.mk`, stops naming the server packages, and composes `keel-linux/unit-postgresql` as `unit.d/postgresql` (its pull request 1). Decision 0013 is why: a database is a component, not a parent layer, so LAMP and LAPP can share one `apache-php` layer. `listen_addresses = '::1,127.0.0.1'` moves with the server, into the component. The line was written here last night, when this layer was found binding the IPv4 loopback alone because Debian's `/etc/hosts` never maps `::1` to `localhost`. It belongs to whoever installs the server: LAPP will carry the component without carrying this recipe. What stays here is the check that the built image has the line, and the boot test still proves what the cluster binds on a running machine. ## The comparison, on the build host Four builds of `bt-layer postgresql --parent core`, all at `SOURCE_DATE_EPOCH=1700000000`, fab 1.1.1+keel2, parent `core` `7acf2c53`, 116 to 126 seconds each: | build | tree | packages | files | symlinks | | --- | --- | --- | --- | --- | | control | main | 430 | 37,439 | 3,802 | | unit | this branch plus `unit.d/postgresql` | 430 | 37,439 | 3,802 | | control, second run | main again, the noise floor | 430 | 37,439 | 3,802 | | control plus this changelog entry | main plus `changelog` only | 430 | 37,439 | 3,802 | The noise floor is **13 of 37,439 regular files**: 6 under `/var/lib/postgresql/**`, plus `/etc/webmin/postgresql/config`, `/var/webmin/module.infos.cache`, `/var/log/alternatives.log`, `/var/log/webmin/webmin.log`, `/var/cache/ldconfig/aux-cache` and the snakeoil key and certificate. All install-time state. Against the fourth build, which carries this changelog entry and nothing else of this branch: - package list **identical**, all 430 entries, name and version - symlinks **identical**, all 3,802, path and target - 14 differing regular files against a 13-file noise floor, and **exactly one** path outside it That one path is `/etc/postgresql/17/main/postgresql.conf`, and the difference is one line, the trailing comment of the line the fix writes: - listen_addresses = '::1,127.0.0.1' # set by conf.d/main of keel-postgresql: see the comment there + listen_addresses = '::1,127.0.0.1' # set by the postgresql component: see the comment in its conf script The value is byte identical, `pg_hba.conf` is identical, and the comment now names the file that writes it. That is the fix surviving the move, measured rather than asserted. The changelog entry alone accounts for the package line `turnkey-postgresql-19.0` 2 against 3 and for three files (`/usr/share/doc/turnkey-postgresql-19.0/changelog`, its `md5sums`, `/var/lib/dpkg/status`), which is why the fourth build exists. The manifest records `units postgresql@1.0.0` and `build_units postgresql@1.0.0`. Nothing was published: the staged `postgresql` layer, `d5220c56`, and its manifest were restored from the backup taken before the first build. Test plan: - [x] four builds and the comparison above - [x] `tests/coverage.sh` unchanged by this branch - [ ] `appliance / build-and-boot`, which fetches the published layer

marcos-mendez commented

2026-09-27 23:56:06 -03:00

(Migrated from github.com)

Independent review, read-only. Verdict: Warning — not mergeable as it stands.
The extraction itself is faithful and decision 0010's prerequisites are genuinely
in place; what is not sound is the evidence around it.

Three merge blockers, shared with the sibling pull request:

The pinned component version does not exist. The Makefile and README
clone --branch v1.0.0, and neither unit repository has any tag or release.
The documented build step fails with Remote branch v1.0.0 not found. Since
decision 0010's assembly step does not exist yet, this clone is the
assembly mechanism. Tag the unit, or pin a commit sha.
The branch is CONFLICTING against main, and the equivalence
measurement was made against a tree that no longer exists. main has since
gained merges that touch Makefile and conf.d/main, including a
root.patched/pre hook on the same target where fab applies the units. All
four builds behind the "0 attributable differences" claim predate that. The
merged tree has never been built.
The changelog revision has been overtaken and would now fail the gate on
rebase. Verified by running the gate's own entry_version/version_increased
against current main.

Two findings about the method, which outlive these pull requests:

The measurement is blind inside /var/lib/mysql/** and
/var/lib/postgresql/**, where only paths are compared and never the nature
of the difference. Those directories hold mysql/global_priv, mysql/user.*
and pg_authid. For a component whose build-time job is deleting accounts,
the one directory the measurement cannot see is the one holding the accounts.
There is no real difference today — the review established that by diffing the
conf scripts against common — but the reported measurement is not what
establishes it.
The measurement is not reproducible. The capture script lives only in a
scratch directory on the build host and the comparison step is committed
nowhere. Decision 0010 step 3 instructs re-running this comparison for every
future component; that instruction has no executable behind it.

Also recorded, and wider than these pull requests: the three green checks
never build the composed layer. appliance / build-and-boot boots the
published layer from the mirror, which on a recipe change is the code before
the change. Both bodies say so honestly with the box unticked. Tracked as
tracker#14.

Duplication has moved rather than gone. common still carries
conf/mysql, conf/pgsql, overlays/mysql, overlays/pgsql and their
makefiles, and they still have consumers — keel-wordpress among them. Nothing
detects divergence between the two copies, and unit-postgresql/conf has
already diverged. Decide whether the common copy is frozen with its consumers
migrating, or whether a check fails when the two drift.

Independent review, read-only. Verdict: **Warning — not mergeable as it stands.** The extraction itself is faithful and decision 0010's prerequisites are genuinely in place; what is not sound is the evidence around it. Three merge blockers, shared with the sibling pull request: 1. **The pinned component version does not exist.** The Makefile and README clone `--branch v1.0.0`, and neither unit repository has any tag or release. The documented build step fails with `Remote branch v1.0.0 not found`. Since decision 0010's assembly step does not exist yet, this clone *is* the assembly mechanism. Tag the unit, or pin a commit sha. 2. **The branch is `CONFLICTING` against `main`, and the equivalence measurement was made against a tree that no longer exists.** `main` has since gained merges that touch `Makefile` and `conf.d/main`, including a `root.patched/pre` hook on the same target where fab applies the units. All four builds behind the "0 attributable differences" claim predate that. The merged tree has never been built. 3. **The changelog revision has been overtaken** and would now fail the gate on rebase. Verified by running the gate's own `entry_version`/`version_increased` against current `main`. Two findings about the method, which outlive these pull requests: - **The measurement is blind inside `/var/lib/mysql/**` and `/var/lib/postgresql/**`**, where only paths are compared and never the nature of the difference. Those directories hold `mysql/global_priv`, `mysql/user.*` and `pg_authid`. For a component whose build-time job is deleting accounts, the one directory the measurement cannot see is the one holding the accounts. There is no real difference today — the review established that by diffing the conf scripts against `common` — but the reported measurement is not what establishes it. - **The measurement is not reproducible.** The capture script lives only in a scratch directory on the build host and the comparison step is committed nowhere. Decision 0010 step 3 instructs re-running this comparison for every future component; that instruction has no executable behind it. Also recorded, and wider than these pull requests: **the three green checks never build the composed layer.** `appliance / build-and-boot` boots the *published* layer from the mirror, which on a recipe change is the code before the change. Both bodies say so honestly with the box unticked. Tracked as tracker#14. **Duplication has moved rather than gone.** `common` still carries `conf/mysql`, `conf/pgsql`, `overlays/mysql`, `overlays/pgsql` and their makefiles, and they still have consumers — `keel-wordpress` among them. Nothing detects divergence between the two copies, and `unit-postgresql/conf` has already diverged. Decide whether the `common` copy is frozen with its consumers migrating, or whether a check fails when the two drift.

marcos-mendez commented

2026-09-27 23:56:08 -03:00

(Migrated from github.com)

One finding specific to this pull request, on top of the shared review comment.

Two build steps are dead on Debian 13, and the test fixture invents the file
that would make them work. unit-postgresql/conf:32 and :35:

sed -i "/^#password_encryption = on/ s/#//"
sed -i "/^shared_buffers =/ s/32/24/"

Against the real article on a PostgreSQL 17 host:

postgresql.conf.sample:97   #password_encryption = scram-sha-256
postgresql.conf.sample:129  #shared_buffers = 128MB

Neither pattern matches. tests/conf.bats then writes a fixture whose comment
claims it is what pg_createcluster leaves behind, containing
#password_encryption = on and shared_buffers = 32MB, and asserts the seds
transformed it. Two of eleven assertions pass against a file the build never
produces, and COVERAGE.md reports 100 percent on that basis.

The image is not wrong: the seds were equally dead in common/conf/pgsql, so
the extraction really is equivalent, and PostgreSQL 17 defaults to
scram-sha-256 regardless — no security regression. What is wrong is that the
suite now certifies dead lines as working. Build the fixture from the installed
postgresql.conf.sample; then either delete the dead seds, which is a change to
the image and needs its own changelog entry, or assert they are no-ops and say
why.

The listen_addresses tests in the same file are the counter-example and the
standard the other two should meet: they drive the real default line, a '*'
line and a missing line, and assert the failure path.

Separately, before LAPP composes this component. The component sets
postgres/postgres by default, and the only removal lives in
keel-postgresql/conf.d/main. By decision 0013 LAPP composes unit-postgresql
onto apache-php, not onto this recipe — so LAPP inherits a well-known
superuser password unless its own conf.d repeats the removal, which is exactly
the copy-paste the split exists to prevent. It is documented in conf-vars but
nothing enforces it. Bind the two halves together rather than documenting the
gap.

One finding specific to this pull request, on top of the shared review comment. **Two build steps are dead on Debian 13, and the test fixture invents the file that would make them work.** `unit-postgresql/conf:32` and `:35`: sed -i "/^#password_encryption = on/ s/#//" sed -i "/^shared_buffers =/ s/32/24/" Against the real article on a PostgreSQL 17 host: postgresql.conf.sample:97 #password_encryption = scram-sha-256 postgresql.conf.sample:129 #shared_buffers = 128MB Neither pattern matches. `tests/conf.bats` then writes a fixture whose comment claims it is what `pg_createcluster` leaves behind, containing `#password_encryption = on` and `shared_buffers = 32MB`, and asserts the seds transformed it. Two of eleven assertions pass against a file the build never produces, and COVERAGE.md reports 100 percent on that basis. The image is not wrong: the seds were equally dead in `common/conf/pgsql`, so the extraction really is equivalent, and PostgreSQL 17 defaults to scram-sha-256 regardless — no security regression. What is wrong is that the suite now certifies dead lines as working. Build the fixture from the installed `postgresql.conf.sample`; then either delete the dead seds, which is a change to the image and needs its own changelog entry, or assert they are no-ops and say why. The `listen_addresses` tests in the same file are the counter-example and the standard the other two should meet: they drive the real default line, a `'*'` line and a missing line, and assert the failure path. **Separately, before LAPP composes this component.** The component sets `postgres`/`postgres` by default, and the only removal lives in `keel-postgresql/conf.d/main`. By decision 0013 LAPP composes `unit-postgresql` onto `apache-php`, not onto this recipe — so LAPP inherits a well-known superuser password unless its own `conf.d` repeats the removal, which is exactly the copy-paste the split exists to prevent. It is documented in `conf-vars` but nothing enforces it. Bind the two halves together rather than documenting the gap.

marcos-mendez commented

2026-10-07 12:49:03 -03:00

(Migrated from github.com)

Superseded by the current build (common's pgsql.mk); the move to .deb overlays (decision 0036) is tracked separately.