diff --git a/migrations/0008_bounded_field_value_index.sql b/migrations/0008_bounded_field_value_index.sql new file mode 100644 index 0000000..a12d538 --- /dev/null +++ b/migrations/0008_bounded_field_value_index.sql @@ -0,0 +1,37 @@ +-- The (field, value) index could not hold a connect screen, and a game whose screen was long enough +-- failed to ingest at all. +-- +-- Observed on the first crawl large enough to find it: three of four hundred games died with +-- "54000: index row size 3048 exceeds btree version 4 maximum 2704 for index +-- game_field_field_value_idx". PostgreSQL's btree cannot index a row wider than about 2704 bytes, +-- and a connect screen is routinely thousands of characters — the longest in this catalogue is 9,376. +-- The failure is not partial: the INSERT is refused, so the whole probe's ingestion is lost for that +-- game, and it is lost again on every future probe, for ever. A game with a generous piece of ASCII +-- art was permanently unlistable. +-- +-- The index's stated purpose (0002) is §9's faceted search: which games have CODEBASE = PennMUSH, or +-- capability.gmcp.measured = true. Every value that purpose looks up is short. NOTHING HAS EVER +-- SEARCHED BY CONNECT SCREEN and nothing ever will — it is a display asset and a fingerprint, and +-- the fingerprint has its own column. So the index covers a bounded prefix, which serves the lookups +-- it was built for and cannot overflow: 256 characters is under the limit even at four bytes each. +-- +-- The stored value is untouched. Truncating what a game said in order to fit our own index would be +-- exactly the kind of quiet lossiness this schema refuses everywhere else; it is the *index* that is +-- bounded, not the fact. +DROP INDEX IF EXISTS game_field_field_value_idx; + +CREATE INDEX game_field_field_value_idx ON game_field (field, left(value, 256)); + +-- The same flaw, one index over: §7.3's identity lookup folds case and whitespace on both columns, +-- and folding does not shorten a connect screen. This one is partial rather than prefixed, because +-- its reader asks an equality question and a prefix would silently turn that into a +-- starts-with — over-matching where the raw index merely refused. Every identity signal §7.3 names +-- is short: a name, a year, a hostname, a hash, a token. A value longer than this is not one of +-- them, so excluding it from the lookup changes no correct answer. +-- +-- CatalogueDirectories carries the same predicate, or the planner cannot use a partial index. +DROP INDEX IF EXISTS game_field_folded_value_idx; + +CREATE INDEX game_field_folded_value_idx + ON game_field (lower(btrim(field)), lower(btrim(value))) + WHERE length(value) <= 256; diff --git a/src/MUI.Crawler/Persistence/CatalogueDirectories.cs b/src/MUI.Crawler/Persistence/CatalogueDirectories.cs index d668463..aaa69c5 100644 --- a/src/MUI.Crawler/Persistence/CatalogueDirectories.cs +++ b/src/MUI.Crawler/Persistence/CatalogueDirectories.cs @@ -101,6 +101,14 @@ SELECT DISTINCT game_id FROM game_field WHERE lower(btrim(field)) = lower(btrim(@field)) AND lower(btrim(value)) = lower(btrim(@value)) + -- The bound is here as well as on the index, and both are deliberate. PostgreSQL's + -- btree cannot hold a row past ~2704 bytes, and a connect screen is thousands of + -- characters, so an unbounded index refused the INSERT and cost the game its whole + -- ingestion. The index is now partial; repeating its predicate here is what lets the + -- planner use it rather than sequentially scanning game_field once per identity + -- signal per probe. It changes no answer: every §7.3 signal — a name, a year, a + -- hostname, a hash, a token — is short, and a value longer than this is not one. + AND length(value) <= 256 """, new { field, value }, cancellationToken: ct)); diff --git a/tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs b/tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs new file mode 100644 index 0000000..d0fb497 --- /dev/null +++ b/tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs @@ -0,0 +1,127 @@ +using Dapper; + +using MUI.Catalog.Persistence; +using MUI.Catalog.Tests.Persistence.Support; + +namespace MUI.Catalog.Tests.Persistence; + +/// +/// A field value too large for an index must not cost a game its listing. +/// +/// +/// Found on the first crawl big enough to find it: three games of four hundred died with +/// 54000: index row size … exceeds btree version 4 maximum 2704, because a connect screen is +/// routinely thousands of characters and the (field, value) index tried to hold one. The +/// failure is total rather than partial — the insert is refused, the whole probe's ingestion is lost, +/// and it is lost again on every future probe. A game with a generous piece of ASCII art was +/// permanently unlistable, and nothing said so. +/// +public class OversizedFieldValueTests +{ + private static readonly DateTimeOffset Now = Seed.Now; + + /// The real shape: a connect screen far past the btree limit, stored whole. + [Test] + public async Task AConnectScreenTooLargeToIndexIsStillStored() + { + await using var db = await PostgresFixture.MigratedAsync(); + var game = await Seed.GameAsync(db); + var store = new NpgsqlGameFieldStore(db.DataSource); + + // Longer than the longest observed in the wild (9,376 characters) and several times the + // index limit, so this fails against the old index and passes against the bounded one. + var screen = string.Join('\n', Enumerable.Repeat(new string('=', 78), 160)); + + await store.UpsertAsync(new GameField( + game, InternalFields.ConnectScreen, FieldSource.Banner, screen, Now, Now)); + + var stored = (await store.ForGameAsync(game)) + .Single(f => f.Field == InternalFields.ConnectScreen); + + // Stored whole. It is the index that is bounded, never the fact — truncating what a game + // sent in order to fit our own index is the kind of quiet lossiness this schema refuses. + await Assert.That(stored.Value).IsEqualTo(screen); + await Assert.That(stored.Value.Length).IsGreaterThan(2704); + } + + /// + /// Two long values that differ only past the indexed prefix are still two distinct rows. + /// + /// + /// The prefix is an index, not a key. If bounding it had collapsed rows that share their first + /// 256 characters — which two connect screens from one codebase easily do — the fix would have + /// traded a loud failure for a silent one. + /// + [Test] + public async Task TwoValuesSharingTheirFirstBytesRemainDistinct() + { + await using var db = await PostgresFixture.MigratedAsync(); + var one = await Seed.GameAsync(db, slug: "one", name: "One"); + var two = await Seed.GameAsync(db, slug: "two", name: "Two"); + var store = new NpgsqlGameFieldStore(db.DataSource); + + var shared = new string('#', 4000); + + await store.UpsertAsync(new GameField( + one, InternalFields.ConnectScreen, FieldSource.Banner, shared + "ONE", Now, Now)); + await store.UpsertAsync(new GameField( + two, InternalFields.ConnectScreen, FieldSource.Banner, shared + "TWO", Now, Now)); + + var first = (await store.ForGameAsync(one)).Single(f => f.Field == InternalFields.ConnectScreen); + var second = (await store.ForGameAsync(two)).Single(f => f.Field == InternalFields.ConnectScreen); + + await Assert.That(first.Value).EndsWith("ONE"); + await Assert.That(second.Value).EndsWith("TWO"); + } + + /// + /// Both indexes over this table are bounded, because both could refuse a connect screen. + /// + /// + /// The first fix caught only one of them and the very next probe failed on the other — so this + /// asserts the property over every index on game_field rather than over the one that was + /// noticed. An index on a raw or merely case-folded value is the shape of the bug: folding does + /// not shorten anything. + /// + [Test] + public async Task NoIndexOnThisTableCanRefuseALongValue() + { + await using var db = await PostgresFixture.MigratedAsync(); + + await using var connection = await db.DataSource.OpenConnectionAsync(); + + var definitions = (await connection.QueryAsync( + "SELECT indexdef FROM pg_indexes WHERE tablename = 'game_field'")).ToList(); + + foreach (var definition in definitions.Where(d => d.Contains("value", StringComparison.Ordinal))) + { + // Either the indexed expression is bounded, or the index only covers rows short enough. + var bounded = definition.Contains("256", StringComparison.Ordinal); + + await Assert.That(bounded) + .IsTrue() + .Because($"an unbounded index over `value` refuses a connect screen: {definition}"); + } + } + + /// The index still exists and still leads on the field, which is what it is for. + [Test] + public async Task TheFacetLookupIsStillIndexed() + { + await using var db = await PostgresFixture.MigratedAsync(); + + await using var connection = await db.DataSource.OpenConnectionAsync(); + + var definition = await connection.ExecuteScalarAsync( + "SELECT indexdef FROM pg_indexes WHERE indexname = 'game_field_field_value_idx'"); + + await Assert.That(definition).IsNotNull(); + await Assert.That(definition!).Contains("field"); + + // Asserted on the bound rather than the spelling: PostgreSQL reports the expression back as + // "left"(value, 256), quoted, and a test that matched the source text would break on a + // formatting difference while saying nothing about whether the index can overflow. + await Assert.That(definition!).Contains("256"); + await Assert.That(definition!.Contains("(field, value)", StringComparison.Ordinal)).IsFalse(); + } +}