perf(data-warehouse): probe target table for existence instead of information_schema - #73065
Conversation
…kgres sink _table_exists runs SELECT 1 FROM information_schema.tables on every non-first batch. On the dogfood org's DuckLake catalog (3,173 tables, 80k snapshots, 577k column-version rows) that forces duckgres to re-materialize its whole-catalog compat view, and under the sink's own concurrent snapshot commits the catalog cache is invalidated every batch — measured ~48s per check under load (1.3s idle), the dominant per-apply cost and the reason throughput was flat regardless of table. Cache the positive result per connection (WeakKeyDictionary): a table that exists stays existing for the connection's life, so the enumeration is paid at most once per connection instead of once per batch. Pairs with the existing per-group session reuse. Negatives are not cached (a table may be created between batches). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
😎 This pull request was merged. |
…ormation_schema Replace the sink's per-batch existence check (SELECT 1 FROM information_schema.tables) with a single-table LIMIT-0 probe. The information_schema check forced duckgres to re-materialize its whole DuckLake catalog compat view; under the sink's own concurrent snapshot commits on a large catalog (3.2k tables, 577k column-version rows) that cost ~48s per check — the dominant per-apply cost, and why throughput was flat regardless of table. Measured under identical concurrent write load: the information_schema enumeration is 1.3s idle / ~48s loaded, while the LIMIT-0 probe is 0.11s idle / 0.59s loaded (it uses the catalog's table_id-indexed single-table path instead of a full-catalog scan). duckgres reports a missing table as a generic XX000 with DuckDB's "... does not exist" message, matched precisely; any other error propagates (a transient error read as absent would wrongly pick the create path). Kept the per-connection positive cache so repeat batches skip the probe entirely and the not-found match only runs on a table's first encounter per connection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/duckgres/processor.py:684-685
**Unrelated Missing Objects Look Absent**
The substring also matches errors such as a missing schema, catalog, or secret encountered while resolving the query. In those cases `_table_exists` returns `False`, so the batch selects the create path instead of propagating the real failure, producing a second misleading error and an unnecessary retry.
Reviews (1): Last reviewed commit: "perf(data-warehouse): probe target table..." | Re-trigger Greptile |
| return True | ||
| except psycopg.Error as err: |
There was a problem hiding this comment.
Unrelated Missing Objects Look Absent
The substring also matches errors such as a missing schema, catalog, or secret encountered while resolving the query. In those cases _table_exists returns False, so the batch selects the create path instead of propagating the real failure, producing a second misleading error and an unnecessary retry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/duckgres/processor.py
Line: 684-685
Comment:
**Unrelated Missing Objects Look Absent**
The substring also matches errors such as a missing schema, catalog, or secret encountered while resolving the query. In those cases `_table_exists` returns `False`, so the batch selects the create path instead of propagating the real failure, producing a second misleading error and an unnecessary retry.
How can I resolve this? If you propose a fix, please make it concise.|
Note 🤖 stamphog reviewed Greptile's unresolved P1 comment is confirmed by the diff: the generic "does not exist" substring match will also swallow errors from a missing schema, catalog, or secret encountered while resolving the query, silently treating them as "table absent" and routing to the create path instead of surfacing the real failure — this is a substantive, unaddressed correctness concern in a data-ingestion path.
Gate mechanics and policy version
|
|
The concern is valid and worth addressing. The DuckDB's specific table-missing message (as verified by the test mock itself) is: Since you already have This won't match schema/catalog/secret errors (their messages use different subjects — |
… message Review (Greptile P1 / stamphog): a bare "does not exist" substring also matches "Schema/Catalog/Secret ... does not exist", which would silently route a real resolution failure to the create path. Match DuckDB's specific "Table with name ... does not exist" instead; everything else propagates. Test added for a schema-not-found error propagating. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the P1 (@greptile-apps / @stamphog): the missing-table detection now anchors to DuckDB's specific |
|
Good fix — the
Looks good overall. The fix is precise and the test coverage is thorough (8 cases exercising probe success, probe absent, cache hit, per-table granularity, per-connection isolation, negatives not cached, transient error propagation, and now missing schema propagation). |
There was a problem hiding this comment.
Data-pipeline change to table-existence detection in warehouse sources; the one substantive review concern (a too-broad error-message match that could misroute a real failure to the create path) was fixed in a later commit with a specific new regression test, and Greptile's follow-up comment confirms the fix against the current code. Author has STRONG familiarity (100% of touched lines, 22 recent merged PRs in this path) which stands in for owning-team assurance despite not being formally on the team.
- Author wrote 100% of the modified lines and has 22 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 51L, 1F substantive, 125L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (125L, 2F, single-area, perf) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 54df693 · reviewed head 9e527da |
Problem
The duckgres sink's
_table_exists()runsSELECT 1 FROM information_schema.tableson every non-first batch (via_plan_batch_operation, to choose create vs insert/merge). On a large DuckLake catalog this forces duckgres to re-materialize its whole-cataloginformation_schemacompat view — every current table and column at the current snapshot.Traced end to end on the dogfood org's catalog (mw-prod-us, 2026-07): 3,173 tables, 80,235 snapshots, 577,076
ducklake_columnrows (88% superseded version history), and theducklake_columnindex is(table_id, …)-leading — so an "all current columns" load can't use it and scans the whole history.pg_stat_activity/pg_lockson the catalog Postgres during live sink activity showed zero blocking. It's work: DuckDB caches the catalog and re-loads it whenever the snapshot advances; the sink commits a snapshot per batch, so the cache is invalidated on every batch and each check re-scans the bloated catalog.information_schema.tables= 1.3s idle → ~48s under load (44.8–52.1s every iteration, one concurrent writer was enough). This is a shared per-apply tax paid by every table, which is why sink throughput was flat (~140/hr) regardless of which table a batch hit — and why neither the earlier session-reuse nor co-claim PRs moved it (the cache is invalidated by other sessions' commits).Change
Replace the
information_schemaenumeration with a single-tableLIMIT 0probe (SELECT 1 FROM <schema>.<table> LIMIT 0). It loads only that one table's metadata via the catalog'stable_id-indexed single-table path, so it stays cheap even under concurrent snapshot commits:information_schema.tables(old)LIMIT 0probe (new)duckgres reports a missing table as a generic
XX000carrying DuckDB's stable"... does not exist"catalog message (verified via psycopg — it is not a typedUndefinedTable), so the probe matches that message for "absent" and propagates anything else — a transient error read as "table absent" would wrongly pick the create path (a benign, self-healingCREATE-then-retry, but avoid it). The probe runs on the autocommit connection outside any transaction, so a raised error is inert.Kept the per-connection positive existence cache (
WeakKeyDictionary): a table that exists stays existing for the connection's life (the sink never drops tables mid-run), so repeat batches skip the probe entirely and the not-found match only runs on a table's first encounter per connection.How did you test this code?
I'm an agent; checks actually run:
test_processor.py, 30 total).information_schema, 0.11s→0.59s for the probe), catalog Postgrespg_locks/index inspection, and the exact psycopg error signature. All test artifacts (a temporary user, a scratch schema) were removed.Follow-ups (separate)
🤖 Agent context
Authored by Claude Code (Fable 5) from a multi-day production investigation. Notably this corrected an earlier hypothesis: the throughput bottleneck is not S3 file-count bloat (compaction) but this per-batch
information_schemare-materialization — established by reproducing the 1.3s→48s swing under concurrent snapshot commits, confirming on the catalog Postgres it's re-read work (not lock contention), and then validating that the single-table probe stays sub-second under the same load.🤖 Generated with Claude Code