Skip to content

fix(clickhouse): writable_events doesn't exist on DATA#61036

Merged
fuziontech merged 1 commit into
masterfrom
pawel/fix/migration
Jun 1, 2026
Merged

fix(clickhouse): writable_events doesn't exist on DATA#61036
fuziontech merged 1 commit into
masterfrom
pawel/fix/migration

Conversation

@orian
Copy link
Copy Markdown
Contributor

@orian orian commented Jun 1, 2026

Problem

migration cannot be applied

Changes

remove DATA node as a target for alter table on writable_events

@orian orian requested a review from a team as a code owner June 1, 2026 23:08
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Comments Outside Diff (1)

  1. posthog/clickhouse/migrations/0267_wire_up_existing_dmat_string_columns.py, line 108-119 (link)

    P2 The comment still says "writable_events lives on DATA in every install" but the fix establishes the opposite for cloud: on cloud, writable_events only exists on INGESTION_EVENTS, not DATA. A reader following the comment will be confused about why DATA was removed from the cloud case.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: posthog/clickhouse/migrations/0267_wire_up_existing_dmat_string_columns.py
    Line: 108-119
    
    Comment:
    The comment still says "writable_events lives on DATA in every install" but the fix establishes the opposite for cloud: on cloud, `writable_events` only exists on `INGESTION_EVENTS`, not `DATA`. A reader following the comment will be confused about why `DATA` was removed from the cloud case.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.

    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!

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
posthog/clickhouse/migrations/0267_wire_up_existing_dmat_string_columns.py:108-119
The comment still says "writable_events lives on DATA in every install" but the fix establishes the opposite for cloud: on cloud, `writable_events` only exists on `INGESTION_EVENTS`, not `DATA`. A reader following the comment will be confused about why `DATA` was removed from the cloud case.

```suggestion
# writable_events lives on INGESTION_EVENTS on cloud (added by migration 0232) and on
# DATA on non-cloud installs. The two are mutually exclusive: DATA does not host
# writable_events on cloud, so we target only the role that actually has the table.
# It is a Distributed engine table (not sharded, not replicated), so neither flag
# applies — explicit False is required to satisfy the migration convention check.
operations += [
    run_sql_with_exceptions(
        ALTER_TABLE_ADD_DMAT_STRING_COLUMNS(
            table="writable_events",
            start=_STRING_RANGE_START,
            end_exclusive=_STRING_RANGE_END,
        ),
        node_roles=([NodeRole.INGESTION_EVENTS] if _is_cloud else [NodeRole.DATA]),
```

Reviews (1): Last reviewed commit: "writable_events doesn't exist on DATA" | Re-trigger Greptile

@orian orian enabled auto-merge (squash) June 1, 2026 23:11
@fuziontech fuziontech disabled auto-merge June 1, 2026 23:12
@fuziontech fuziontech merged commit ca85437 into master Jun 1, 2026
252 of 256 checks passed
@fuziontech fuziontech deleted the pawel/fix/migration branch June 1, 2026 23:12
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

🎭 Playwright report

2 failed tests:

  • create experiment via wizard, add metrics, and launch (chromium)
  • Deleting a dashboard navigates to the dashboards list (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this comment? Help fix flakies and failures and it'll disappear!

@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented Jun 1, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-06-01 23:34 UTC Run
prod-us ✅ Deployed 2026-06-02 10:18 UTC Run
prod-eu ✅ Deployed 2026-06-02 10:20 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants