Claude finished @Yeraze's task —— View job

PR Review: fix(migration): rebuild channel_database on PostgreSQL tombstone exhaustion

• Read migration implementation file
• Read migration test file
• Analyze code quality, bugs, performance, security
• Post comprehensive review feedback

Summary

This is a well-reasoned fix for a subtle but serious PostgreSQL crash path. The root cause analysis is accurate (migration 021/063 ADD/DROP cycle consuming one attnum per restart), and the chosen fix (table rebuild) is the correct approach — there is no in-place way to reclaim tombstoned attnums in PostgreSQL. The code is generally high quality. I have a few observations below, mostly minor.

Issues / Observations

1. Sequence reset sets value to MAX(id) — off by one on next insert

File: 104_add_channel_database_hash.ts:116-121

SELECT setval(
  'channel_database_new_id_seq',
  COALESCE((SELECT MAX(id) FROM channel_database_new), 0)
)

setval(seq, n) sets the last-returned value to n, so the next nextval() will return n + 1. This is correct behavior and matches the intent ("advance past the highest copied id"). However, there's an edge case: if channel_database_new is empty (no rows), COALESCE returns 0, and setval(seq, 0) will make the next insert get id=1 — which is fine. No bug here, but worth a comment for the next reader since the idiom is sometimes confused with the two-argument form setval(seq, n, false).

Low priority / doc nit.

2. Tombstone threshold of 1500 is somewhat arbitrary — consider documenting the margin

File: 104_add_channel_database_hash.ts:177

if (tombstoneCount >= 1500) {

The hard PostgreSQL limit is 1600, so 1500 gives a 100-slot buffer. That's probably fine for this table (future migrations are unlikely to add 100 more columns to channel_database), but it would be helpful to add a brief inline comment explaining the chosen value, e.g.:

// Rebuild if within 100 of PostgreSQL's 1600 attnum hard limit.
if (tombstoneCount >= 1500) {

Nit.

3. DROP TABLE channel_database CASCADE comment is slightly misleading

File: 104_add_channel_database_hash.ts:123-125

// DROP CASCADE removes all FK constraints in referencing tables
// (rows in channel_database_permissions are preserved).
await client.query(`DROP TABLE channel_database CASCADE`);

DROP TABLE ... CASCADE drops dependent objects (FK constraints, views), but it does not drop the rows in referencing tables — rows in channel_database_permissions pointing at the now-dropped table will just have a dangling channelDatabaseId after the drop, until the FK is re-attached. The comment says "rows are preserved" — this is technically true but could confuse someone into thinking referential integrity is maintained during the window between the DROP and the FK re-attach. A more precise comment would note: "rows survive but without a FK constraint until step below."

This is a correctness gap only during the migration window, and since the whole operation runs in a single migration call it's low risk. Still worth clarifying the comment.

Minor clarity issue.

4. Re-attached FK lacks a constraint name

File: 104_add_channel_database_hash.ts:139-145

ALTER TABLE channel_database_permissions
  ADD FOREIGN KEY ("channelDatabaseId")
      REFERENCES channel_database(id)
      ON DELETE CASCADE

The original FK constraint (created in an earlier migration) likely had a name (e.g., channel_database_permissions_channelDatabaseId_fkey). After CASCADE drop and re-add, the new constraint will get an autogenerated name that may differ from the original. This is generally harmless but could cause issues if any migration, test, or admin script references the constraint by name.

Recommendation: Check the original constraint name from the schema migration that created channel_database_permissions and use ADD CONSTRAINT <name> FOREIGN KEY ... to restore it consistently.

Low-medium priority.

5. No transaction wrapping around the rebuild

File: 104_add_channel_database_hash.ts:66-147

The rebuildChannelDatabasePostgres function performs ~9 DDL/DML operations. If the migration fails partway through (e.g., network blip, OOM, crash), the database could be left in an inconsistent state: channel_database may have been dropped but channel_database_new not yet renamed. The DROP TABLE IF EXISTS channel_database_new at the top covers cleanup of the scratch table, but it doesn't cover the case where channel_database itself was already dropped.

Ideally the entire rebuild would be wrapped in a transaction (PostgreSQL supports transactional DDL). The calling migration runner would need to supply a transactional client for this, which may not be possible given the current runner architecture. At minimum, the function could document this non-atomicity as a known risk.

Medium priority — worth at least a code comment noting the caveat.

6. canonicalCols list does not include channelHash — correct, but subtly so

File: 104_add_channel_database_hash.ts:103-108

const canonicalCols = [
  'name', 'psk', 'pskLength', 'description', 'isEnabled',
  'enforceNameValidation', 'sortOrder', 'decryptedPacketCount',
  'lastDecryptedAt', 'createdBy', 'createdAt', 'updatedAt',
];
const colsToCopy = canonicalCols.filter((c) => liveCols.has(c));

channelHash is intentionally excluded because it won't exist in the old table (that's the whole point of the migration). The new table has it with a NULL default, so migrated rows simply get NULL. This is correct. A comment here would make the intent obvious — e.g., // channelHash is new; old table won't have it, new table defaults to NULL.

Nit.

Test Coverage

The 3 existing SQLite tests are solid:

• Column creation ✓
• Idempotency ✓
• NULL default + value round-trip ✓

However, the PostgreSQL rebuild path has zero automated test coverage in this PR, which is explicitly acknowledged in the test file header. Given that this is the most complex part of the fix (table rebuild, sequence reset, FK re-attachment), it carries the highest risk. The PR notes it "requires a live PostgreSQL connection."

Recommendation: Consider adding integration tests for the PostgreSQL path using a pg test helper (similar to patterns likely already existing in the repo for other PG migrations), or at minimum document the manual verification steps performed. The tombstoneCount >= 1500 branch in particular is hard to verify without a test.

Verdict

The fix is correct and addresses a real, impactful production crash. The approach (detect tombstone saturation, rebuild table, copy data, restore FK) is sound. The issues noted above are mostly minor clarity/robustness concerns; the only medium-priority item is the lack of transactional wrapping and the constraint name consistency. The missing PostgreSQL integration tests are the most notable gap for a change this complex.

Approved with suggestions — the medium-priority items are worth addressing before merge if feasible.