[rjzondervan]

Internal review draft. This PR exists only to run the review skill on the fork before the upstream PR is opened. The base review-base/main equals nextcloud/tables main (8309f95). The upstream PR description will be written separately.

Refs nextcloud#3092

Changes

• AddMissingIndicesListener adds (column_id, value) indices to the number, selection, datetime and usergroup cell tables, so existing installations get them through occ db:add-missing-indices.
• The migrations that create these tables add the same indices for new installations.
• Selection, datetime and usergroup values are text: MySQL/MariaDB index a 64-character prefix, and Oracle (CLOB) skips these three.
• Unit test for the listener.

Checks run locally

• cs, psalm, rector and php lint are clean.
• phpunit: 426/426 on MariaDB.
• Fresh install on SQLite, MariaDB and PostgreSQL 16 creates the indices, and db:add-missing-indices finds nothing missing afterwards.
• Benchmark on a 100k-row × 30-column table (MariaDB and PostgreSQL 16, repeated runs) is to be posted on the issue.

AI assistance: Claude Code (claude-opus-5-5) was used for implementation and benchmarking; commits carry Assisted-by trailers.

🤖 Generated with Claude Code

[rjzondervan]

Inline findings for this round; the verdict follows as a separate comment.

[rjzondervan]

Verdict: APPROVE (Thorough) — self-review posted as COMMENT (GitHub blocks self-APPROVE)

No blockers: the indices are correct on every install and upgrade path. All four findings were fixed on this branch in 9fe3687 and 73014e0; nothing is left open from this review.

🟡 Concerns (2)

• ✅ fixed in 9fe3687 — "Covers selection option ids entirely" is false for multi-selection — lib/Listener/AddMissingIndicesListener.php:30. Multi-selection stores json_encode() of the id array (RowCellSelectionMapper.php:50), which passes 64 characters at about 15–20 options; the same claim is in the commit message of fd4dca6.
• ✅ fixed in 9fe3687 — PostgreSQL rejects long multi-selection values — lib/Migration/Version000700Date20230916000000.php:116. Reproduced on PostgreSQL 16: with the index, inserting a 1,000-id multi-selection fails with index row size 3288 exceeds btree version 4 maximum 2704; skipping the selection index on PostgreSQL costs nothing measured.

🟢 Minor (2)

• ✅ fixed in 9fe3687 — Test does not pin the number index's table — tests/unit/Listener/AddMissingIndicesListenerTest.php:35. Changing the number index's table name keeps the test green.
• ✅ fixed in 73014e0 — Nothing ties migration index definitions to listener — lib/Migration/Version000700Date20230916000000.php:106. Three copies agree today, but a drifted name would give fresh installs a duplicate index without any test going red.

✅ Verified clean

• No duplicate index on any path: db:add-missing-indices checks by name, the listener and both migrations use the same four names, and a fresh SQLite install at this head has all four _cv indices with nothing left to add (a dry run on PostgreSQL also finds nothing).
• Editing the shipped migrations is Nextcloud's standard pattern here: fresh installs get the index from the creating migration, existing installs from the AddMissingIndicesEvent listener, exactly once each.
• API compatibility for NC 33–36: Table|ITable in addValueIndex(), addMissingIndex(..., $options) and getDatabaseProvider() exist on every supported version (stable33 OCP stubs and server 36 checked; 34/35 not checked separately).
• MySQL/MariaDB key size: 64 characters × 4 bytes + the int column stays well under the 767/3072-byte limits; Oracle skips only the three CLOB columns.
• The existing (column_id, row_id) indices are untouched, as the maintainer asked on nextcloud#3092.
• phpunit 426/426 at this head (MariaDB).

Gates: not run — no Hydra gate runner on this machine and no gate job in this repository; the repository's own checks (cs, psalm, rector, lint, phpunit) ran green locally on this tree.
CI: the four Playwright jobs failed on GitHub API 504s during composer install (infrastructure, not this change); no check is required on this base.
Fixes: 9fe3687 leaves the selection value index out on PostgreSQL (listener and migration), corrects the prefix docblock and pins the number index's table in the test; 73014e0 adds CellValueIndicesTest, which compares the migrations' value indices with the listener's on MySQL, PostgreSQL, SQLite and Oracle. Checked after the last edit: cs, psalm, rector and lint clean; phpunit 431/431 (SQLite); four control mutations each turned a test red; a fresh PostgreSQL 16 install has no selection value index and saves a 1,000-id multi-selection. A pre-push adversarial check found no new issue in the fix; its wording note on the new test's docblock is applied (the message of 73014e0 keeps the earlier wording).
Out of scope, not fixed here: UsergroupBusiness::validateValue() accepts any integer type without an existence check, so on PostgreSQL an oversized fake id now fails with a 500 instead of being stored. That code predates this PR; it is a candidate for a follow-up issue.
CI on 73014e0: started at the push; not waited for.

Connected issues

• nextcloud#3092 (open) — implemented by this PR. The f2 fix narrows it: the selection index will not be added on PostgreSQL, like the text indices on Oracle, so the issue's acceptance criterion should say so. No state change.

🤖 Generated with Claude Code