feat(clickhouse): add declarative schema alongside migrations - #111079
rorylshanks wants to merge 2 commits into
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🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong. ✅ Duplication (Python) — cleanNew Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying. ✅ Duplication (TypeScript) — cleanNew TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying. ✅ Bundle size — no changeUncompressed size of every built Total: 69.79 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 854 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 92.7 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 29.0 KiB | ../node_modules/.pnpm/zod@4.3.6/node_modules/zod/v4/core/schemas.js |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Largest files eagerly shipped from src/scenes/dashboard/Dashboard.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Largest files eagerly shipped from src/scenes/activity/explore/EventsScene.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
Largest files eagerly shipped from src/scenes/session-recordings/detail/SessionRecordingDetail.tsx
| Size | File |
|---|---|
| 315.5 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.20 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.20 MiB · 19 files | no change | ████░░░░░░ 38.4% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 44 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 835.5 KiB | dist/toolbar/toolbar-app-FSDWO46I.css |
| 657.5 KiB | dist/toolbar/chunk-chunk-BALLB66W.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-7JWMBALG.js |
| 138.2 KiB | dist/toolbar/chunk-chunk-RXJG6CZC.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-YIYWW6XJ.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-MM7MZI2L.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-EZFR5QGQ.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — no change
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 960.35 MiB · no change
|
[Critical risk] Adds infrastructure dispatch that triggers production schema changes. The PR should not merge until the pull-request job limits credential exposure and the catalogue preserves existing custom metrics. Reviews (1) · Last reviewed commit: "feat(clickhouse): add declarative schema..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| env: | ||
| DOCKERHUB_USERNAME: ${{ vars.DOCKERHUB_USER }} | ||
| DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} |
There was a problem hiding this comment.
If the Docker Hub token is configured, a same-repository pull request that changes bin/clickhouse-schema can read it: this job checks out and runs that script with DOCKERHUB_TOKEN in the job environment. The author could transmit the credential. Scope the token to the Docker Hub login step instead.
How this was verified: The pull-request checkout supplies the executed script, and the Docker Hub token is assigned to its job environment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci-clickhouse-schema.yml
Line: 36-38
Comment:
**Token exposed to PR code**
If the Docker Hub token is configured, a same-repository pull request that changes `bin/clickhouse-schema` can read it: this job checks out and runs that script with `DOCKERHUB_TOKEN` in the job environment. The author could transmit the credential. Scope the token to the Docker Hub login step instead.
**How this was verified:** The pull-request checkout supplies the executed script, and the Docker Hub token is assigned to its job environment.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| query = <<-SQL | ||
| SELECT * REPLACE (toFloat64(value) AS value) | ||
| FROM ${var.database}.custom_metrics_test | ||
| UNION ALL | ||
| SELECT * REPLACE (toFloat64(value) AS value) | ||
| FROM ${var.database}.custom_metrics_replication_queue |
There was a problem hiding this comment.
When the catalogue adopts a database built by the active Python migrations, it replaces custom_metrics with a view that omits the events-recent ingestion-lag metric. It also includes counter metrics only in test mode, although the active migration enables them. Those metrics disappear from the scraped endpoint. Preserve both sources in the declared view.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/clickhouse/schema/catalog/custom_metrics/main.tf
Line: 25-30
Comment:
**Existing metrics disappear**
When the catalogue adopts a database built by the active Python migrations, it replaces `custom_metrics` with a view that omits the events-recent ingestion-lag metric. It also includes counter metrics only in test mode, although the active migration enables them. Those metrics disappear from the scraped endpoint. Preserve both sources in the declared view.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| topic = "${var.database}_input,${var.database}_extra" | ||
| columns = local.input_columns | ||
| } | ||
| mv_select = "team_id, timestamp, value" |
There was a problem hiding this comment.
The wrapper supports a custom native port and secure connection, but this fixture always uses native port 9000, and the check queries plain HTTP on port 8123. Developers using other local connection settings cannot run the check against their server. Pass the selected connection settings to both clients.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/clickhouse/schema/lib/table_family/tests/fixture/main.tf
Line: 59-62
Comment:
**Family check ignores connection settings**
The wrapper supports a custom native port and secure connection, but this fixture always uses native port 9000, and the check queries plain HTTP on port 8123. Developers using other local connection settings cannot run the check against their server. Pass the selected connection settings to both clients.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| `bin/migrate --scope=clickhouse` and `python manage.py migrate_clickhouse` create the database, call the script and load the reference data. | ||
| The test suite builds its databases the same way, with `CLICKHOUSE_SCHEMA_KAFKA=false` and `CLICKHOUSE_SCHEMA_TEST=true`. Each regular test process recreates its database first and supplies a complete Keeper path with a unique run ID and the `{table}` macro. This avoids reconciling fixture definitions and reusing paths still owned by asynchronous table drops. AI evaluations retain their database between runs. | ||
| It records that initial schema once per test process and restores it between packages and after destructive fixtures. |
There was a problem hiding this comment.
These lines say migration commands and tests already use the declarative schema, but this foundation-stage PR leaves them on Python migrations. An operator could mistake a normal migration or test run for validation of the new catalogue. Mark this as future behavior or move it to the cutover documentation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/clickhouse/schema/README.md
Line: 168-170
Comment:
**Cutover described as active**
These lines say migration commands and tests already use the declarative schema, but this foundation-stage PR leaves them on Python migrations. An operator could mistake a normal migration or test run for validation of the new catalogue. Mark this as future behavior or move it to the cutover documentation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| finally: | ||
| run("destroy", "-auto-approve", "-no-color") |
There was a problem hiding this comment.
If an apply or assertion fails and destroy also fails, the cleanup error replaces the original error. CI then reports the failed cleanup instead of the schema-check failure that needs fixing. Preserve the original error while attempting cleanup.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/clickhouse/schema/lib/table_family/tests/check.py
Line: 138-139
Comment:
**Cleanup can hide failure**
If an apply or assertion fails and `destroy` also fails, the cleanup error replaces the original error. CI then reports the failed cleanup instead of the schema-check failure that needs fixing. Preserve the original error while attempting cleanup.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughAdds an OpenTofu catalogue for ClickHouse table families, dictionaries, views, and ingestion resources. Adds local execution tools, integration checks, CI validation, and a dispatch workflow for schema changes. The catalogue covers event, identity, session, metrics, log, trace, and reference-data schemas. Documentation describes the staged rollout alongside existing Python migrations. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to The declarative schema is additive, but its runner can apply the catalogue to any ClickHouse database without confirmation. Some catalogue objects would also misroute or lose ingested rows on a fresh apply. Resolve these issues before relying on the canary rollout. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The staged rollout keeps existing migrations authoritative and does not directly apply schemas to production. However, the new notification path inherits infrastructure-repository credentials, and credential transport, local state confidentiality, and interrupted-apply recovery remain incompletely established. Registry-secret exposure also follows an existing CI pattern. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
posthog/clickhouse/schema/lib/table/main.tf-149-149 (1)
149-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA
modify_columnsentry replaces the whole column object.
try(local.o.modify_columns[c.name], c)swaps in the override object as-is. An override that sets only{ type = "..." }drops the column'sname. Whennameis missing, the provider gets a column without a name, or the type conversion to thecolumnslist fails. Merge the override into the base column so a partial modification keeps the other attributes.Proposed fix
- [for c in var.columns : try(local.o.modify_columns[c.name], c) if !contains(local.drop_columns, c.name)], + [for c in var.columns : merge(c, try(local.o.modify_columns[c.name], {})) if !contains(local.drop_columns, c.name)],posthog/clickhouse/schema/README.md-168-169 (1)
168-169: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the migration and test setup instructions.
These lines say
bin/migrate,migrate_clickhouse, and the test suite use this runner. The staged-rollout section says they still use Python during this foundation stage. Describe the current commands here and move the OpenTofu setup instructions to the cutover documentation. Otherwise, operators can mistake a Python migration run for validation of this catalogue..github/workflows/cd-clickhouse-schema-dispatch.yml-28-35 (1)
28-35: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick winSecurity Misconfiguration
Reachability: Internal
Exploitability: Difficult
CWE: CWE-250Limit the app token to
contents: write.The step passes no
permission-*inputs, so the minted token gets every permission of the installation onposthog-cloud-infra. zizmor reports this. The token then goes to the third-party actionpeter-evans/repository-dispatch. A repository dispatch needs onlycontents: write. Without a limit, a compromised action gets write access beyond what the dispatch needs.Proposed fix
owner: PostHog repositories: posthog-cloud-infra + permission-contents: writeAs per coding guidelines: "Cross-repo tokens set explicit
owner:+repositories:(least privilege)." The repository and owner are already scoped, but the permissions are not.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 29a76b85-9c78-4a6a-a98f-66151915fa23
📒 Files selected for processing (79)
.github/workflows/cd-clickhouse-schema-dispatch.yml.github/workflows/ci-clickhouse-schema.ymlbin/clickhouse-schemadocker/clickhouse/config.d/default.xmldocs/published/handbook/engineering/databases/schema-changes.mdhogli.yamlposthog/clickhouse/schema/.gitignoreposthog/clickhouse/schema/README.mdposthog/clickhouse/schema/catalog/adhoc_events_deletion/main.tfposthog/clickhouse/schema/catalog/ai_events/main.tfposthog/clickhouse/schema/catalog/app_metrics/main.tfposthog/clickhouse/schema/catalog/billing_usage_records.tfposthog/clickhouse/schema/catalog/channel_definition/channel_definitions.jsonposthog/clickhouse/schema/catalog/channel_definition/main.tfposthog/clickhouse/schema/catalog/clickhouse_cleanup/main.tfposthog/clickhouse/schema/catalog/cohort_membership/main.tfposthog/clickhouse/schema/catalog/cohortpeople/main.tfposthog/clickhouse/schema/catalog/custom_metrics/main.tfposthog/clickhouse/schema/catalog/distinct_id_usage/main.tfposthog/clickhouse/schema/catalog/dmat_slot_assignments/main.tfposthog/clickhouse/schema/catalog/document_embeddings/main.tfposthog/clickhouse/schema/catalog/duplicate_events/main.tfposthog/clickhouse/schema/catalog/error_tracking/main.tfposthog/clickhouse/schema/catalog/events/main.tfposthog/clickhouse/schema/catalog/events_dead_letter_queue/main.tfposthog/clickhouse/schema/catalog/events_json/main.tfposthog/clickhouse/schema/catalog/events_recent/main.tfposthog/clickhouse/schema/catalog/events_team_daily_stats/main.tfposthog/clickhouse/schema/catalog/exchange_rate/main.tfposthog/clickhouse/schema/catalog/experiments_preaggregated/main.tfposthog/clickhouse/schema/catalog/flag_evaluations/main.tfposthog/clickhouse/schema/catalog/groups/main.tfposthog/clickhouse/schema/catalog/heatmaps/main.tfposthog/clickhouse/schema/catalog/hog_invocation_results/main.tfposthog/clickhouse/schema/catalog/ingestion_warnings/main.tfposthog/clickhouse/schema/catalog/llma_metrics_daily/main.tfposthog/clickhouse/schema/catalog/log_entries/main.tfposthog/clickhouse/schema/catalog/logs/main.tfposthog/clickhouse/schema/catalog/main.tfposthog/clickhouse/schema/catalog/marketing_preaggregated/main.tfposthog/clickhouse/schema/catalog/message_assets/main.tfposthog/clickhouse/schema/catalog/metrics/main.tfposthog/clickhouse/schema/catalog/performance_events/main.tfposthog/clickhouse/schema/catalog/person/main.tfposthog/clickhouse/schema/catalog/person_distinct_id/main.tfposthog/clickhouse/schema/catalog/person_distinct_id_overrides/main.tfposthog/clickhouse/schema/catalog/person_group_membership.tfposthog/clickhouse/schema/catalog/person_overrides/main.tfposthog/clickhouse/schema/catalog/person_static_cohort/main.tfposthog/clickhouse/schema/catalog/pg_embeddings/main.tfposthog/clickhouse/schema/catalog/platform_alert_events/main.tfposthog/clickhouse/schema/catalog/plugin_log_entries/main.tfposthog/clickhouse/schema/catalog/preaggregation_results/main.tfposthog/clickhouse/schema/catalog/precalculated/main.tfposthog/clickhouse/schema/catalog/property_definitions.tfposthog/clickhouse/schema/catalog/property_values/main.tfposthog/clickhouse/schema/catalog/query_log_archive/main.tfposthog/clickhouse/schema/catalog/raw_sessions/main.tfposthog/clickhouse/schema/catalog/session_replay/main.tfposthog/clickhouse/schema/catalog/sessions/main.tfposthog/clickhouse/schema/catalog/system_processes/main.tfposthog/clickhouse/schema/catalog/tophog/main.tfposthog/clickhouse/schema/catalog/traces/main.tfposthog/clickhouse/schema/catalog/usage_report_events_preagg/main.tfposthog/clickhouse/schema/catalog/web_bot_definition/main.tfposthog/clickhouse/schema/catalog/web_bot_definition/web_bot_definitions.jsonlposthog/clickhouse/schema/catalog/web_preaggregated/main.tfposthog/clickhouse/schema/checksums.txtposthog/clickhouse/schema/lib/dictionary/main.tfposthog/clickhouse/schema/lib/materialized_view/main.tfposthog/clickhouse/schema/lib/table/main.tfposthog/clickhouse/schema/lib/table_family/main.tfposthog/clickhouse/schema/lib/table_family/tests/check.pyposthog/clickhouse/schema/lib/table_family/tests/fixture/main.tfposthog/clickhouse/schema/lib/view/main.tfposthog/clickhouse/schema/local/main.tfposthog/clickhouse/schema/local/modules.tfposthog/clickhouse/schema/local/variables.tfposthog/clickhouse/schema/provider-version.txt
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.
| env: | ||
| DOCKERHUB_USERNAME: ${{ vars.DOCKERHUB_USER }} | ||
| DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | ⚡ Quick win
Sensitive Data Exposure
Reachability: External
Exploitability: Moderate
CWE: CWE-200 — Exposure of Sensitive Information to an Unauthorized Actor
Keep the Docker Hub token out of PR-controlled steps.
The job-level DOCKERHUB_TOKEN remains available when this workflow executes the checked-out bin/clickhouse-schema. A same-repository PR that receives secrets can read the token from that process environment. Scope the token to the Docker login step and confirm that same-repository PR authors are trusted with this registry credential.
|
|
||
| case "${1:-apply}" in | ||
| test-family) export TOFU="$TOFU"; exec python3 "$SCHEMA_DIR/lib/table_family/tests/check.py" ;; | ||
| apply) exec "$TOFU" -chdir="$ROOT_DIR" apply -auto-approve -parallelism=16 "${@:2}" ;; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Restrict apply to an approved schema target.
apply -auto-approve accepts any CLICKHOUSE_HOST and CLICKHOUSE_DATABASE. An operator with the normal application environment can therefore apply this unadopted catalogue to an existing database. Require an explicit isolated-target or adopted-canary confirmation before apply; keep plan available for inspection. This enforces the foundation-stage boundary stated in posthog/clickhouse/schema/README.md. As per coding guidelines, “Apply the new catalogue only to an isolated database or an explicitly adopted infrastructure canary.”
Source: Coding guidelines
| module "writable_error_tracking_issue_fingerprint_embeddings" { | ||
| source = "../../lib/table" | ||
|
|
||
| enabled = local.write && !contains(local.deployment.exclude, "writable_error_tracking_issue_fingerprint_embeddings") | ||
| database = var.database | ||
| name = "writable_error_tracking_issue_fingerprint_embeddings" | ||
| engine = "Distributed('posthog_single_shard', '${var.database}', 'error_tracking_issue_fingerprint_embeddings')" | ||
| columns = concat(local.kafka_error_tracking_issue_fingerprint_embeddings_columns, [ | ||
| { name = "_timestamp", type = "DateTime" }, | ||
| { name = "_offset", type = "UInt64" }, | ||
| { name = "_partition", type = "UInt64" }, | ||
| ]) | ||
| override = try(local.deployment.overrides["writable_error_tracking_issue_fingerprint_embeddings"], {}) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n -C2 'error_tracking_issue_fingerprint_embeddings' posthog/clickhouse/schemaRepository: PostHog/posthog
Length of output: 6286
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- error tracking catalogue ---'
sed -n '1,245p' posthog/clickhouse/schema/catalog/error_tracking/main.tf
printf '%s\n' '--- exact repository references ---'
rg -n -C2 '(^|[^A-Za-z0-9_])error_tracking_issue_fingerprint_embeddings([^A-Za-z0-9_]|$)' .
printf '%s\n' '--- table family and table module definitions ---'
rg -n -C3 'module ".*table_family|source = .*table_family|module ".*table"|source = "../../lib/table"' posthog/clickhouse/schema/catalog posthog/clickhouse/schema/lib
printf '%s\n' '--- diff against supplied merge base ---'
git diff --stat 45542e4461a83703e27f21f0a9c330df39f74ac1 4a0ca022cda45926eb7239baf7020994a44da4cd -- posthog/clickhouse/schema
git diff --unified=25 45542e4461a83703e27f21f0a9c330df39f74ac1 4a0ca022cda45926eb7239baf7020994a44da4cd -- posthog/clickhouse/schema/catalog/error_tracking/main.tf
printf '%s\n' '--- table module files ---'
find posthog/clickhouse/schema -path '*/lib/*' -type f -maxdepth 6 -printRepository: PostHog/posthog
Length of output: 41079
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- legacy embeddings migrations ---'
rg -n -C12 'error_tracking_issue_fingerprint_embeddings|0153|0155' posthog/clickhouse/migrations
printf '%s\n' '--- table module ---'
cat -n posthog/clickhouse/schema/lib/table/main.tf
printf '%s\n' '--- table family module ---'
cat -n posthog/clickhouse/schema/lib/table_family/main.tf
printf '%s\n' '--- legacy HCL embeddings declarations ---'
sed -n '720,790p' posthog/clickhouse/hcl/roles/ingestion_small/local/tables.hcl
sed -n '3700,3830p' posthog/clickhouse/hcl/sql/local-single/all.sqlRepository: PostHog/posthog
Length of output: 40354
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration 0153 ---'
sed -n '1,260p' posthog/clickhouse/migrations/0153* 2>/dev/null || true
printf '%s\n' '--- migration 0155 ---'
sed -n '1,180p' posthog/clickhouse/migrations/0155_add_document_embeddings_table.py
printf '%s\n' '--- embedding SQL definitions ---'
rg -n -C20 'ERROR_TRACKING_FINGERPRINT_EMBEDDINGS|error_tracking_issue_fingerprint_embeddings|DOCUMENT_EMBEDDINGS_TABLE_SQL|DOCUMENT_EMBEDDINGS_WRITABLE_TABLE_SQL' products/error_tracking posthog/clickhouse/migrations/0153* posthog/clickhouse/migrations/0155_add_document_embeddings_table.py
printf '%s\n' '--- schema root inclusion and deployment ---'
rg -n -C5 'error_tracking|catalog' posthog/clickhouse/schema/catalog/main.tf posthog/clickhouse/schema/local posthog/clickhouse/schema/README.mdRepository: PostHog/posthog
Length of output: 43327
Declare the storage table for error_tracking_issue_fingerprint_embeddings.
writable_error_tracking_issue_fingerprint_embeddings is a Distributed table that targets error_tracking_issue_fingerprint_embeddings. This catalogue declares no storage table for that target. A fresh schema apply can therefore create the Distributed table without its remote storage table. When the materialized view consumes a row, forwarding can fail and pending data can remain queued on disk.
Add a storage-only table_family for error_tracking_issue_fingerprint_embeddings. Match the legacy ReplacingMergeTree schema and placement, and avoid recreating the existing writable, Kafka, and materialized-view objects.
| enabled = local.storage && !contains(local.deployment.exclude, "query_log_archive_buffer") | ||
| database = var.database | ||
| name = "query_log_archive_buffer" | ||
| engine = "Buffer('posthog', 'sharded_query_log_archive', 16, 10, 60, 10000, 1000000, 10000000, 100000000)" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The Buffer table always writes to the posthog database.
Buffer('posthog', 'sharded_query_log_archive', ...) ignores var.database. Writes arrive through writable_query_log_archive, which targets ${var.database}.query_log_archive_buffer. When database is not posthog, for example in an isolated test database, the Buffer flushes rows into posthog.sharded_query_log_archive. That is the wrong table, and it may not exist. Every other object in this file follows var.database.
Proposed fix
- engine = "Buffer('posthog', 'sharded_query_log_archive', 16, 10, 60, 10000, 1000000, 10000000, 100000000)"
+ engine = "Buffer('${var.database}', 'sharded_query_log_archive', 16, 10, 60, 10000, 1000000, 10000000, 100000000)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| engine = "Buffer('posthog', 'sharded_query_log_archive', 16, 10, 60, 10000, 1000000, 10000000, 100000000)" | |
| engine = "Buffer('${var.database}', 'sharded_query_log_archive', 16, 10, 60, 10000, 1000000, 10000000, 100000000)" |
This branch has not been deployed
1 participant
Problem
Operators can validate the declarative schema while existing migration execution remains in service.
Changes
Adds the schema catalogue, pinned tooling, validation and dispatch workflows. Provisions the logs cluster and system log tables required by fresh schema application. Existing Python migrations, HCL checks and test setup remain active.
Rollout gate: Merge first, then merge infra canary https://github.com/PostHog/posthog-cloud-infra/pull/10906. Select the merged catalogue commit and verify single-view import-only adoption before the next layer. Merge bottom-first; do not land the whole stack together.
Before:
After:
How did you test this code?
Local catalogue validation and table-family checks passed. The foundation created all 381 resources in a fresh isolated database with Kafka ingestion enabled; the subsequent plan was empty.
posthog/clickhouse/test/test_managed_schema.py --reuse-dbpassed against local services. Python lint/format, lockfile and workflow checks passed. Live cloud rollout, the complete backend suite and both events-schema variants remain CI/review gates.Test rationale: The imported managed-schema regression checks reconstruction after fixture restore. Existing table-family checks validate the catalogue and require an empty second plan; no split-only tests were added.
Release status
Automatic notifications
Docs update
Updated the existing schema README and applicable schema-change guidance to describe this rollout stage.
🤖 Agent context
Autonomy: Human-driven (agent-assisted). Agent: Codex, GPT-6.
Staged replacement for #109937, which remains available for comparison. Source changes were split using Git, GitHub CLI and
gh stack; current master changes were retained. Skills: /stacking-prs, /writing-pr-descriptions, /running-ci-preflight, /authoring-ci-workflows, /clickhouse-migrations, /writing-tests, /writing-code-comments and /reviewing-with-coderabbit. Local CodeRabbit review was skipped at the directing user's request because the CLI was missing. No customer material was used. Human review and CI patch coverage are pending on this draft.