OpenConceptLab/ocl_online#476 | Covering list indexes so deep pages walk their OFFSET in the index

[paynejd]

Relates to OpenConceptLab/ocl_online#476. The speed-up depends on #937.

Summary

Adds covering versions of the HEAD list indexes: the same key and predicate, plus INCLUDE (id).

Existing	Added
concepts_repo_list_idx	concepts_repo_list_ids_idx
mappings_repo_list_idx	mappings_repo_list_ids_idx

The existing indexes stay for now. A later migration drops them once EXPLAIN on staging and production shows the new ones in use, as the ticket requires.

CREATE INDEX CONCURRENTLY … ON mappings (parent_id, updated_at DESC) INCLUDE (id)
  WHERE NOT retired AND is_active AND id = versioned_object_id;

With #937, a deep page of a HEAD list selects its ids first: SELECT id … ORDER BY updated_at DESC LIMIT n OFFSET m. With id in the index, Postgres walks that OFFSET as an index-only scan instead of reading every skipped row from the table. On a cold cache, that table walk is most of a deep page's cost (timings on the ticket). Without #937 nothing changes: page queries read whole rows, and the new index serves them as the old one did.

The old indexes were created with RunSQL and aren't in Meta.indexes, so these migrations don't touch model state either.

Migrations

concepts.0090 and mappings.0061 are non-atomic and use CONCURRENTLY.

New index before migrate	What migrate does
Missing, table ≤ 3M rows (fresh, test, dev)	Builds it, checks it's valid
Missing, table > 3M rows	Stops with the SQL to run by hand
Built by hand, valid	Nothing
Present but INVALID (interrupted build)	Stops with how to rebuild it

The migrations never drop the existing indexes. Reversing drops only the new ones.

Rollout: build by hand first (recommended)

Every API container runs migrate (pre_startup.sh) before it starts serving. On production-sized tables, a CONCURRENTLY build takes minutes, because it scans the table twice. If it outlasts the load balancer's health-check grace, the container is replaced mid-build. That leaves an INVALID index, and every restart hits it.

So in a quiet window, before deploying:

1. Run the CREATE INDEX CONCURRENTLY above for each table, on staging first.
2. Check pg_index.indisvalid.
3. Confirm EXPLAIN on a deep page uses the new index.

The deploy's migrate then only checks the new indexes. If the manual build is skipped, staging's migrate stops with the SQL to run. Staging deploys before the production approval, so that happens before production.

Tests

• core/integration_tests/tests_list_indexes.py (new):
  • on the test database, the new indexes exist, are valid and cover id, and the existing ones are still there;
  • with a mocked cursor, each migration refuses to build on a large table, uses an index built by hand, stops on an invalid one, and never drops an index going forward.
• Local Postgres 14:
  • forward adds the new index and keeps the old one; reverse drops only the new one;
  • a fresh test database ends up with both indexes, the new one valid.
• On a synthetic 600k-row table with this index's shape, the ids query with OFFSET 90000 runs as an Index Only Scan with Heap Fetches: 0.

Review

• Codex pass 1: F1.1 (P1, the old index was dropped before anyone confirmed the new one is used) and F1.2 (P2, an interrupted rollback could drop the valid index). Both fixed in 6ff431e by making the migrations add-only.
• Codex pass 2: clean.
• Simplification review: 3 cuts, applied in b3b2b6b: two test trims and an inlined constant.
• Codex check of the cuts: clean.
• Locally: the full suite (2,540 tests) passes and pylint -j0 core/ is clean.

🤖 Generated with Claude Code

[paynejd]

Codex review, pass 1 (codex-cli 0.162.1, commit f44d882)

Verdict: 2 findings (P1, P2).

• F1.1 (P1) 0090/0061: the migrations drop the old index once the new one is valid, without the check that it's used, which the ticket's criteria require. On a small table (QA), the old index goes before anyone can EXPLAIN the real query.
• F1.2 (P2) reverse: an interrupted rollback can leave an INVALID old index. A retry then skips creating it (IF NOT EXISTS) and drops the valid covering index.

findings.json

{
    "reviewer": "codex-cli 0.162.1",
    "perspective": "defects",
    "mode": "full",
    "commit": "f44d8828",
    "verdict": "findings",
    "prior": [],
    "findings": [
        {
            "id": "F1.1",
            "severity": "P1",
            "tag": null,
            "file": "core/concepts/migrations/0090_concepts_repo_list_ids_idx.py",
            "line": 44,
            "claim": "Both migrations drop the old indexes after checking validity alone, bypassing the required confirmation that replacements are used.",
            "scenario": "On a QA/staging table below the threshold, startup creates the covering index and immediately drops the old index before gunicorn serves any requests. There is no intervening opportunity to validate the actual HEAD query's plan or cold-cache performance. Deploying this diff without prerequisite oclapi2#937 also removes the old indexes while HEAD pagination still fetches full rows.",
            "action": "Defer both old-index drops to a separately gated cleanup after oclapi2#937 and the required staging/production EXPLAIN and timing checks.",
            "lines_delta": null,
            "confidence": "high"
        },
        {
            "id": "F1.2",
            "severity": "P2",
            "tag": null,
            "file": "core/mappings/migrations/0061_mappings_repo_list_ids_idx.py",
            "line": 49,
            "claim": "Retrying an interrupted rollback can drop the valid covering index while leaving an invalid old index.",
            "scenario": "A rollback's concurrent creation of the old index is interrupted, leaving it INVALID. On retry, CREATE INDEX IF NOT EXISTS skips that existing index, then restore_index drops the valid covering index and reports success. The intended original list index remains unusable. PostgreSQL documents both behaviors in its [CREATE INDEX reference](https://www.postgresql.org/docs/14/sql-createindex.html). Both migrations have this defect.",
            "action": "Check the restored old index's indisvalid before dropping the covering index; preserve the covering index and fail with recovery instructions when invalid. Add an interrupted-rollback retry test.",
            "lines_delta": null,
            "confidence": "high"
        }
    ]
}

[paynejd]

Response to Codex pass 1, both fixed in 6ff431e:

• F1.1 (P1): the migrations only add the covering indexes now. The old ones stay until EXPLAIN on staging and production shows the new ones in use; a later migration drops them.
• F1.2 (P2): fixed by the same change. Rollback only drops the covering index, so an interrupted rollback can't leave the list without a valid index.

Verified on Postgres 14:

• Fresh test database: after migrating, both indexes exist and the new one is valid (6 tests).
• Local database: forward adds the new index and keeps the old one; reverse drops only the new one.

[paynejd]

Codex review, pass 2 (codex-cli 0.162.1, commit 6ff431e)

Verdict: clean. F1.1 and F1.2 resolved; no new findings.

findings.json

{
    "reviewer": "codex-cli 0.162.1",
    "perspective": "defects",
    "mode": "verify",
    "commit": "6ff431e0",
    "verdict": "clean",
    "prior": [
        {
            "id": "F1.1",
            "status": "resolved"
        },
        {
            "id": "F1.2",
            "status": "resolved"
        }
    ],
    "findings": []
}

[paynejd]

Simplification review (fresh Claude subagent, commit 6ff431e)

Verdict: 3 cuts proposed, all applied in b3b2b6b.

• S1 (dup-test): the mocked small-table build. Creating the test database runs the real build, and ListCoveringIndexTest checks the result. Applied.
• S2 (shrink): the validity join in the DB test. A migrated database can't hold an invalid index, because the migration raises. Applied.
• S3 (shrink): inlined the single-use PREDICATE in both migrations. Applied.

After the cuts: 5 index tests pass on a freshly created test database, and pylint -j0 core/ is clean. The full suite passed at 6ff431e (2,540 tests).

simplify.json

{
    "reviewer": "claude subagent",
    "perspective": "simplify",
    "mode": "full",
    "commit": "6ff431e0",
    "verdict": "findings",
    "prior": [],
    "findings": [
        {
            "id": "S1",
            "severity": null,
            "tag": "dup-test",
            "file": "core/integration_tests/tests_list_indexes.py",
            "line": 51,
            "claim": "test_builds_on_a_small_table repeats ListCoveringIndexTest: creating the test database runs the real build on small tables, and ListCoveringIndexTest checks the result.",
            "scenario": null,
            "action": "nothing replaces it",
            "lines_delta": -6,
            "confidence": "medium"
        },
        {
            "id": "S2",
            "severity": null,
            "tag": "shrink",
            "file": "core/integration_tests/tests_list_indexes.py",
            "line": 17,
            "claim": "The pg_index join and assertTrue(valid) can go: add_index raises on an INVALID index, so a migrated test database can't hold one.",
            "scenario": null,
            "action": "select indexdef from pg_indexes only; keep the definition and old-index assertions",
            "lines_delta": -3,
            "confidence": "medium"
        },
        {
            "id": "S3",
            "severity": null,
            "tag": "shrink",
            "file": "core/concepts/migrations/0090_concepts_repo_list_ids_idx.py",
            "line": 15,
            "claim": "PREDICATE is used once, inside CREATE_NEW_INDEX (same in mappings 0061).",
            "scenario": null,
            "action": "inline the literal predicate",
            "lines_delta": -2,
            "confidence": "medium"
        }
    ]
}

[paynejd]

Codex review, check of cuts (codex-cli 0.162.1, commit b3b2b6b)

Verdict: clean. The simplification commit broke nothing.

findings.json

{
    "reviewer": "codex-cli 0.162.1",
    "perspective": "defects",
    "mode": "cuts",
    "commit": "b3b2b6b4",
    "verdict": "clean",
    "prior": [
        {
            "id": "F1.1",
            "status": "resolved"
        },
        {
            "id": "F1.2",
            "status": "resolved"
        }
    ],
    "findings": []
}