feat(data-warehouse): make the project warehouse connection read/write #74256
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,10 +210,15 @@ def _row_team_id(row: dict) -> int | None: | |
| return None | ||
|
|
||
|
|
||
| def configure_project_user( | ||
| *, organization_id: UUID | str, team_id: int, table_suffix: str, password: str | ||
| ) -> dict[str, str]: | ||
| """Apply the project's read/write credential, creating its team row only when absent. | ||
|
|
||
| Duckgres issues two team-scoped logins: `project_reader` (read-only) and `project_user` | ||
| (read/write). Both are scoped to exactly the same namespaces — the only difference is | ||
| whether writes are authorized — so a project connection gets the writable one and the | ||
| project boundary is unchanged. | ||
|
|
||
| The org-team row is Duckgres-owned state that also drives external-writer discovery | ||
| (viaduck/millpond write targets) and may be hand-set (break-glass edits, legacy layouts). | ||
|
|
@@ -245,28 +250,29 @@ def configure_project_reader( | |
| credential_response = _request( | ||
| "PUT", | ||
| organization_id, | ||
| f"/teams/{team_id}/project-user", | ||
|
Contributor
There was a problem hiding this comment.
When an existing source is reconciled before Knowledge Base Used: Batch exports and data warehouse bulk data movement Prompt To Fix With AIThis is a comment left during a code review.
Path: products/data_warehouse/backend/presentation/views/managed_warehouse.py
Line: 253
Comment:
**Failed handshake disables existing sources**
When an existing source is reconciled before `/teams/:id/project-user` is available, the credential-kind migration commits catalog deletion and `direct_query_enabled=False` before this request fails, causing the connection to lose its tables and remain unavailable until a later successful sweep.
**Knowledge Base Used:** [Batch exports and data warehouse bulk data movement](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog/-/docs/batch-exports-warehouse.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Member
Author
There was a problem hiding this comment. Agreed, and thanks — this is the same hazard Copilot raised, and it is the one I called out in the PR description as a hard deploy-order requirement: PostHog/duckgres#999 must be deployed before this merges. Full analysis in my reply to Copilot, short version: the obvious fix (handshake before mutating) would regress the concurrency property Current plan is to gate on deploy order (the feature cannot function before #999 ships anyway, and the state self-heals on the next sweep once Duckgres is up). If the team prefers not to rely on sequencing, I will put the migration behind a flag and flip it post-deploy.
Contributor
There was a problem hiding this comment. The concurrency argument holds — the DB write being the claim is load-bearing for Between the two options: the deploy-order gate is fine given that the PR description already calls it out prominently with a hard warning, and the self-healing property means the window of breakage is bounded to the time between this merge and duckgres #999 landing. That's an acceptable operational risk as long as the merge isn't automated. The flag approach is cleaner if there's any chance of the sequencing slipping (e.g., the duckgres PR stalls, someone merges this during an incident window without checking), but it does add a cleanup step. Given you've already documented the hazard clearly in the PR description, the deploy-order gate seems sufficient here — no further change needed from a review standpoint. |
||
| json_body={"password": password}, | ||
| require_enabled=False, | ||
| ) | ||
| if not status.is_success(credential_response.status_code) or not isinstance(credential_response.data, dict): | ||
| raise RuntimeError("Failed to create the project's managed warehouse user") | ||
| username = credential_response.data.get("username") | ||
| response_password = credential_response.data.get("password") | ||
| if not isinstance(username, str) or not username or not isinstance(response_password, str) or not response_password: | ||
| raise RuntimeError("Managed warehouse user response did not include credentials") | ||
| return {"username": username, "password": response_password} | ||
|
|
||
|
|
||
| def project_namespaces(*, organization_id: UUID | str, team_id: int) -> tuple[set[str], set[tuple[str, str]]] | None: | ||
| """Return the (whole schemas, legacy posthog-schema tables) the project's login may see. | ||
|
|
||
| Mirrors the Duckgres policy derivation from the org-team row: the login is granted the row's | ||
| schema_name, its data-imports schema (override or `<schema>_data_imports`), the modeled-data | ||
| schema, and `posthog.<override>` for each non-NULL legacy events/persons override — including | ||
| overrides that spell the derived default name. None means no enabled row exists (fail closed). | ||
|
|
||
| Mode-independent by design: Duckgres derives the SAME namespaces for `project_reader` and | ||
| `project_user`, so this mirror is correct for both and must not gain a mode argument. | ||
| """ | ||
| row = _get_project_team_row(organization_id=organization_id, team_id=team_id) | ||
| if row is None or row.get("enabled") is not True: | ||
|
|
@@ -427,11 +433,12 @@ def _register_provisioning_team(organization_id: UUID | str, team_id: int) -> No | |
|
|
||
|
|
||
| def _ensure_direct_source(team_id: int, organization_id: UUID | str) -> None: | ||
| """Best-effort: register the org's managed warehouse as the team's scoped query connection. | ||
|
|
||
| A managed warehouse speaks the Postgres wire protocol, so each member team gets an | ||
| ExternalDataSource pointed at the org server. Duckgres scopes its credential to the | ||
| project: read/write inside the project's own namespaces, and nothing outside them. | ||
| A failure here must never block onboarding. | ||
| """ | ||
| try: | ||
| # Keep the data_warehouse/warehouse_sources stack off this adapter's import path. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct diagnosis, and greptile flagged the same thing — this is the hazard I called out in the PR description as a hard deploy-order requirement. Agreed it is real: on a kind mismatch the source is re-credentialed, disabled and its catalog dropped before the handshake, so a 404 leaves existing connections dark.
On the two suggested fixes, though — I think the first one would regress a property this file is deliberately built around, and I want to flag that before anyone implements it.
"Mint credentials first, then atomically swap" breaks the concurrency design. The DB write is not just bookkeeping, it is the claim:
ensure_managed_warehouse_direct_sourcepersists the credential underselect_for_updateso a concurrent sweep reads it back and reuses it instead of minting a second password.test_concurrent_credential_setup_reuses_the_persisted_credentialpins exactly that (both re-entrant calls must request the same password). If the handshake moves first, two sweeps mint different passwords; Duckgres's upsert is last-write-wins and so is the row write, and the two can interleave such that the persisted password is not the one Duckgres ended up storing — a permanently broken connection, which is worse than the transient one we are trying to avoid."Treat 404 as keep using project_reader" does not help by itself, because by the time the 404 comes back the wipe has already committed. It would have to be combined with deferring the mutation, which lands back on the problem above.
A fix that preserves the claim-first property would need the new credential staged separately (e.g. a pending-credential field) and promoted only after a successful handshake — real design work, not a review-comment change.
Given that, my inclination is to keep the current behaviour and treat deploy order as the control, because:
If the team would rather not depend on deploy sequencing, the cheapest safe alternative is to merge this behind a flag and flip it after Duckgres is deployed. Happy to do that — flagging the tradeoff rather than picking unilaterally.