Repository navigation
Six SQL-text pins assert a behaviour by grepping for its current spelling, so a semantically neutral rewrite reds four untouched files #3217
Description
Activity
- added a commit that references this issue
on Sep 9, 2026 Claude posting for Erik Darling
This is five pins, not six.
TheCountAndTheRowsShareTheirFilterdoes not belong in it, and including it would have removed real coverage.I read it. It is not a substring search for one statement's current spelling — it holds a clause list and asserts both SQL texts contain each clause:
foreach (var clause in new[] { "WHERE server_id = $1", "collection_time = (SELECT MAX(...))", "($2::text IS NULL OR database_name = $2::text)" }) { Assert.Contains(clause, ReaderSql, StringComparison.Ordinal); Assert.Contains(clause, DarlingObjectStatsReaderSource.IndexUsageMatchCountSql, StringComparison.Ordinal); }
The invariant is textual identity across two statements, and its docstring says why: "The count query has to apply the SAME filter as the rows, or the ratio it feeds is between two different questions and 'truncated' becomes noise on every call." The count read has no cross join, so it was never aliased — which is exactly why qualifying one side made them diverge and this pin fired. That is the invariant working, not brittleness. Relaxing it to accept divergent text would delete its ability to catch a real divergence, which is the one thing here that would actually mislead a user.
So the distinction this issue needs, and did not have: a pin that greps for a statement's spelling to assert a behaviour is the defect. A pin that compares two statements' spelling to assert they agree is sound, because agreement of text is the property. Same
Assert.Contains, opposite verdicts, and the difference is what the assertion is for.The five that stand:
IndexUsageTruncationTests.cs:47/78/98,DarlingMcpPvsToolsTests.cs:72,DarlingMcpObjectStatsToolsTests.cs:118,DarlingMcpPlanCorrectionToolsTests.cs:77— minus whichever of thoseTheCountAndTheRowsShareTheirFilterowns. Re-derive the list from that criterion rather than from the failure list in the body; the failure list is what a qualification pass happened to break, which is not the same population.Credit for this to the owner of #3212, who checked instead of accepting my framing and declined the part that was wrong.
- added 4 commits that reference this issue
on Sep 9, 2026 Claude posting for Erik Darling
Fixed by #3226, merged to
devasc9f04f3fe049. Closing by hand.Ten defect-shaped tests, not five sites — re-derived from the criterion rather than from the failure list in the body, which is what one qualification pass happened to break and a different population. The conversion touches 26 assertions across 11 tests. Routes chosen per pin: execute against the real PostgreSQL that
Darling PostgreSQL testsalready stands up, for the twoIndexUsageTruncationTestsclaims that are row properties; normalise away alias qualifiers for the other eight; parse rejected for all, because there is no parser in the dependency graph.TheCountAndTheRowsShareTheirFilterwas correctly excluded, and it has now been validated against the exact change this issue was filed about. #3212 — the qualification pass — rewrote all three readers these pins read, each wrapped in aWITH svrCTE. Every converted pin survived with zero failures, and that pin stayed quiet, because #3212 moved the count read and the rows read in step. The invariant that correctly fires on a one-sided qualification correctly does not on a two-sided one. That is stronger evidence than any mutation: the pin was exercised by the real change rather than by a synthetic one.Two things found while doing it, both fixed in the same PR:
sys/dbo/INFORMATION_SCHEMAwere missing from the normaliser's exclusion set. Without them, taking the helper to T-SQL would have strippedsys.and let a query that dropped the schema stay green — silent, and the one direction this issue forbids.The normaliser silently mangled dotted string literals.
'foo.bar'→'bar','prod.pos.use1'→'pos.use1'. Version numbers were never at risk — the(?=[A-Za-z_])lookahead saves'v1.2'— but a dotted non-digit literal had its first component stripped, weakening whatever assertion depended on it with nothing failing. Made to refuse rather than parse, since a parser is what this issue's own analysis rejected. Red-proofed in both directions, and the second is the one that carries it: widening the guard to reject any dotted literal reds the'v1.2'control.Left as measured, not fixed: roughly 1,130 more substring SQL pins of this shape exist in the tree. That figure is worth having before anyone treats "fix the class" as a bounded task.
- added a commit that references this issue
on Sep 22, 2026
Six tests broke on PR #3212 from a change that altered no behaviour: qualifying
database_nametoios.database_nameafter adding a table alias. Nothing was ambiguous, nothing regressed, and six assertions in four files this commit never touched went red.They broke because they assert a behaviour by searching for the string that currently expresses it.
The clearest instance
IndexUsageTruncationTests.TheQueryTakesADatabaseFilter_AndNullMeansEveryDatabase, whose doc comment states a behavioural claim:and whose body is:
The doc says the filter makes a question answerable. The test greps for a substring. It cannot distinguish "the filter was removed" from "the filter was rewritten to mean exactly the same thing" — and it reports both as the same failure, with a message about a reporter's question that a text change did not affect.
Same shape at
IndexUsageTruncationTests.cs:78/98,DarlingMcpPvsToolsTests.cs:72,DarlingMcpObjectStatsToolsTests.cs:118,DarlingMcpPlanCorrectionToolsTests.cs:77.Why this is worth fixing rather than living with
It fails toward the alarming label, which is the right direction — a text pin that goes red on a harmless rewrite is far better than one that stays green on a removed filter. This is not a request to loosen them.
The cost is that the failure teaches the wrong lesson. #3212's remedy is to revert the qualification, and that is correct for that PR — but if the disposition reads as "the qualification was wrong", the repository has acquired a SQL style rule whose actual origin is test brittleness. The qualification was harmless and arguably better. The next contributor who qualifies a column for a real reason — a genuine ambiguity, a second table — hits the same six failures and concludes the codebase forbids qualification.
This is the enumerated-versus-derived problem in its purest form, and it is the same family as #3208 (a census that must be derived rather than hand-declared), #3204 (a bound enforced against the wrong relationship) and #3211 (an invariant no guard expresses).
What a fix looks like
Not "delete the assertions". The behaviour is worth pinning; the question is what expresses it.
ios.database_nameanddatabase_nameare the same token to the pin and a removed filter still reds.Darling PostgreSQL testsalready stands up a real PostgreSQL. A test that runs the query with$2null and non-null and checks the row sets differ asserts the behaviour the doc comment claims, and no rewrite that preserves meaning can break it.Whichever route, the doc comment and the assertion should end up making the same claim. Today one says "answerable" and the other says "contains".
Do not fold this into #3212
#3212 should revert the unrequired qualification, ship, and name these pins in its body as the reason six unrelated tests broke. Rewriting six assertions in four files is a separate change with its own evidence, and doing it inside a timezone fix would make both harder to review.