fix(monitoring): send the table's container with the maintenance target - #1091
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ba68151 to
d856797
Compare
cevheri
left a comment
There was a problem hiding this comment.
Thanks for this. The PostgreSQL half is right: I ran all three buttons against a live PostgreSQL 17 with tables in app and in a dotted odd.schema, and both now succeed where they answered relation "public.Mytable" does not exist. It also merges cleanly with main and the full suite passes.
Three providers need a fix before I merge:
- MongoDB:
assertContainerIsBoundcompares withconfig.database, but the row sendsgetDatabaseName(). On a connection-string connection every per-collection button now fails withbound to the database "". Compare withgetDatabaseName(). - Couchbase: the Tables row carries the bucket in
schemaName, not a scope, so Analyze buildstravel.travel.travel. - Oracle: with an owner, the index list comes from
ALL_INDEXES, butALTER INDEX "X" REBUILDis unqualified and rebuilds in the connected user's schema. Qualify it with the owner.
The #772 acceptance also asked for per-provider tests (non-default container, bare target, a name needing quoting) and the docs/providers/ maintenance sections, and only PostgreSQL has them so far. A test that the route and the hook pass container through is missing too. Those tests would have caught all three issues above.
…et (libredb#772) Clicking Vacuum, Analyze or Reindex on a table outside the public schema answered `relation "public.Mytable" does not exist`, because the row already rendered `schemaName` beside the table and the call dropped it, so the provider fell back to `public`. `runMaintenance` now takes the container as a third argument, and the two call sites pass `table.schemaName`. PostgreSQL, SQL Server, DuckDB, MySQL, ClickHouse and Oracle qualify with it (Oracle switching to owner-aware catalog reads), and the engines whose operation cannot name a container say so instead of pretending the target qualified: SQLite, libSQL and MongoDB ignore it with a comment, Couchbase scopes the keyspace, Trino refuses. Docs in providers/postgres.md and DATABASE_PROVIDERS.md carry the new signature. Three provider tests pin the qualification, including a container that itself contains a dot, which the old split-on-name reading could not survive.
…er-provider tests and docs it owed (libredb#772) MongoDB's refuse-other-database check compared config.database, which a connection-string connection never sets - so it refused the very database the provider is bound to, and every per-collection button answered 'bound to the database ""'. It compares getDatabaseName() now. Couchbase's Tables row is the bucket-level one, and reading its bucket back as a scope built travel.travel.travel, no keyspace at all. A bucket container is placed at the default collection, the same placement every bucket-level catalog row gets; any other container is the scope outright. Oracle read the index list from ALL_INDEXES when an owner arrived, but the rebuild was unqualified - ALTER INDEX "X" REBUILD acts on the CONNECTED schema, which is not where the list came from. The rebuild names the owner. Tests: container arms for MSSQL, MySQL, DuckDB, ClickHouse, SQLite, libSQL and Trino beside the three fixed providers, plus the route and hook pass-through tests the review asked for. Docs: the container's meaning is stated in every touched provider section.
d856797 to
afdb791
Compare
|
All three fixed in MongoDB now compares Couchbase places a bucket container at the default collection. That row is the only Tables row this provider has ( Oracle qualifies the rebuild. The list came from Tests. The per-provider arms are in: MSSQL, MySQL, DuckDB and ClickHouse get container, bare and quoting arms each, and SQLite, libSQL and Trino get one pinning that the container is ignored with the reason. The route arm asserts the container reaches Docs. Every touched provider section now states what the container means for that engine, in Verified on the touched files under bun 1.4.2 (the CI pin): 12 spec files, 1921 tests, all passing; typecheck, format, lint, One note on scope: the earlier push of this branch was rebased onto current |
|
CI note on the current head. The only red leg on The file is untouched by this branch. The same test passes on other runs: the windows leg of run 36232944381 (2026-09-26 09:29) runs it green, and so does the leg on run 36239308227. Reading it as a fetch flake on the runner. I don't have re-run rights on this repo, so I'm noting it rather than bumping the head for a retry. |
cevheri
left a comment
There was a problem hiding this comment.
Thanks, all three fixes check out. My probe against afdb7919 passes the MongoDB connection-string row, builds `travel`.`_default`.`_default` and rebuilds "HR"."ORDERS_PK". Merged with main, the suite passes at 100% coverage; the Windows red is the DigitalOcean live fetch.
One more round:
- ClickHouse:
qualify()quotes the container withSQLBaseProvider.escapeIdentifier, which clickhouse/objects.ts:1073 records as unsafe there: a backslash escapes the closing quote. Containerx\with target.t FINAL SETTINGS optimize_throw_if_noop = 1 --parses (clickhouse format26.7.1) asOPTIMIZE TABLE `x".`.t FINAL SETTINGS optimize_throw_if_noop = 1. A dotted target could already do this. Please overrideescapeIdentifierfor ClickHouse to escape the backslash too, as mssql.ts does, with a test for a trailing\. - The audit event records only
target, soapp.ordersandpublic.ordersnow log identically. Please record the container. - A non-string
containerfails in the provider asidentifier.replace is not a function. Please answer 400 at the route. docs/API_DOCS.mdlists nocontainerfor this route.- Couchbase answers
Updated statistics for travelafter touching only the default collection. Please name the keyspace. - libsql and trino now stack two docblocks on
runMaintenance; please merge them.
…enance route (libredb#1091) ClickHouse: escapeIdentifier() is overridden to escape the backslash before the quote, the order literal() in objects.ts uses. The inherited escaper doubles only the quote character, and a backslash inside a quoted identifier is an ESCAPE on this engine, so a container of `x\` swallowed its closing quote and the target that followed was reparsed as more statement text. An arm drives the review's own request, container `x\` with a target spelling trailing FINAL/SETTINGS clauses, plus one for a bare target ending in a backslash and one for the escaper itself. Audit: the maintenance event now carries `container` beside `target`, on the event and on the stdout line, omitted when the request named none the way `reason` and `bucket` are. app.orders and public.orders recorded identically while the row held only the target. Route: a non-string `container` answers 400 with '"container" must be a string naming the target's container' before any provider is opened. It used to reach the provider's identifier escaper and fail as identifier.replace is not a function, so the caller read a 500. Couchbase: the analyze reply names the keyspace the statement addressed (`travel`.`inventory`.`hotel`) rather than the bare target, which reported the bucket after touching only its default collection. libsql and trino: the two stacked docblocks on runMaintenance are merged into one each. Docs: docs/API_DOCS.md documents `container` on this route (field row, request example, and the 400 it answers); the ClickHouse provider doc records the override and its reason; the Couchbase analyze row names the reply. The citation renumbering is included: the route block moved 108-131 to 123-152, connectionName 117 to 138, the audit.ts ranges moved, and the two API_DOCS line cites follow their targets.
|
All six cleared in 1. ClickHouse and the arm asserts the naive spelling 2. Audit container. 3. Route 400. Type-checked before any provider is opened: An empty string is not malformed: it reads as a request that named no container, the same way an empty 4. 5. Couchbase. The analyze reply names the keyspace the statement addressed: 6. Stacked docblocks. libsql and trino each had two docblocks on Verification.
One note beyond the six: my edits moved lines that five docblocks elsewhere cite by number (the route's 108-131 and 117, two |
Punctuation only: the clause that used a dash pair is parenthesised, and the one after a quoted answer takes a comma. No wording or claim changes.
|
The head moved to |
…ine, and retire D49 Four line citations this branch renumbered, and a MongoDB pair its inserts shifted, pointed at other lines once main was merged in. They now name the symbol or the section instead. clickhouse.md 6.3 gave `acceptsSourceEdits` as the refusal's reason. It again records the unmeasured escaper, and section 8 carries the live check of the override. D49 is the defect this PR fixes, so it is deleted, and the DuckDB test comment that cited it now points at libredb#772. D118 still described the two-argument contract.
cevheri
left a comment
There was a problem hiding this comment.
Thanks, all six check out on 0ab20270. Against a live ClickHouse 26.7.1 the fixture's demo.bs_one\ now optimizes by all three spellings, and the review's container x\ is read as a database name and refused with UNKNOWN_DATABASE, where main fails on a syntax error. Through the running server a non-string container gets the 400 and the audit line carries container. I pushed two commits on top: a merge of main, since #1092 and #1148 changed the same provider docs, and a docs-only commit that cites by symbol where that merge moved a line, restates the reason in section 6.3 of clickhouse.md, and deletes D49, the backlog entry for this bug.
What
Clicking Analyze, Vacuum or Reindex on the monitoring page answered
for every table outside the
publicschema. The row already renderedschemaNamebeside the table name, and the call dropped it, so PostgreSQL's qualifier fell back topublic.Closes #772.
How
runMaintenancetakes the container as a third argument, and the two call sites pass theschemaNametheir row already holds.escapeIdentifierrebuildIndexestakes the ownerkillis untouched everywhere, because its "target" is a PID rather than a table.Two of these were found while building rather than reported: Oracle's index rebuild answered ORA-01418 for a table name, and ClickHouse's parts listing read the wrong database, both because the schema never arrived.
Not a behaviour change for existing callers
The container is optional and the old readings stay when it is absent: a bare name still defaults to
public, andschema.tableis still quoted per-part.Tests
Three provider tests pin the qualifier at the layer that builds the SQL:
Checking the container by splitting the name cannot work, since a schema may contain a dot, and that case is the one the third test above separates.
Verified by reverting the fix in
postgres.tsand watching exactly the two container tests fail while the no-container one stays green, then mutating the qualifier to re-split the container on its dots and watching only the dot case fail.Existing assertions in
TablesTab.test.tsxandOperationsTab.test.tsxare updated to the three-argument call. 501 tests pass across the four touched files,tsc --noEmitis clean, andbun run lintreports 0 errors. The 13 pre-existing failures elsewhere inbun run testare identical on a clean base.