feat: PostgreSQL sink backend

[dcoric]

Adds PostgreSQL as a supported sink backend, complete with schema migrations, data migration, and production connection/auth options.

This is an integration branch. The PostgreSQL work was split into small, reviewable PRs that build on each other, and merging them into main one at a time would leave a half-configured backend in place for days at a time. They target this branch instead, so each keeps its own review and CI, and main sees the finished backend in one piece.

Collected PRs

PR	Scope	Status
#1532	PostgreSQL sink backend (users, repos, pushes, session store)	Merged
#1581	Versioned DDL schema migrations with advisory locking	Merged
#1582	Data migration from mongo/fs into PostgreSQL	Merged
#1583	Idle pool client error handling	Merged
#1584	Configurable connection (discrete fields, TLS, pool tuning)	Merged
#1590	Normalise repo permissions into a repo_users join table	Merged
#1591	AWS RDS/Aurora IAM authentication	Merged
#1692	Pushes hot-path indexes and slimmer list projections	Merged
#1689	sink-parity agent skill and AGENTS.md steering for backend parity	Merged

All nine pieces have merged and each was reviewed and CI-verified individually against this branch (full pipeline including the PostgreSQL integration lane). The branch carries no drift from main. This PR is the complete feature, ready for final review. Related issues: #1688, #1690, #1691.

What lands

• postgres sink selectable via the existing sink config, alongside fs and mongo
• Full Sink interface parity with the mongo and NeDB backends, including getRepoPushRollupsByCanonicalUrl, getPushesForUserProfile, updateRepo and the migration hooks
• Versioned, advisory-locked DDL migrations applied on startup
• migrate-to-postgres command for existing mongo/fs deployments
• Connection via connection string, discrete fields or PG* env vars, with TLS and pool tuning
• Optional RDS/Aurora IAM token auth
• Design decisions recorded in the architecture doc, plus a sink-parity skill so future adaptor changes keep all backends aligned
• Unit plus integration coverage, with a dedicated PostgreSQL CI lane

Note on migrations

main has since grown a cross-backend migration framework in src/db/migrations, which records logical migrations by string id through Sink hooks. That is a different concern from #1581, which versions the postgres DDL itself and serialises concurrent runs with an advisory lock. Both are kept: the postgres adapter implements the framework hooks in src/db/postgres/migrations.ts, and the DDL runner lives alongside it in src/db/postgres/schemaMigrations.ts.

Existing deployments are unaffected: the default sink remains the filesystem backend and postgres is opt-in.

[jescalada]

Don't forget to revert these, we're getting duplicate jobs on the CI 😃

Will do a full review and AI scan later 👍🏼

[dcoric]

Good catch, reverted - the triggers are back to main only. They existed so the stacked PRs (based on feat/postgres) got the full pipeline; with everything merged, this PR's own runs are covered by the original filter and the addition was just producing the duplicate runs you saw. The only ci.yml change left in the diff is the intended one: the PostgreSQL service container and integration test lane that ship with the feature.

[jescalada]

@dcoric Thanks for this! My main question is the JSONB design choice: I'd like to know why you chose this beyond symmetry with Mongo/NeDB. What are the pros and cons of JSONB over normalization, and most importantly, which gives faster reads & writes?

@finos/git-proxy-maintainers I've reviewed all of the smaller component PRs previously, so this is a final pass + AI scan for the whole PR. I'd appreciate an extra look! 😃

[jescalada]

Wondering if the buildPoolConfig function could be refactored? I noticed a lot of repeated code here, and it could be more readable 🤔

[dcoric]

Refactored in 435c232: the duplicated discrete-field copying is now applyDiscreteFields (with an includePassword switch for IAM mode, where a token provider replaces the password), and the SSL and pool-tuning blocks moved into applySsl / applyPoolTuning. buildPoolConfig itself is left as a readable three-way branch over the connection modes. Behaviour is identical — the existing unit tests over warnings and precedence pass unchanged.

[jescalada]

Is it okay to leave this empty?

[dcoric]

Intentional — that's the catch around ROLLBACK inside withTransaction. If rollback itself fails, the connection is already broken: the error worth surfacing is the original one from the transaction body, which is rethrown on the next line, and finally releases the client so the pool discards it. The comment inside the block says as much. Happy to add a debug log there if you'd prefer the rollback failure visible too.

[jescalada]

I thought Repo was guaranteed to have the project attribute - any reason why we we're defaulting to '' here?

From /src/db/types:

export class Repo {
  project: string;
  name: string;
  url: string;
  users: { canPush: string[]; canAuthorise: string[] };
...

[dcoric]

The Repo class declares project: string, but what actually reaches the sink at runtime are plain objects from the HTTP handlers and tests, not validated class instances — and the mongo/fs backends silently accept a missing project today. The column is TEXT NOT NULL DEFAULT '', so ?? '' makes postgres accept the same inputs the other backends do rather than throwing a not-null violation only on this sink. Tightening it to a thrown error would be a cross-backend behaviour change, which I'd rather not smuggle into this PR.

[jescalada]

I think this might be exploitable with two concurrent addPublicKey requests. If two users add the same public key simultaneously, they second call might be able to bypass the findUserBySSHKey check if the first call didn't complete the DB update.

Very unlikely to happen with real usage, but something that AI scans will probably complain about 😄

[dcoric]

Good catch — fixed in 61cb038. addPublicKey now runs in a transaction that first takes pg_advisory_xact_lock(hashtextextended(key, 0)): concurrent adds of the same key serialise on the lock and the loser then sees the winner's row in the duplicate check. (A unique constraint can't span elements of a JSONB array, hence the advisory lock rather than a schema change.) The target user row is also read FOR UPDATE for the fingerprint check. Added a deterministic integration test racing two users adding the same key — exactly one wins, the other gets DuplicateSSHKeyError.

Mongo carries the identical race; left out of scope here since it needs a different mechanism there.

[jescalada]

AI scan comment on this one:

authorise / reject / cancel in pushes.ts: All three do getPush → mutate in memory → writeAudit, with no transaction and no row lock, and writeAudit overwrites data wholesale via ON CONFLICT DO UPDATE. Two reviewers acting on the same push at the same moment produce a straight lost update, and worse, if the proxy writes step results for that push id between the read and the write, those results vanish from the audit record. The fix is cheap since withTransaction already exists: wrap the three of them and read with SELECT data FROM pushes WHERE id = $1 FOR UPDATE. Mongo has the same shape, so it's not a regression, but Postgres is the backend where you can actually fix it.

I'm not sure if it correctly scanned other files as well: I don't think we're using withTransaction outside of updateRepo in /repo.ts...

Long story short, we should check if it's appropriate to wrap all our writeAudit-related calls in withTransaction 👍🏼

[dcoric]

Agreed — fixed in 61cb038. authorise/reject/cancel now read the row with SELECT ... FOR UPDATE inside withTransaction and write through the same client, so two concurrent decisions (or a step-result write racing a reviewer) serialise instead of the later write discarding the earlier one. Also added an integration test running authorise and reject concurrently against the same push.

On the wider question: writeAudit itself stays un-wrapped — a single upsert is already atomic, and the proxy is the only writer while a push is in flight. It's the read-modify-write callers that needed the lock, and those three were the only ones in the postgres adapter.

[jescalada]

AI comment on this one:

createRepo isn't atomic. The INSERT INTO repos returns an id, then the function loops addUserToRole once per username, each of which is two more un-transacted round trips. A crash or connection drop partway leaves a repo row with partial or empty canPush / canAuthorise. Since those arrays are what gate pushing and approving, "created but missing its grants" is a state I'd rather not be able to reach. Same withTransaction treatment, and you can collapse the loop into a single insert over unnest($2::text[]).

[dcoric]

Fixed in 61cb038, exactly as the scan suggested: the repo insert and its permission grants run in one withTransaction, and the per-user loop is collapsed into a single INSERT ... SELECT ... FROM unnest($3::text[]). The same set-based insert is now shared with updateRepo's wholesale permission replacement, which had the same loop.

[jescalada]

Another interesting AI comment on this one:

A design question rather than a bug: repo_users.username is plain TEXT with no foreign key, and deleteUser doesn't touch the table. Delete alice, create a new alice six months later, and she quietly inherits every grant the old one had. updateUser renaming a username has the same problem in reverse, orphaning grants. Mongo carries the identical hole, so it's not new, but normalising into a join table is precisely the moment you get to fix it, and leaving it means the normalisation buys structure without buying integrity.

TL;DR: Deleting a user by username and adding it again (with the same username) makes the new one inherit the old user's permissions. Might be worth making an issue for this since it's apparently reproducible in Mongo too 🤷🏼

[dcoric]

Agreed on filing an issue — opened #1705 covering all three backends. Fixing only postgres here would make the backends diverge behaviourally (mongo/fs carry the identical hole via the usernames embedded in repos.users), and keeping the sinks at parity is the whole point of this PR, so the cleanup belongs in a cross-backend change.

[jescalada]

Last AI comment:

On migrations, one operational caveat worth documenting. connect() runs runMigrations lazily on the first query of any process, so the runtime DB role needs DDL rights permanently, which is awkward in the regulated shops that are the target audience here. And migration 5 drops repos.users, so during a rolling deploy any still-running older process breaks the moment a new one boots. I'd add an autoMigrate: false escape hatch plus a note in the architecture doc about deploy ordering. Related: connect-pg-simple runs with createTableIfMissing: true, which is a second DDL path living outside the versioned migration list entirely.

TL;DR: Make auto-migration optional since admin rights might no be enabled by default (especially in banks and the like). An organization would have to enable admin rights for the runtime DB, trigger the migration and then go back to regular rights.

[dcoric]

Implemented in 435c232: new autoMigrate option on the postgres sink (default true, so behaviour is unchanged unless opted into). With autoMigrate: false startup performs no DDL at all — it only verifies the schema is current and refuses to start with the pending versions named. Migrations are applied out-of-band with DDL-capable credentials via the new npm run migrate:postgres:schema script (same advisory-locked runner, so it's safe against concurrent runs).

Also closed the second DDL path you flagged: the connect-pg-simple session table is now owned by migration 7 (IF NOT EXISTS adopts databases where the store already created it) and createTableIfMissing is switched off, so every piece of DDL flows through the versioned list. The architecture doc gained sections on autoMigrate and deploy ordering, including the migration-5 rolling-deploy caveat.

[jescalada]

I'm wondering about the implications of using JSONB for pushes, or the hybrid thing we have for users and repos. What are the downsides of this? Isn't it considerably harder to understand and maintain JSONB queries? Or is the idea to just be able to port over any Mongo behaviour easily (since we'd be operating with documents rather than rows)? What about efficiency of reading and writing to DB vs normalizing?

I'm worried about the maintainability of JSONB over normalized SQL tables. Although there's also the question of efficiency - if we normalized the DB, we would have to build the Action objects from different tables via queries 🤷🏼

[dcoric]

Expanded this section in 435c232 with a dedicated "JSONB over full normalisation is deliberate" bullet carrying the reasoning (and fixed the stale claim that repo permissions are still JSONB — they're normalised now). Fuller answer on the main review thread.

[dcoric]

@jescalada Thanks for the thorough review! On the main question — JSONB vs normalization — this was discussed near the end of the Aug 26 community call, where we settled on storing documents as JSON in Postgres and avoiding full normalization, to keep maintenance simple and keep the sinks easy to hold at feature parity. The reasoning, now also written into the architecture doc's design-decisions section:

Access patterns. Pushes are written and read back whole, by id, and listed by timestamp. Nothing queries inside the document relationally — the handful of filtered fields (error, blocked, authorised, canceled, rejected, timestamp) are already promoted to real columns, and the two JSONB lookups the profile/activity pages make are covered by expression indexes (migration 6). Fully normalizing an Action would decompose a deeply nested document (steps, commit data, attestation) across many tables on every write and reassemble it with multi-way joins on every read, purely to serve queries the app never runs.

Reads and writes. A read is a single-row fetch by primary key either way, but JSONB skips the multi-join reassembly; a write is one upsert instead of a transactional multi-table write. For this workload — low write volume, document-shaped data — JSONB is at least as fast in both directions. Maintainability cuts the same way: the adapter stays a thin mapping, and a new Action field costs nothing here instead of a schema migration + insert/select/mapping change per table.

Where normalization pays, we did it. Repo permissions are queried relationally ("who can push where"), so they were normalized into repo_users (#1559 — this was one of #1497's open questions, resolved exactly this way). Users/repos are deliberately hybrid: real columns for everything queried or constrained (username, email, oidc_id, admin), JSONB only for the publicKeys blob. So the rule is: JSONB only where the data is genuinely a document.

Parity and portability. All three backends operating on the same document shapes is what makes the sink-parity contract tractable, and it's why migrate:postgres can move data between sinks without lossy transformation.

---

All the inline findings are now addressed on the branch:

• 61cb038 — authorise/reject/cancel run in transactions with FOR UPDATE; createRepo is atomic with a set-based unnest() grant insert; addPublicKey closes its TOCTOU race with a transaction-scoped advisory lock. New unit + deterministic integration tests for the races.
• 435c232 — autoMigrate: false escape hatch for DDL-restricted deployments (+ npm run migrate:postgres:schema for out-of-band runs), the session table folded into the versioned migration list (no more createTableIfMissing side channel), buildPoolConfig deduplicated, and the architecture doc updated with the JSONB rationale, autoMigrate, and deploy-ordering notes.
• The username-reuse/grant-inheritance hole is filed as Deleting or renaming a user leaves their repo permission grants behind (all sink backends) #1705 — it's identical in mongo/fs, so it belongs in a cross-backend fix rather than this PR.