fix(data-warehouse): version duckgres import table names - #76032
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/warehouse_sources/backend/facade/duckgres.py |
75.0% | 14 |
products/warehouse_sources/backend/duckgres_naming.py |
89.5% | 28, 30 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 30777330481 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
tasks |
██████████████░░░░░░ 70.3% |
33,203 / 47,255 |
signals |
████████████████░░░░ 81.5% |
25,406 / 31,169 |
cdp |
████████████████░░░░ 82.1% |
3,285 / 3,999 |
data_modeling |
█████████████████░░░ 85.7% |
7,720 / 9,012 |
notebooks |
█████████████████░░░ 86.0% |
7,794 / 9,060 |
managed_warehouse |
█████████████████░░░ 86.3% |
5,514 / 6,389 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
data_warehouse |
█████████████████░░░ 87.4% |
10,967 / 12,547 |
cohorts |
█████████████████░░░ 87.5% |
6,393 / 7,309 |
exports |
██████████████████░░ 87.9% |
7,078 / 8,054 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
engineering_analytics |
██████████████████░░ 89.3% |
6,529 / 7,309 |
dashboards |
██████████████████░░ 89.5% |
5,989 / 6,693 |
conversations |
██████████████████░░ 89.5% |
17,703 / 19,783 |
alerts |
██████████████████░░ 90.3% |
4,482 / 4,966 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
error_tracking |
██████████████████░░ 91.0% |
10,928 / 12,010 |
slack_app |
██████████████████░░ 91.1% |
9,664 / 10,610 |
marketing_analytics |
██████████████████░░ 91.2% |
12,111 / 13,284 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
product_analytics |
███████████████████░ 92.5% |
5,849 / 6,321 |
early_access_features |
███████████████████░ 92.6% |
1,287 / 1,390 |
mcp_analytics |
███████████████████░ 92.6% |
3,775 / 4,076 |
ai_observability |
███████████████████░ 92.8% |
15,618 / 16,821 |
surveys |
███████████████████░ 93.2% |
5,853 / 6,281 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
approvals |
███████████████████░ 93.3% |
3,437 / 3,682 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
web_analytics |
███████████████████░ 93.5% |
15,375 / 16,447 |
workflows |
███████████████████░ 94.1% |
7,251 / 7,707 |
endpoints |
███████████████████░ 94.2% |
8,655 / 9,192 |
review_hog |
███████████████████░ 94.6% |
8,101 / 8,563 |
skills |
███████████████████░ 94.6% |
3,158 / 3,337 |
logs |
███████████████████░ 95.4% |
10,487 / 10,987 |
experiments |
███████████████████░ 95.5% |
26,047 / 27,287 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
feature_flags |
███████████████████░ 96.4% |
17,488 / 18,144 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
replay_vision |
███████████████████░ 96.5% |
16,556 / 17,151 |
customer_analytics |
███████████████████░ 97.1% |
10,463 / 10,777 |
warehouse_sources |
███████████████████░ 97.3% |
359,895 / 369,891 |
data_catalog |
████████████████████ 97.8% |
2,589 / 2,648 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
⚠️ Django migration SQL — 1 new migration to review
We've detected new migrations on this PR. Review the SQL output for each migration:
products/warehouse_sources/backend/migrations/0116_add_duckgres_table_name.py
BEGIN;
--
-- Add field duckgres_table_name to externaldataschema
--
ALTER TABLE "posthog_externaldataschema" ADD COLUMN "duckgres_table_name" varchar(63) NULL;
COMMIT;Last updated: 2026-08-03 00:29 UTC (a9fda31)
✅ Django migration risk — no migrations to analyze
No Django migrations need risk analysis.
Prompt To Fix All With AI### Issue 1
products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/duckgres/processor.py:607-608
**Legacy table names change**
When an existing unpinned schema contains a camel-cased source type or prefix, or its physical name exceeds 63 characters, delegating to `duckgres_data_imports_table_name` replaces the v3 sink's previous camel-case splitting and tagged truncation with a different fallback. Subsequent batches target a different physical table, splitting new imports from previously written data and making the existing data disappear from queries.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile |
| def _duckgres_table_name(schema: ExternalDataSchema) -> str: | ||
| source_type = schema.source.source_type | ||
| normalized_name = schema.normalized_name | ||
| raw_name = ( | ||
| f"{source_type}_{schema.source.prefix}_{normalized_name}" | ||
| if schema.source.prefix | ||
| else f"{source_type}_{normalized_name}" | ||
| ) | ||
| return NamingConvention.normalize_identifier(raw_name, max_length=63) | ||
| return duckgres_data_imports_table_name(schema) |
There was a problem hiding this comment.
When an existing unpinned schema contains a camel-cased source type or prefix, or its physical name exceeds 63 characters, delegating to duckgres_data_imports_table_name replaces the v3 sink's previous camel-case splitting and tagged truncation with a different fallback. Subsequent batches target a different physical table, splitting new imports from previously written data and making the existing data disappear from queries.
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: 607-608
Comment:
**Legacy table names change**
When an existing unpinned schema contains a camel-cased source type or prefix, or its physical name exceeds 63 characters, delegating to `duckgres_data_imports_table_name` replaces the v3 sink's previous camel-case splitting and tagged truncation with a different fallback. Subsequent batches target a different physical table, splitting new imports from previously written data and making the existing data disappear from queries.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| cleaned = default_prefix | ||
| if cleaned[0].isdigit(): | ||
| cleaned = f"{default_prefix}_{cleaned}" | ||
| return cleaned[:_DUCKGRES_IDENTIFIER_MAX_LENGTH] |
There was a problem hiding this comment.
Medium: Colliding table names bypass table access controls
A user allowed to create a warehouse source can choose a long prefix whose first 63 normalized characters match another source's name. Because binding has no per-team uniqueness check, both logical tables are pinned to the same physical table; subsequent syncs overwrite or append to that shared table, and querying the attacker's authorized logical table can return data from the access-restricted table. Preserve the legacy candidate for the first binding, but detect an existing pin within the team and assign a deterministic collision-resistant suffix before persisting it.
PR overviewThis PR updates Duckgres warehouse import table naming to use versioned names when binding logical imports to physical tables. One security issue remains open: a user who can create warehouse sources may craft a long name that collides with another source’s normalized physical table name. This can cause cross-source overwrites or appends and allow the user to query data from an access-restricted table. No reported issues have been addressed yet. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
|
Reviews (2): Last reviewed commit: "refactor(data-warehouse): derive Duckgre..." | Re-trigger Greptile |
b2f94cd to
a211ee8
Compare
fuziontech
left a comment
There was a problem hiding this comment.
Automated review generated on behalf of @fuziontech.
Blocking: products/managed_warehouse/backend/common.py:552 makes every reader and writer derive the physical table from one org-level naming version, but an existing org can already contain both copy-managed sources (written with copy_v1) and v3-sink sources (written with legacy_batch_v1; ownership is selected per source type in ducklake_copy_data_imports_workflow.py:252). The documented migration policy assigns existing orgs legacy_batch_v1, so existing copy-created tables such as tiktokads_ad_report immediately resolve as tik_tok_ads_ad_report without any table migration. Queries will fail or read a newly created table lacking history, while mixed-version workers can write different tables during rollout. Please preserve the physical name per external schema, or atomically migrate/re-prime existing tables with a coordinated compatibility rollout before switching resolution.
fuziontech
left a comment
There was a problem hiding this comment.
on second thought....why not 👍
Problem
Duckgres data-import writers have two historical physical table-name formats. The copy workflow preserves source slugs such as
tiktokads, while the batch sink uses DLT-compatible snake case such astik_tok_ads. Deriving the name independently on every path can make writers and readers target different tables, while changing one global algorithm can rename tables that already contain data.Newly provisioned organizations should use the copy workflow format. Existing batch-sink organizations must retain their current format without adding naming state to each external schema.
Changes
The Duckgres control plane now owns the organization-level naming policy, and every data-import path resolves the physical table name from that policy:
flowchart LR Policy[Duckgres organization policy] --> Resolver[Versioned table-name resolver] Resolver --> Copy[Copy workflow] Resolver --> Register[Registration workflow] Resolver --> Batch[Batch sink] Resolver --> Reader[Query binding] Copy --> Table[(Physical Duckgres table)] Register --> Table Batch --> Table Reader --> Table classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class Policy phRed; class Resolver,Copy,Register,Batch,Reader phBlue; class Table phGray;copy_v1keeps the copy workflow format andlegacy_batch_v1keeps DLT-compatible snake case.Important
Deploy PostHog/duckgres#1022 and apply any operator-managed organization policy overrides before deploying this PR.
How did you test this code?
DEBUG=1 ./manage.py makemigrations warehouse_sources --dry-run --check(No changes detected)uv run mypy --cache-fine-grained .(Success: no issues found in 17387 source files)./bin/hogli ci:preflight --fix --against refs/remotes/origin/pr-74277-merged(zero failures)The tests cover policy propagation, rolling-deploy fallback, exact naming behavior, and the regression where independent writers could resolve different physical names. I did not run an end-to-end production import.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
Updated
products/managed_warehouse/backend/README.mdwith the organization-level table-naming contract.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Codex Desktop implemented and validated this change. No shareable agent session link is available. Repo skills used:
/django-migrations,/writing-tests,/writing-code-comments,/writing-user-facing-copy,/running-ci-preflight, and/implementing-warehouse-sources, plus the GitHub publishing skills.The selected design keeps the rollout policy and durable naming decision at organization scope in Duckgres. PostHog reads that policy at runtime and uses one pure versioned resolver across every writer and reader, without persisting duplicate state on
ExternalDataSchema.