Keep the docid lookup (.spt) in sync with the index format version on ALTER; repair v70 lookups under a v71 header (#4852)

[tudorvasinca]

Fixes #4852.

Root cause

Index format v.71 (013bfbb, "harden and optimize UUID primary IDs") added an 8-byte UUID-entries offset to the .spt (docid lookup) header, and LookupReader_c::SetData() is gated on the chunk's index_format_version. Every in-place operation that rewrites a disk chunk header — ALTER TABLE ... ADD/DROP COLUMN, ADD/DROP field, SaveHeader(), RewriteHeader() — stamps the current INDEX_FORMAT_VERSION but leaves .spt untouched.

So on a chunk built before v.71 and ALTERed by a v.71+ daemon, the lookup is decoded 8 bytes off: every docid→rowid lookup returns garbage rowids, and whatever consumes them segfaults — UPDATE (Update_CollectRowPtrs → Update_Blobs), DELETE/REPLACE (KillMulti), id filters and docstore reads (GetRowidByDocid). In memory the chunk keeps the version it was loaded with, so nothing fails until the daemon restarts; then the binlog replay of the same UPDATE crash-loops the startup — exactly the sequence in the issue (our chunks were built by 28.4.4 = v.70 and ALTERed after upgrading to 29.0.2; the log shows a clean SIGTERM at 11:29 and every crash after).

Reproduced by replaying the poisoned binlog against the affected chunk under gdb: header 71, .spt in the v.70 layout (first checkpoint at offset 471 instead of 24616), Intersect() yields rowid 0xf5010000 for docid 191429 → DeadRowMap_c::IsSet on main / sphGetBlobAttr on 29.0.2. The retained data dir has 120 chunks in that state.

Change

• docidlookup.cpp/h: CheckDocidLookupFormat() validates the layout against a format version (the first checkpoint must start right after the header and the checkpoint table); DetectDocidLookupVersion() names the layout actually present; UpgradeDocidLookupFile() rewrites a pre-v.71 file into the current layout via a temporary file (header field inserted, checkpoint offsets shifted).
• CSphIndex_VLN::UpgradeDocidLookup() runs before the header is rewritten in AddRemoveAttribute, AddRemoveField, SaveHeader and RewriteHeader, so a chunk is brought to the current format together with its header (and m_uVersion is bumped only then).
• PreallocDocidLookup() validates the lookup on load. The unambiguous case — v.71+ header over a v.70 lookup, i.e. what released daemons produced on ALTER — is repaired in place with a warning; any other mismatch fails the chunk with a clear error instead of crashing later.
• indextool --check reports the mismatch.
• gtest UuidDocidLookupTest.FormatCheckAndUpgrade (v.70 layout detected under a v.71 version, upgrade, round-trip).

Not addressed here (separate issue candidate): RtIndex_c::AddRemoveAttribute treats a failing per-chunk AddRemoveAttribute as a warning while the RT schema has already advanced (fixme: we can't rollback).

Verification

On the reporter's actual data (29.0.2 chunks, planetrc_1_en, 197k docs):

• as-is corrupted chunk + the poisoned binlog: loader logs docid lookup is in the pre-v.71 layout while the header is v.71; upgrading the lookup in place, the table loads, the previously crash-looping UPDATE replays, UPDATE/DELETE/WHERE id IN on the affected docs work, indextool --check passes afterwards;
• a copy with the header reverted to v.70 (the pre-ALTER state): ALTER TABLE ... ADD COLUMN upgrades the lookup, headers go to the current version, lookups keep working across a restart;
• before the fix indextool --check on the corrupted chunk: FAILED, docid lookup layout does not match index format v.71: first checkpoint at 471, expected 24616 (the lookup is in the pre-v.71 layout ...).

A small scanner that finds affected chunks in a data dir (header version vs .spt layout) is attached to the issue.

[CLAassistant]

All committers have signed the CLA.

[tudorvasinca]

CI follow-up, pushed in the second commit:

• test_250 -- pick up indexes from new path using rotation failed on every platform because of this PR: the test rotates in the committed test/test_250/data/plain2.*, a v.54 table from 2019 whose .spt is 10 bytes (a layout that predates the 2022 "split docid lookup" header of docs / docs-per-checkpoint / max-docid). The daemon has always read such lookups as-is, and the new CheckDocidLookupFormat() rejected it as too short, so the rotation kept the old table. The check (and the in-place upgrade on ALTER) is now gated on DOCID_LOOKUP_SPLIT_VERSION = 65: the split landed inside v.64 without a version bump (5.0.x wrote the old layout, 6.0 the new one), so v.65 is the first version guaranteed to carry the current layout; tables below it are read exactly as before. Covered by two more expectations in UuidDocidLookupTest.FormatCheckAndUpgrade (the 10-byte legacy lookup passes below the gate, fails at it, the upgrade is a no-op below it).
• The remaining unrecoverable case (a >= v.65 header over a lookup that is neither the v.70 nor the v.71 layout) stays a load error, now saying that the table must be rebuilt.
• test_142 -- RT on *nix: watchdog and replaying binlog also failed in that run (5 retries); it fails the same way on other PRs' runs (e.g. Group sorter: rebuild the group hash after a finalizing cut; make a missing-key FindAndDelete safe (#4856) #4859), so I believe it is unrelated.

[klirichek]

Ideally it would be perfect if .spt format was constructed as backward-compatible; but right now we already missed this train.

Well, if it is happened, that current builds break an index with ALTER, that is should be fixed as a root case.

That is, if we going to rewrite header, the spt should also be rewritten, if necessary, at the same time, so that situation when header has v.71+, but .spt lookup doesn't match it should be impossible. So, if we rewrite header, we should check, if we rewrite from the pre-UUID state, and if necessary, rewrite .sph also. Usually 'alter' operations are expected as long-running, so one more action there is perfectly ok. So, daemons responsibility is this 'writing in sync'.

With bad versions of daemon, if table is never altered, it has old header and old lookup, and nobody cares.
But if it's header came out of sync because of alter, it is better just to reject load such table with clear message why. Universal advice to a user in this case - 'check the table with indextool'.

Future versions of daemon should not try to fix anything 'on the fly' in case table is already inconsistent. That is puprpose of indextool.

[klirichek]

not sure at all that file should be rewritten during simple prealloc().
It looks more safe in this case just to report the error and reject loading table.

[tudorvasinca]

Agreed - removed in 6b83674. Prealloc only validates now and refuses the table: docid lookup layout does not match index format v.72: ... ; check the table with indextool, then rebuild or restore it (manticoresearch#4852). Verified on a chunk with a v.72 header over a v.70-layout lookup: the daemon starts, the table is refused with that message, and indextool --check reports the same.

[klirichek]

That is bad idea. RewriteHeader should perform exactly one thing - rewrite the header. I.e. only .sph file; with most safe way. Break the const contract and perform any other changes is too legacy; it will be really hard to diagnose/support it in future.

[tudorvasinca]

Agreed - RewriteHeader() only rewrites the header again (6b83674). Its one caller, the ATTACH ext-copy (which ALTER ... RENAME goes through via CREATE TABLE LIKE), upgrades the docid lookup first through a CSphIndex virtual, in the same operation - so 'writing in sync' stays with the operation, and the header writer keeps its const contract.

[klirichek]

what if on further upgrade some new field will be added?
If addressed only one pivot version, say, 71, it should not be sensible if in v.81 header size again changed.
Also, if it is sticked to concrete version - that is totally ok and desirable to name the function more concretely, as generic name is a bit confusing.

[tudorvasinca]

Reworked in 6b83674: the sizes are now explicit per-version constants (DOCID_LOOKUP_HEADER_SIZE_V65 / _V71) behind DocidLookupHeaderSize(), and a static_assert on INDEX_FORMAT_VERSION next to the upgrade code makes the next format bump fail to compile until the layout table is re-confirmed - a v.81 header change cannot silently pass or fail this validation any more.

[tudorvasinca]

Thanks for the direction, @klirichek - pushed 6b83674 (the branch is also rebased onto current main):

• No repair on the fly: PreallocDocidLookup() only validates; an inconsistent table is refused with ...spt: docid lookup layout does not match index format v.72: first checkpoint at X, expected Y (the lookup is in the pre-v.71 layout; the header was rewritten with a newer format version without converting it); check the table with indextool, then rebuild or restore it (manticoresearch#4852). indextool --check reports the same condition.
• Header and .spt written in sync by the operation: the ALTER entry points (AddRemoveAttribute, AddRemoveField) upgrade the lookup before saving the header, as before; RewriteHeader() is pure again, and its caller (the ATTACH ext-copy that ALTER ... RENAME uses through CREATE TABLE LIKE) upgrades the lookup first via a CSphIndex virtual in the same operation. The hook in the uncalled SaveHeader() is dropped.
• Concrete names + future versions: DOCID_LOOKUP_HEADER_SIZE_V65 / _V71 constants behind DocidLookupHeaderSize(), and a static_assert on INDEX_FORMAT_VERSION that forces the layout table to be re-confirmed on the next format bump.

Verified: the docidlookup gtests pass; a freshly built table whose chunk lookup was rewritten into the v.70 layout under its v.72 header is refused on load with the message above and flagged by indextool --check; a plain ALTER on a pre-v.71 table still converts the lookup in the same operation. The ATTACH-path placement is covered at compile/unit level - tell me if you want a CLT test for the RENAME flow on top.

[klirichek]

I've think about how possible is to fix it globally with version upgrade, may be you can suggest something better, or use my solution.

In addition, we report that index should be fixed, but provide no instrument for it. May be add option to indextool, or even to index-converter which will just do these things offline. Full rebuilding of the index will also work, but in the case we can rewrite the file easy way, it is more appropriate.

[klirichek]

read about upgrading version to 73-74. It should help to totally avoid that static assert. We don't need to support this fix during a long time; let's fix once and avoid future checks

[tudorvasinca]

Gone in 84a12d0 - with v.73+ never checked, the assert has no reason to exist.

[tudorvasinca]

Pushed 84a12d0 with your scheme, @klirichek:

• Versions: INDEX_FORMAT_VERSION is now 74. v.71/v.72 headers are suspect and are the only ones whose lookup layout is checked on load; v.73/v.74 are the fixed twins (73 = the v.71 feature set, 74 = v.72's), written only with the lookup in sync and never checked - so the static_assert is gone, and once everything is v.73+, this issue needs no attention at all. A rewritten header of an existing table stamps the fixed version (71->73, 72->74, through BuildHeader_t::m_uFormatVersion); fresh builds stamp 74.
• Offline instrument: indextool --fix-docid-lookup <table> repairs a rejected table on the files, per disk chunk for RT tables: converts a pre-v.71 lookup to the current layout when needed and stamps the fixed header version; a consistent suspect table just gets the stamp. Damage beyond the recognizable pre-v.71 layout still says rebuild/restore.

Verified with a build of this branch: a fresh table stamps v.74; a fabricated v.72 header over a v.70-layout lookup is refused on load, indextool --fix-docid-lookup converts it and stamps v.74, after which the daemon loads it and docid lookups (UPDATE by id) work; a consistent v.71 table gets stamp-only to v.73; the docidlookup gtests cover the suspect gate (70 skipped, 71/72 checked, 73/74 skipped) and the twin mapping.

[klirichek]

I approve in general, imply that tests should pass before merge.

[tudorvasinca]

Thanks for the approval, @klirichek! CI is fully green on 84a12d0 — 49 checks passed, no failures.