Six SQL-text pins assert a behaviour by grepping for its current spelling, so a semantically neutral rewrite reds four untouched files

Six tests broke on PR #3212 from a change that altered no behaviour: qualifying database_name to ios.database_name after 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:

The filter, which is what makes the reporter's question answerable at all: before this there was no way to ask about one database, so the only way to see a quiet database's indexes was to hope no louder one outranked it.

and whose body is:

Assert.Contains("($2::text IS NULL OR database_name = $2::text)", ReaderSql, StringComparison.Ordinal);
Assert.Contains("LIMIT $3", ReaderSql, StringComparison.Ordinal);

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.

Parse rather than grep where the claim is structural. A filter's presence is a property of the statement, not of its spelling — the same argument De-skew default_trace_events.event_time to UTC at every Darling read boundary (#3198) #3202 made for reading provenance out of a collector's own query text rather than trusting a declaration.
Or assert against a normalised form — strip alias qualifiers before comparing, so ios.database_name and database_name are the same token to the pin and a removed filter still reds.
Or execute it. These are store reads and Darling PostgreSQL tests already stands up a real PostgreSQL. A test that runs the query with $2 null 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.

Scope get_running_jobs' frame claim to its own payload, and settle on…

Claude posting for Erik Darling

This is five pins, not six. TheCountAndTheRowsShareTheirFilter does 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 those TheCountAndTheRowsShareTheirFilter owns. 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.

Keep T-SQL's schema qualifiers out of the alias stripper, before anyo…

De-skew the sixteen server-local timestamps MCP reads return, in both…

Make SqlTextPin refuse a needle whose dotted literal its alias stripp…

Five instruments that assert something other than what they check (#3211

Claude posting for Erik Darling

Fixed by #3226, merged to dev as c9f04f3fe049. 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 tests already stands up, for the two IndexUsageTruncationTests claims 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.

TheCountAndTheRowsShareTheirFilter was 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 a WITH svr CTE. 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_SCHEMA were missing from the normaliser's exclusion set. Without them, taking the helper to T-SQL would have stripped sys. 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.

Darling's four object-locking reads anchor on the server's latest cap…

Navigation Menu

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

The clearest instance

Why this is worth fixing rather than living with

What a fix looks like

Do not fold this into #3212

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Six SQL-text pins assert a behaviour by grepping for its current spelling, so a semantically neutral rewrite reds four untouched files #3217

Description

The clearest instance

Why this is worth fixing rather than living with

What a fix looks like

Do not fold this into #3212

Activity

erikdarlingdata commented on Sep 9, 2026

erikdarlingdata commented on Sep 9, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions