Skip to content

Carry Subject schema, label and Main Subject flag on the subject-page index - #1335

Draft
alistair3149 wants to merge 4 commits into
masterfrom
1333-subject-page-index-headers
Draft

Carry Subject schema, label and Main Subject flag on the subject-page index#1335
alistair3149 wants to merge 4 commits into
masterfrom
1333-subject-page-index-headers

Conversation

@alistair3149

Copy link
Copy Markdown
Member

Fixes #1333

neowiki_subject_page gains three columns — nwsp_schema VARBINARY(255) NULL, nwsp_label BLOB NULL and
nwsp_is_main TINYINT NOT NULL — read from the raw slot JSON the index writer already parses, at the call
site that already writes the table. No new index: they ride a row the caller already looks up by key. Naming
a Subject takes exactly these three plus the page name, which reads already join.

A prerequisite, not a win on its own: nothing reads the columns yet, and
#1334 is filed but unassigned and unscheduled.
Nine of the
ten SubjectDisplayName call sites already hold both the Subject and its page's PageSubjects; one would
save a lookup. The 78% figure quoted on the issue measures a walk that loads whole Subjects for their
statements, which these columns cannot spare it. They pay off for a reader that needs names without
statements — #1324's endpoint over #1334's relation index. Landing the migration together with #1334 instead
of ahead of it is a reasonable call to make on this PR.

Decisions worth a look:

  • Read raw, never deserialized, per ADR 32, so a Subject too broken to deserialize is still named. A blank
    label or Schema name stores as null, matching SubjectLabel::fromText and SchemaName.
  • The unchanged-page fast path compares all five columns of a page's rows rather than the ids alone, so
    renaming a Subject, or changing which one is Main, now updates the index where nothing happened before.
    Ordinary edits therefore write more often than they did, and ClearDefaultSubjectLabels rewrites every row
    it touches. Not measured against ADR 29's interactive-save budget.
  • The backfill guards on the columns, not the table. update.php runs post-update scripts outside
    doUpdates(), so the backfill runs even when --noschema skipped the ALTER.
  • The update key is bumped, so an existing wiki backfills — 77 pages in 0.5 s on the demo wiki.

Tests cover null-on-blank, the five-column comparison through a rename, and the columns on both the hook and
the rebuild paths. The guard's negative branch has no test: reaching it needs DDL inside a test transaction.
It was exercised by hand instead, as below.

Known gaps, not fixed here:

  • ADR 32 says of this table "It is not a fallback and not a cache", and is not amended; these columns are
    exactly that. Whether the amendment belongs here, with Answering "what points at this Subject" #1334, or the columns belong elsewhere is a call for
    review.
  • A wiki upgraded through MediaWiki's web updater gets the columns without the backfill, leaving them null on
    every pre-existing row. Latent while nothing reads them, wrong display names once something does. Repair is
    php maintenance/run.php extensions/NeoWiki/maintenance/RebuildSubjectPageIndex.php --force.
  • Nothing bounds a label's length before it reaches the BLOB. Production ($wgSQLMode = '') truncates
    silently, and the truncated row then fails the fast-path comparison on every later save; dev and CI
    (STRICT_ALL_TABLES) throw inside the revision transaction instead.

Manual upgrade check

  1. On a wiki holding the previous schema, run php maintenance/update.php --quick --noschema. It reports
    ...skipping schema change, then The NeoWiki subject -> page index is not up to date with the schema yet; nothing to rebuild., and does not record the backfill as done.
  2. Run php maintenance/update.php --quick. It adds the field, then reports Done. Indexed N pages holding Subjects.
  3. SELECT COUNT(*), SUM(nwsp_schema IS NOT NULL), SUM(nwsp_is_main) FROM neowiki_subject_page; — every row
    names a schema, and the Main count is one per page that has a Main Subject, which need not be every page.

AI-authored — Claude Code, Opus 5 (xhigh); scope set by @alistair3149 across a series of explicit choices, with the sequencing changed once when a measurement was corrected mid-session; not yet human-reviewed, though a same-session /pr-review found and fixed two blockers (the --noschema crash and the missing rebuild-path test) and its design verdict is the deflation in the second paragraph; verified with 1962 non-database tests, eight database test classes, make cs clean, and the migration plus the --noschema failure and its fix reproduced on a dev wiki; CI not yet run.

alistair3149 and others added 4 commits August 31, 2026 21:30
… index

Naming a Subject needs three fields the subject slot alone holds: its Schema, its
label, and whether it is its page's Main Subject. A surface listing Subjects
therefore reads and parses a hosting page per Subject, and gets every Subject on
that page whether it wanted them or not. On a depth-3 relation walk over the demo
wiki those reads are 78% of the marginal cost.

The index writer already parses the slot JSON, so it records the three fields
alongside the mapping it was writing anyway. They are read raw, like the ids, so a
Subject too broken to deserialize is still indexed under the name it claims.

Existing wikis take the columns through a schema patch, and the backfill runs again
on a new update key.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017cqUCA4KUn2Y1VumDkQyqm
update.php runs its post-update scripts outside doUpdates(), so the backfill runs
even when --noschema or --schema skipped the schema change. Asking whether the table
exists was the right question while the table was the only schema object here: a
skipped CREATE TABLE leaves nothing to write to. A skipped ALTER leaves the table in
place without its columns, and the backfill then dies on the first page with
"Unknown column 'nwsp_schema' in 'SELECT'".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017cqUCA4KUn2Y1VumDkQyqm
The backfill is the reason the update key was bumped, and nothing asserted it wrote
anything but the mapping.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017cqUCA4KUn2Y1VumDkQyqm
SubjectLabel::fromText treats whitespace as absence and SchemaName refuses it, so
recording "   " as a label contradicted both, and contradicted the column's own
comment promising null for a Subject that names none.

Also drops the raw subject-id reader, which the header reader left without a caller.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017cqUCA4KUn2Y1VumDkQyqm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Carry Subject schema, label and Main Subject flag on the subject-page index

1 participant