Fix replacement in attached Postgres catalogs - #6078
clowrance-proppilot wants to merge 1 commit into
Conversation
Signed-off-by: clowrance-proppilot <clowrance@propertypilot.com>
|
Thanks for tracking this down, and for linking the related issues so clearly. Using the target table's catalog to decide how to replace it, instead of the connection's current catalog, is the right direction. My main concern is the DROP TABLE ... CASCADE. The PR description says SQLMesh recreates the virtual-layer views after physical evaluation. That only happens when a plan is applied, during promotion. A regular sqlmesh run never re-promotes. So every scheduled run of a FULL model in an attached Postgres catalog would: drop the prod virtual-layer view, and the views of any dev environments that share the snapshot, without restoring them until a later plan re-promotes that snapshot. A no-change plan doesn't do that. And the run still reports success. We'd be swapping a loud error for silent breakage, which is harder for users to notice. Separately, DuckDB runs with SUPPORTS_TRANSACTIONS = False, so the DROP followed by the CTAS isn't atomic. If the CTAS fails, the table is gone and there's nothing to roll back to. Could we avoid dropping the table at all? TrinoEngineAdapter.replace_query already solves a similar problem. It checks the target table's catalog type (get_catalog_type_from_table) and passes supports_replace_table_override to the base replace_query. For Postgres-attached catalogs we could pass False whenever the target is Postgres. The existing insert-overwrite path (delete and insert into the table in place) would then handle the refresh. Dependent views and grants stay intact, and no CASCADE is needed. It would also be great to add an integration test against the Docker Postgres setup: run a FULL model twice with virtual-layer views in place, and check that the views still exist and return data afterward. The current unit test mocks fetchone, so it doesn't show how DuckDB's Postgres extension actually behaves. |
| if catalog_type == "ducklake": | ||
| partitioned_by_exps = kwargs.pop("partitioned_by", None) | ||
| elif catalog_type == "postgres" and replace: | ||
| self.execute(exp.Drop(this=table, kind="TABLE", exists=True, cascade=True)) |
There was a problem hiding this comment.
This CASCADE will also drop every SQLMesh virtual-layer view that points at this physical table: prod and any dev environments sharing the snapshot. Those views only get recreated during plan promotion, not during sqlmesh run. So after a scheduled run of a FULL model, prod. would disappear until someone applies a plan that re-promotes this snapshot. It would also drop any non-SQLMesh views on this table in Postgres.
DuckDB doesn't run this in a transaction either (SUPPORTS_TRANSACTIONS = False), so if the CTAS below fails, the table's data is lost.
Could we route Postgres-attached targets to the insert-overwrite path instead (see the summary comment), so the table is never dropped?
| if isinstance(table_name_or_schema, exp.Schema) | ||
| else exp.to_table(table_name_or_schema) | ||
| ) | ||
| catalog = table.catalog or self.get_current_catalog() |
There was a problem hiding this comment.
👍 Resolving the catalog from the target table is the right fix here. Small nit: the base adapter already has get_catalog_type_from_table doing the same catalog resolution. If the fix moves into replace_query, following the Trino adapter's pattern, it may be worth reusing that logic or following its shape.
| table_name.sql(dialect=self.dialect) | ||
| if isinstance(table_name, exp.Table) | ||
| else table_name | ||
| table.sql(dialect=self.dialect) if isinstance(table, exp.Table) else table |
There was a problem hiding this comment.
Nit: table is now always an exp.Table (it comes from exp.to_table or Schema.this), so this isinstance check is dead code. It can just be table.sql(dialect=self.dialect).
| pd.testing.assert_frame_equal(adapter.fetchdf("SELECT * FROM test_table"), df) | ||
|
|
||
|
|
||
| def test_replace_query_attached_postgres( |
There was a problem hiding this comment.
This checks the generated SQL well. Because fetchone is mocked, though, it can't show how the Postgres extension behaves (whether CASCADE is pushed down, what happens to dependent views). Could we add an integration test with the Docker Postgres engine? For example: attach Postgres, plan a FULL model, run it again, then check that the prod virtual-layer view still exists and can be queried.
2 participants
Description
DuckDB does not support
CREATE OR REPLACE TABLEfor tables in an attached Postgres catalog. SQLMesh currently selects replacement behavior using the current DuckDB catalog rather than the target table catalog, so a full refresh of an attached Postgres table attempts unsupported DDL. It also fails when the physical table has dependent SQLMesh virtual-layer views.This change:
DROP TABLE IF EXISTS ... CASCADEfollowed by the normal create pathSQLMesh recreates the virtual-layer views after physical model evaluation, so cascading those dependencies matches the full-refresh lifecycle.
Related context:
This PR addresses physical table replacement in an attached Postgres catalog; it does not claim to close #5438 by itself.
Test Plan
make stylemake fast-testpytest tests/core/engine_adapter/test_duckdb.py -qChecklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO