Bound the other two PostgreSQL collectors that had no ceiling - #2619
Conversation
#2617 was a collector that read 461 GB in one statement because I bounded each index and not the cycle. Auditing the rest for the same shape rather than waiting for each to fail found two more, neither yet broken and both unbounded. pg_column_stats had no LIMIT. It is a catalog read rather than a page scan so it is nowhere near as costly, but row count is tables x columns - 361 tables clear the size floor on one measured target, and a wide schema turns that into five figures per database per day. It already ordered by relpages DESC, so a cap keeps exactly the columns a plan-shape question gets asked about. pg_buffer_usage is the more interesting one, because no LIMIT could have helped. Its OUTPUT is bounded - grouped per relation with an eight-buffer floor - but pg_buffercache materialises one row per shared buffer and takes buffer partition locks doing it, and the aggregate needs every one of them. On a large instance that is millions of rows that cannot be capped away. It gets a command timeout instead, so a slow scan ends as a classified timeout naming the collector rather than as "Exception while reading from stream" - which is precisely how #2617 presented, and why a collector that had never once succeeded went unnoticed. Both verified still returning correct rows against a live instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// unclassified Npgsql failure — which is exactly how #2617 presented and why it went unnoticed for a | ||
| /// collector that had never once succeeded. A classified timeout says which collector was too slow.</para> | ||
| /// </summary> | ||
| public override int? CommandTimeoutSecondsOverride => 120; |
There was a problem hiding this comment.
No pinning test for this override. The sibling fix in #2618 (PgIndexBloatCollector) added a regression guard in Lite.Tests/PgIndexBloatCollectorDefinitionTests.cs:
Assert.NotNull(PgIndexBloatCollector.Instance.CommandTimeoutSecondsOverride);
Assert.True(PgIndexBloatCollector.Instance.CommandTimeoutSecondsOverride >= 120);PgBufferUsageCollectorDefinitionTests.cs has no equivalent assertion, so a future edit that drops or lowers this override (undoing the fix this PR exists to make) wouldn't be caught by the test suite — only by another production incident like #2617.
| tables x columns: 361 tables over the size floor on one measured target, and a wide schema turns that | ||
| into five figures per database per DAY. The ORDER BY already puts the biggest tables first, so the cap | ||
| keeps exactly the columns a plan-shape question is asked about. */ | ||
| LIMIT 5000"; |
There was a problem hiding this comment.
Same gap as PgBufferUsageCollector: PgColumnStatsCollectorDefinitionTests.cs has no assertion that this query is bounded (e.g. Assert.Matches(new Regex(@"LIMIT\s+\d+"), Sql())). Every other correctness property of this query is pinned by a dedicated test in that file — this LIMIT is the one this PR adds and it's the one left unpinned, so a later edit that drops it silently regresses back to the unbounded-read shape #2617/#2619 exist to fix.
ReviewScope is exactly the two files described: Correctness — Both changes check out against the shared collector machinery:
Lite/Darling parity — Both apps consume the same Test coverage gap — left two inline comments. Neither collector's changed behavior is pinned by a test, unlike the immediately-preceding No security, injection, or SQL-style issues — queries are static (no interpolated user input), and T-SQL style rules don't apply here (pure PostgreSQL collector code). |
1 participant
#2617 was a collector that read 461 GB in one statement because I bounded each index and not the cycle. Rather than wait for the others to fail the same way, I audited every PostgreSQL collector for the same shape. Two more were unbounded — neither broken yet.
pg_column_stats— no LIMITA catalog read rather than a page scan, so nowhere near #2617's severity. But row count is tables × columns, and 361 tables clear the size floor on one measured target; a wide schema turns that into five figures per database per day.
It already ordered by
relpages DESC, so a cap keeps exactly the columns anyone asks a plan-shape question about.pg_buffer_usage— the interesting one, because no LIMIT would helpIts output is bounded — grouped per relation with an eight-buffer floor. Its input is not:
pg_buffercachematerialises one row per shared buffer and takes buffer partition locks doing it, and the aggregate needs every one of them. On a large instance that is millions of rows that cannot be capped away.So it gets a command timeout instead. Without one, a slow scan ends as
Exception while reading from stream— an unclassified Npgsql failure, which is precisely how #2617 presented and exactly why a collector that had never once succeeded went unnoticed. A classified timeout at least names which collector was too slow.Worth stating the distinction, because it is the reusable part: capping output and bounding work are different problems. #2617 needed the first, this needs the second, and reaching for
LIMITon this one would have looked like a fix and changed nothing.Both verified still returning correct rows against a live PostgreSQL instance.