Skip to content

fix(monitoring): send the table's container with the maintenance target - #1091

Merged
cevheri merged 6 commits into
libredb:mainfrom
sloemo01:fix/maintenance-container
Sep 27, 2026
Merged

cevheri merged 6 commits into
libredb:mainfrom
sloemo01:fix/maintenance-container

Conversation

@sloemo01

Copy link
Copy Markdown
Contributor

What

Clicking Analyze, Vacuum or Reindex on the monitoring page answered

relation "public.Mytable" does not exist

for every table outside the public schema. The row already rendered schemaName beside the table name, and the call dropped it, so PostgreSQL's qualifier fell back to public.

Closes #772.

How

runMaintenance takes the container as a third argument, and the two call sites pass the schemaName their row already holds.

Engine What the container does
PostgreSQL qualifies the target, quoted whole; the name is never re-split
SQL Server bracket-qualifies through escapeIdentifier
DuckDB qualifies the target
MySQL qualifies only when the container is not the connected database
ClickHouse replaces the database half of the name
Oracle owner-aware catalog reads, rebuildIndexes takes the owner
Couchbase scopes the keyspace
SQLite, libSQL, MongoDB ignore it, with a note saying why the operation cannot name one
Trino refuses, since its maintenance has no container form

kill is 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, and schema.table is still quoted per-part.

Tests

Three provider tests pin the qualifier at the layer that builds the SQL:

  • a container qualifies the target, and the target name is not re-split
  • a container that itself contains a dot survives the qualifier (the old split-on-name reading could not survive this, which is the reason the parameter exists)
  • without a container the old readings stay

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.ts and 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.tsx and OperationsTab.test.tsx are updated to the three-argument call. 501 tests pass across the four touched files, tsc --noEmit is clean, and bun run lint reports 0 errors. The 13 pre-existing failures elsewhere in bun run test are identical on a clean base.

@codecov

codecov Bot commented Sep 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sloemo01
sloemo01 force-pushed the fix/maintenance-container branch from ba68151 to d856797 Compare September 23, 2026 02:36
@cevheri cevheri added the bug Something isn't working label Sep 23, 2026

@cevheri cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: assertContainerIsBound compares with config.database, but the row sends getDatabaseName(). On a connection-string connection every per-collection button now fails with bound to the database "". Compare with getDatabaseName().
  • Couchbase: the Tables row carries the bucket in schemaName, not a scope, so Analyze builds travel.travel.travel.
  • Oracle: with an owner, the index list comes from ALL_INDEXES, but ALTER INDEX "X" REBUILD is 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.
@sloemo01
sloemo01 force-pushed the fix/maintenance-container branch from d856797 to afdb791 Compare September 25, 2026 23:02
@sloemo01

Copy link
Copy Markdown
Contributor Author

All three fixed in afdb7919, with the tests the acceptance asked for.

MongoDB now compares getDatabaseName() instead of config.database. You were right about the reading, and the consequence was worse than a wrong sentence: a connection-string connection sets no config.database, so the check refused the database the provider is bound to and every per-collection button on that connection failed. Two arms pin it: the bound name is accepted, and a different one is still refused with the name in the sentence.

Couchbase places a bucket container at the default collection. That row is the only Tables row this provider has (getTableStats() reports the bucket under both schemaName and tableName), and it is the same placement every bucket-level catalog row already gets from resolveKeyspaceOf(): _default._default, not travel.travel.travel. Any other container is the scope, used as one rather than parsed back out of the display name, and the bare no-container reading is unchanged. Three arms cover the three readings plus one for quoting.

Oracle qualifies the rebuild. The list came from ALL_INDEXES with the owner bound, and ALTER INDEX "X" REBUILD was acting on the connected schema, which is not where the list came from. Each rebuild is now ALTER INDEX "<owner>"."<index>" REBUILD, and an arm asserts the unqualified spelling never appears.

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 runMaintenance and that its absence arrives as undefined; the hook arm asserts the request body carries it.

Docs. Every touched provider section now states what the container means for that engine, in docs/providers/.

Verified on the touched files under bun 1.4.2 (the CI pin): 12 spec files, 1921 tests, all passing; typecheck, format, lint, security:check and readme:check clean; 0 uncovered lines across the three changed providers.

One note on scope: the earlier push of this branch was rebased onto current main (d44250e0) to keep it mergeable, so the head moved from d8567977 to afdb7919.

@sloemo01

Copy link
Copy Markdown
Contributor Author

CI note on the current head. The only red leg on afdb7919 is Cross-platform Tests (windows-latest), and the failing test is the live-fetch arm of tests/unit/digitalocean-distribution.test.ts, where the real CLI fetches the DigitalOcean marketplace listing and counts the reads. This run came back with 0 after about 10.7 seconds; the other 13 tests in that file pass.

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 cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with SQLBaseProvider.escapeIdentifier, which clickhouse/objects.ts:1073 records as unsafe there: a backslash escapes the closing quote. Container x\ with target .t FINAL SETTINGS optimize_throw_if_noop = 1 -- parses (clickhouse format 26.7.1) as OPTIMIZE TABLE `x".`.t FINAL SETTINGS optimize_throw_if_noop = 1. A dotted target could already do this. Please override escapeIdentifier for ClickHouse to escape the backslash too, as mssql.ts does, with a test for a trailing \.
  • The audit event records only target, so app.orders and public.orders now log identically. Please record the container.
  • A non-string container fails in the provider as identifier.replace is not a function. Please answer 400 at the route.
  • docs/API_DOCS.md lists no container for this route.
  • Couchbase answers Updated statistics for travel after 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.
@sloemo01

Copy link
Copy Markdown
Contributor Author

All six cleared in f71f052a, tests first (each arm was run against afdb7919 and failed before the fix landed), with the citation renumbering the edits forced.

1. ClickHouse escapeIdentifier. Overridden in clickhouse/index.ts the way mssql.ts overrides it: the backslash is doubled before the quote, which is the order literal() in objects.ts uses (doubling the quote first would leave the backslash looking like an escape of the doubled pair). Your request is now an arm: container x\ with target .t FINAL SETTINGS optimize_throw_if_noop = 1 -- emits

OPTIMIZE TABLE "x\\"".t FINAL SETTINGS optimize_throw_if_noop = 1 --" FINAL

and the arm asserts the naive spelling "x". never appears. Two more arms cover a bare target ending in a backslash and the escaper itself (escape("x\") is "x\\"). The provider doc records the override and its reason (§2.3 member table, [§8](#8-maintenance)).

2. Audit container. container?: string on AuditEvent and on the stdout AuditLogLine, spread only when set, the way reason and bucket are. The maintenance route passes the request's container through. Arm: two requests differing only in container produce two events that differ in container, and a whole-database request records none.

3. Route 400. Type-checked before any provider is opened:

{ "error": "\"container\" must be a string naming the target's container" }  with 400

An empty string is not malformed: it reads as a request that named no container, the same way an empty target reads as the whole-database form. Arms cover both.

4. docs/API_DOCS.md. container now has a field row on the route's request table, a mention in the request example, and the 400 it answers in the error section, with the wording moved to "a fifth 400" since the type check is a new one.

5. Couchbase. The analyze reply names the keyspace the statement addressed: Updated statistics for \travel`.`inventory`.`hotel`, not the bare target. Arm asserts the exact string after runMaintenance("analyze", "inventory.hotel")`; the provider doc's analyze row records it.

6. Stacked docblocks. libsql and trino each had two docblocks on runMaintenance; merged into one in each.

Verification.

  • bun tests/run-tests.ts tests/integration/db/clickhouse-provider.test.ts tests/api/db/maintenance.test.ts tests/integration/db/couchbase-provider.test.ts tests/security/audit-redaction.test.ts -> 4 files passed, 404 tests pass (212 + 28 + 132 + 32).
  • bun tests/run-tests.ts --jobs=4 (full suite) -> 599 files passed, 20762 tests: 20753 pass, 9 skip. The default 11-job run is flaky on this machine under load (load average 18 on 11 CPUs) and fails 15 files that pass alone; the same 15 fail identically at afdb7919 with the change stashed, so it is contention rather than this diff. 12 helm-chart files were not run: the chart's postgresql dependency is not built here.
  • bun tests/run-tests.ts --coverage --merge-into=coverage/lcov.info --jobs=4 then bun run coverage:check -> check-coverage: OK - 63809/63809 lines (100.00%).
  • bun run format, bun run lint (0 errors, 246 pre-existing warnings), bun run typecheck all clean.

One note beyond the six: my edits moved lines that five docblocks elsewhere cite by number (the route's 108-131 and 117, two audit.ts ranges, two API_DOCS.md lines). Renumbered in the same commit.

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.
@sloemo01

Copy link
Copy Markdown
Contributor Author

The head moved to 0ab20270, a punctuation-only follow-up to the docs lines from the reply above. All checks are green on it.

…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 cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cevheri
cevheri merged commit 1b38970 into libredb:main Sep 27, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] on the monitoring page, the 3 action buttons show "Relation does not exist"

2 participants