diff --git a/.github/patches/extensions/sqlsmith/fix.patch b/.github/patches/extensions/sqlsmith/fix.patch index 9cab6e09c4ed..a91f80defae3 100644 --- a/.github/patches/extensions/sqlsmith/fix.patch +++ b/.github/patches/extensions/sqlsmith/fix.patch @@ -26,6 +26,15 @@ index 72e8f5b..8eafec0 100644 } return delete_statement; +@@ -302,7 +303,7 @@ void StatementGenerator::GenerateCTEs(QueryNode &node) { + } + while (RandomPercentage(20)) { + auto cte = make_uniq(); +- cte->query = unique_ptr_cast(GenerateSelect()); ++ cte->query_node = std::move(unique_ptr_cast(GenerateSelect())->node); + for (idx_t i = 0; i < 1 + RandomValue(10); i++) { + cte->aliases.push_back(GenerateIdentifier()); + } @@ -957,9 +958,12 @@ unique_ptr StatementGenerator::GenerateWindowFunction(optional case ExpressionType::WINDOW_NTILE: case ExpressionType::WINDOW_FIRST_VALUE: @@ -80,6 +89,15 @@ index 4602928..3e56aca 100644 #include "duckdb/parser/statement/select_statement.hpp" #endif +@@ -206,7 +209,7 @@ void StatementSimplifier::Simplify(CommonTableExpressionMap &cte) { + SimplifyMap(cte.map); + for (auto &cte_child : cte.map) { + // simplify individual ctes +- Simplify(cte_child.second->query->node); ++ Simplify(cte_child.second->query_node); + } + } + @@ -357,8 +360,6 @@ void StatementSimplifier::SimplifyExpression(duckdb::unique_ptr &info, auto cte_info = make_uniq(); cte_info->aliases = info->aliases; - auto cte_select = make_uniq(); - cte_select->node = std::move(inner_node); - cte_info->query = std::move(cte_select); + cte_info->query_node = std::move(inner_node); outer_select->cte_map.map.insert(info->view_name, std::move(cte_info)); diff --git a/extension/autocomplete/transformer/transform_select.cpp b/extension/autocomplete/transformer/transform_select.cpp index 79d46238ee8b..22b8a668809f 100644 --- a/extension/autocomplete/transformer/transform_select.cpp +++ b/extension/autocomplete/transformer/transform_select.cpp @@ -14,6 +14,12 @@ #include "duckdb/parser/tableref/at_clause.hpp" #include "duckdb/parser/query_node/set_operation_node.hpp" #include "duckdb/parser/tableref/pivotref.hpp" +#include "duckdb/parser/statement/insert_statement.hpp" +#include "duckdb/parser/statement/update_statement.hpp" +#include "duckdb/parser/statement/delete_statement.hpp" +#include "duckdb/parser/query_node/insert_query_node.hpp" +#include "duckdb/parser/query_node/update_query_node.hpp" +#include "duckdb/parser/query_node/delete_query_node.hpp" namespace duckdb { @@ -1597,7 +1603,15 @@ CommonTableExpressionMap PEGTransformerFactory::TransformWithClause(PEGTransform transformer.Transform>>(with_statement_list[entry_idx]); if (is_recursive) { - auto &query_node = with_entry.second->query->node; + auto &query_node = with_entry.second->query_node; + if (!query_node) { + throw ParserException("Recursive CTEs with DML statements are not supported"); + } + if (query_node->type == QueryNodeType::INSERT_QUERY_NODE || + query_node->type == QueryNodeType::UPDATE_QUERY_NODE || + query_node->type == QueryNodeType::DELETE_QUERY_NODE) { + throw ParserException("Recursive CTEs with DML statements are not supported"); + } if (!query_node->modifiers.empty()) { for (auto &modifier : query_node->modifiers) { if (modifier->type == ResultModifierType::LIMIT_MODIFIER || @@ -1645,7 +1659,7 @@ PEGTransformerFactory::TransformWithStatement(PEGTransformer &transformer, optio auto table_ref = transformer.Transform>(list_pr.Child(5)); D_ASSERT(table_ref->type == TableReferenceType::SUBQUERY); auto subquery_ref = unique_ptr_cast(std::move(table_ref)); - result->query = std::move(subquery_ref->subquery); + result->query_node = std::move(subquery_ref->subquery->node); return make_pair(cte_name, std::move(result)); } @@ -1657,10 +1671,28 @@ unique_ptr PEGTransformerFactory::TransformCTEBody(PEGTransformer &tra // CTEBodyContent <- SelectStatementInternal / Statement auto &content_list = inner->Cast(); auto &body_choice = content_list.Child(0); - if (body_choice.result->name != "SelectStatementInternal") { - throw ParserException("A CTE needs a SELECT"); + if (body_choice.result->name == "SelectStatementInternal") { + auto select_statement = transformer.Transform>(body_choice.result); + return make_uniq(std::move(select_statement)); + } + // DML body (INSERT / UPDATE / DELETE) - transform as a Statement and extract its QueryNode + auto sql_stmt = transformer.Transform>(body_choice.result); + unique_ptr query_node; + switch (sql_stmt->type) { + case StatementType::INSERT_STATEMENT: + query_node = unique_ptr_cast(std::move(sql_stmt->Cast().node)); + break; + case StatementType::UPDATE_STATEMENT: + query_node = unique_ptr_cast(std::move(sql_stmt->Cast().node)); + break; + case StatementType::DELETE_STATEMENT: + query_node = unique_ptr_cast(std::move(sql_stmt->Cast().node)); + break; + default: + throw ParserException("A CTE body must be a SELECT, INSERT, UPDATE, or DELETE statement"); } - auto select_statement = transformer.Transform>(body_choice.result); + auto select_statement = make_uniq(); + select_statement->node = std::move(query_node); return make_uniq(std::move(select_statement)); } diff --git a/src/include/duckdb/parser/common_table_expression_info.hpp b/src/include/duckdb/parser/common_table_expression_info.hpp index 8649e7831bcb..0637b984daf8 100644 --- a/src/include/duckdb/parser/common_table_expression_info.hpp +++ b/src/include/duckdb/parser/common_table_expression_info.hpp @@ -13,25 +13,32 @@ namespace duckdb { +class QueryNode; class SelectStatement; +class Serializer; +class Deserializer; struct CommonTableExpressionInfo { + CommonTableExpressionInfo() = default; ~CommonTableExpressionInfo(); + //! Used by deserialization: prefers query_node; if null, uses query->node. + CommonTableExpressionInfo(unique_ptr query, unique_ptr query_node); + vector aliases; vector> key_targets; vector> payload_aggregates; - unique_ptr query; + //! The root QueryNode for this CTE (SELECT, INSERT, UPDATE, or DELETE) + unique_ptr query_node; CTEMaterialize materialized = CTEMaterialize::CTE_MATERIALIZE_DEFAULT; -public: + CTEMaterialize GetMaterializedForSerialization(Serializer &serializer) const; + unique_ptr GetQueryForSerialization(Serializer &serializer) const; + void Serialize(Serializer &serializer) const; static unique_ptr Deserialize(Deserializer &deserializer); unique_ptr Copy(); - -private: - CTEMaterialize GetMaterializedForSerialization(Serializer &serializer) const; }; } // namespace duckdb diff --git a/src/include/duckdb/parser/transformer.hpp b/src/include/duckdb/parser/transformer.hpp index 4607555666c9..b10dd43d1cb6 100644 --- a/src/include/duckdb/parser/transformer.hpp +++ b/src/include/duckdb/parser/transformer.hpp @@ -302,8 +302,8 @@ class Transformer { string TransformAlias(duckdb_libpgquery::PGAlias *root, vector &column_name_alias); vector TransformStringList(duckdb_libpgquery::PGList *list); void TransformCTE(duckdb_libpgquery::PGWithClause &de_with_clause, CommonTableExpressionMap &cte_map); - unique_ptr TransformRecursiveCTE(duckdb_libpgquery::PGCommonTableExpr &node, - CommonTableExpressionInfo &info); + unique_ptr TransformRecursiveCTE(duckdb_libpgquery::PGCommonTableExpr &node, + CommonTableExpressionInfo &info); unique_ptr TransformUnaryOperator(const string &op, unique_ptr child); unique_ptr TransformBinaryOperator(string op, unique_ptr left, diff --git a/src/include/duckdb/storage/serialization/nodes.json b/src/include/duckdb/storage/serialization/nodes.json index ebd5098264d2..de5282702a4d 100644 --- a/src/include/duckdb/storage/serialization/nodes.json +++ b/src/include/duckdb/storage/serialization/nodes.json @@ -13,7 +13,10 @@ { "id": 101, "name": "query", - "type": "SelectStatement*" + "type": "SelectStatement*", + "version": "v2.0.0", + "status": "read_only", + "serialize_property": "GetQueryForSerialization(serializer)" }, { "id": 102, @@ -30,8 +33,15 @@ "id": 104, "name": "payload_aggregates", "type": "vector" + }, + { + "id": 105, + "name": "query_node", + "type": "QueryNode*", + "version": "v2.0.0" } - ] + ], + "constructor": ["query", "query_node"] }, { "class": "CommonTableExpressionMap", diff --git a/src/main/appender.cpp b/src/main/appender.cpp index 7f6699283298..03103baefa84 100644 --- a/src/main/appender.cpp +++ b/src/main/appender.cpp @@ -483,7 +483,7 @@ unique_ptr BaseAppender::ParseStatement(unique_ptr table // Create the CTE info. auto cte_info = make_uniq(); - cte_info->query = std::move(cte_select); + cte_info->query_node = std::move(cte_select->node); cte_info->materialized = CTEMaterialize::CTE_MATERIALIZE_NEVER; // Add the appender data as a CTE to the CTE map of the statement. diff --git a/src/main/relation/query_relation.cpp b/src/main/relation/query_relation.cpp index 79aa1f9814d9..c991aeae245f 100644 --- a/src/main/relation/query_relation.cpp +++ b/src/main/relation/query_relation.cpp @@ -82,7 +82,7 @@ BoundStatement QueryRelation::Bind(Binder &binder) { select->node = std::move(select_node); auto cte_info = make_uniq(); - cte_info->query = std::move(select); + cte_info->query_node = std::move(select->node); auto subquery = make_uniq(std::move(select_stmt), "query_relation"); auto top_level_select = make_uniq(); diff --git a/src/optimizer/cte_inlining.cpp b/src/optimizer/cte_inlining.cpp index b225929bc159..ae53d5f64e93 100644 --- a/src/optimizer/cte_inlining.cpp +++ b/src/optimizer/cte_inlining.cpp @@ -87,6 +87,23 @@ bool CTEInlining::EndsInAggregateOrDistinct(const LogicalOperator &op) { return false; } +static bool HasSideEffects(const LogicalOperator &op) { + switch (op.type) { + case LogicalOperatorType::LOGICAL_INSERT: + case LogicalOperatorType::LOGICAL_UPDATE: + case LogicalOperatorType::LOGICAL_DELETE: + return true; + default: + break; + } + for (auto &child : op.children) { + if (HasSideEffects(*child)) { + return true; + } + } + return false; +} + void CTEInlining::TryInlining(unique_ptr &op) { if (op->type == LogicalOperatorType::LOGICAL_PREPARE) { // we are in a prepare statement, if we have to copy an operator during inlining, @@ -104,6 +121,10 @@ void CTEInlining::TryInlining(unique_ptr &op) { auto &cte = op->Cast(); auto ref_count = CountCTEReferences(*op, cte.table_index); if (ref_count == 0) { + if (HasSideEffects(*cte.children[0])) { + // DML CTEs must always execute for side effects even when unreferenced + return; + } // this CTE is not referenced, we can remove it op = std::move(op->children[1]); return; diff --git a/src/parser/parsed_expression_iterator.cpp b/src/parser/parsed_expression_iterator.cpp index e01fe65f7bdc..c994cadbb749 100644 --- a/src/parser/parsed_expression_iterator.cpp +++ b/src/parser/parsed_expression_iterator.cpp @@ -372,7 +372,9 @@ void ParsedExpressionIterator::EnumerateQueryNodeChildren( } for (auto &kv : node.cte_map.map) { - EnumerateQueryNodeChildren(*kv.second->query->node, expr_callback, ref_callback); + if (kv.second->query_node) { + EnumerateQueryNodeChildren(*kv.second->query_node, expr_callback, ref_callback); + } } } diff --git a/src/parser/query_node.cpp b/src/parser/query_node.cpp index b6691df0ccbd..de6494ecf417 100644 --- a/src/parser/query_node.cpp +++ b/src/parser/query_node.cpp @@ -5,10 +5,6 @@ #include "duckdb/parser/query_node/recursive_cte_node.hpp" #include "duckdb/parser/query_node/cte_node.hpp" #include "duckdb/common/limits.hpp" -#include "duckdb/common/serializer/serializer.hpp" -#include "duckdb/common/serializer/deserializer.hpp" -#include "duckdb/parser/statement/select_statement.hpp" - namespace duckdb { CommonTableExpressionMap::CommonTableExpressionMap() { @@ -27,7 +23,9 @@ CommonTableExpressionMap CommonTableExpressionMap::Copy() const { for (auto &al : kv.second->payload_aggregates) { kv_info->payload_aggregates.push_back(al->Copy()); } - kv_info->query = unique_ptr_cast(kv.second->query->Copy()); + if (kv.second->query_node) { + kv_info->query_node = kv.second->query_node->Copy(); + } kv_info->materialized = kv.second->materialized; res.map[kv.first] = std::move(kv_info); } @@ -42,7 +40,7 @@ string CommonTableExpressionMap::ToString() const { // check if there are any recursive CTEs bool has_recursive = false; for (auto &kv : map) { - if (kv.second->query->node->type == QueryNodeType::RECURSIVE_CTE_NODE) { + if (kv.second->query_node && kv.second->query_node->type == QueryNodeType::RECURSIVE_CTE_NODE) { has_recursive = true; break; } @@ -89,7 +87,8 @@ string CommonTableExpressionMap::ToString() const { } else { result += " AS ("; } - result += cte.query->ToString(); + D_ASSERT(cte.query_node); + result += cte.query_node->ToString(); result += ")"; first_cte = false; } @@ -169,7 +168,8 @@ bool QueryNode::Equals(const QueryNode *other) const { if (!ParsedExpression::ListEquals(entry.second->payload_aggregates, other_entry->second->payload_aggregates)) { return false; } - if (!entry.second->query->Equals(*other->cte_map.map.at(entry.first)->query)) { + if (!entry.second->query_node || + !entry.second->query_node->Equals(other->cte_map.map.at(entry.first)->query_node.get())) { return false; } } @@ -191,7 +191,9 @@ void QueryNode::CopyProperties(QueryNode &other) const { for (auto &agg : kv.second->payload_aggregates) { kv_info->payload_aggregates.push_back(agg->Copy()); } - kv_info->query = unique_ptr_cast(kv.second->query->Copy()); + if (kv.second->query_node) { + kv_info->query_node = kv.second->query_node->Copy(); + } kv_info->materialized = kv.second->materialized; other.cte_map.map[kv.first] = std::move(kv_info); } diff --git a/src/parser/transform/helpers/transform_cte.cpp b/src/parser/transform/helpers/transform_cte.cpp index 6ff7b60853aa..15a05dbeb2dc 100644 --- a/src/parser/transform/helpers/transform_cte.cpp +++ b/src/parser/transform/helpers/transform_cte.cpp @@ -1,9 +1,16 @@ #include "duckdb/common/enums/set_operation_type.hpp" #include "duckdb/common/exception.hpp" #include "duckdb/common/exception/parser_exception.hpp" +#include "duckdb/common/serializer/serializer.hpp" #include "duckdb/parser/query_node/cte_node.hpp" #include "duckdb/parser/query_node/recursive_cte_node.hpp" +#include "duckdb/parser/query_node/insert_query_node.hpp" +#include "duckdb/parser/query_node/update_query_node.hpp" +#include "duckdb/parser/query_node/delete_query_node.hpp" #include "duckdb/parser/statement/select_statement.hpp" +#include "duckdb/parser/statement/insert_statement.hpp" +#include "duckdb/parser/statement/update_statement.hpp" +#include "duckdb/parser/statement/delete_statement.hpp" #include "duckdb/parser/transformer.hpp" namespace duckdb { @@ -11,16 +18,15 @@ namespace duckdb { unique_ptr CommonTableExpressionInfo::Copy() { auto result = make_uniq(); result->aliases = aliases; - result->query = unique_ptr_cast(query->Copy()); - - for (auto &key : result->key_targets) { + if (query_node) { + result->query_node = query_node->Copy(); + } + for (auto &key : key_targets) { result->key_targets.push_back(key->Copy()); } - - for (auto &agg : result->payload_aggregates) { + for (auto &agg : payload_aggregates) { result->payload_aggregates.push_back(agg->Copy()); } - result->materialized = materialized; return result; } @@ -35,6 +41,30 @@ CTEMaterialize CommonTableExpressionInfo::GetMaterializedForSerialization(Serial return CTEMaterialize::CTE_MATERIALIZE_DEFAULT; } +unique_ptr CommonTableExpressionInfo::GetQueryForSerialization(Serializer &serializer) const { + // Field 101 is only written for old storage versions (v7 and earlier). + // For v8+ this method is not called; the QueryNode is written directly as field 106. + if (!query_node || + (query_node->type != QueryNodeType::SELECT_NODE && query_node->type != QueryNodeType::SET_OPERATION_NODE && + query_node->type != QueryNodeType::RECURSIVE_CTE_NODE)) { + throw SerializationException( + "DML CTEs (INSERT/UPDATE/DELETE) require storage version v2.0.0 or higher and cannot be " + "serialized to older storage formats"); + } + auto select = make_uniq(); + select->node = query_node->Copy(); + return select; +} + +CommonTableExpressionInfo::CommonTableExpressionInfo(unique_ptr query, + unique_ptr query_node) { + if (query_node) { + this->query_node = std::move(query_node); + } else if (query) { + this->query_node = std::move(query->node); + } +} + void Transformer::ExtractCTEsRecursive(CommonTableExpressionMap &cte_map) { for (auto &cte_entry : stored_cte_map) { for (auto &entry : cte_entry.get().map) { @@ -88,19 +118,42 @@ void Transformer::TransformCTE(duckdb_libpgquery::PGWithClause &de_with_clause, throw NotImplementedException("CTE collations not supported"); } // we need a query - if (!cte.ctequery || cte.ctequery->type != duckdb_libpgquery::T_PGSelectStmt) { - throw ParserException("A CTE needs a SELECT"); + if (!cte.ctequery) { + throw ParserException("A CTE body must be a SELECT, INSERT, UPDATE, or DELETE statement"); } + bool is_recursive = cte.cterecursive || de_with_clause.recursive; + // CTE transformation can either result in inlining for non recursive CTEs, or in recursive CTE bindings // otherwise. - if (cte.cterecursive || de_with_clause.recursive) { - info->query = TransformRecursiveCTE(cte, *info); + if (cte.ctequery->type == duckdb_libpgquery::T_PGSelectStmt) { + if (is_recursive) { + info->query_node = TransformRecursiveCTE(cte, *info); + } else { + Transformer cte_transformer(*this); + auto select = cte_transformer.TransformSelectStmt(*cte.ctequery); + info->query_node = std::move(select->node); + } } else { + // DML body (INSERT / UPDATE / DELETE) + if (is_recursive) { + throw ParserException("Recursive CTEs with DML statements are not supported"); + } Transformer cte_transformer(*this); - info->query = cte_transformer.TransformSelectStmt(*cte.ctequery); + if (cte.ctequery->type == duckdb_libpgquery::T_PGInsertStmt) { + auto stmt = cte_transformer.TransformInsert(PGCast(*cte.ctequery)); + info->query_node = unique_ptr_cast(std::move(stmt->node)); + } else if (cte.ctequery->type == duckdb_libpgquery::T_PGUpdateStmt) { + auto stmt = cte_transformer.TransformUpdate(PGCast(*cte.ctequery)); + info->query_node = unique_ptr_cast(std::move(stmt->node)); + } else if (cte.ctequery->type == duckdb_libpgquery::T_PGDeleteStmt) { + auto stmt = cte_transformer.TransformDelete(PGCast(*cte.ctequery)); + info->query_node = unique_ptr_cast(std::move(stmt->node)); + } else { + throw ParserException("A CTE body must be a SELECT, INSERT, UPDATE, or DELETE statement"); + } } - D_ASSERT(info->query); + D_ASSERT(info->query_node); auto cte_name = string(cte.ctename); auto it = cte_map.map.find(cte_name); @@ -125,16 +178,14 @@ void Transformer::TransformCTE(duckdb_libpgquery::PGWithClause &de_with_clause, } } -unique_ptr Transformer::TransformRecursiveCTE(duckdb_libpgquery::PGCommonTableExpr &cte, - CommonTableExpressionInfo &info) { +unique_ptr Transformer::TransformRecursiveCTE(duckdb_libpgquery::PGCommonTableExpr &cte, + CommonTableExpressionInfo &info) { auto &stmt = *PGPointerCast(cte.ctequery); - unique_ptr select; switch (stmt.op) { case duckdb_libpgquery::PG_SETOP_UNION: { - select = make_uniq(); - select->node = make_uniq_base(); - auto &result = select->node->Cast(); + auto node = make_uniq_base(); + auto &result = node->Cast(); result.ctename = string(cte.ctename); result.union_all = stmt.all; if (stmt.withClause) { @@ -147,26 +198,18 @@ unique_ptr Transformer::TransformRecursiveCTE(duckdb_libpgquery for (auto &key : info.key_targets) { result.key_targets.emplace_back(key->Copy()); } - break; + if (stmt.limitCount || stmt.limitOffset) { + throw ParserException("LIMIT or OFFSET in a recursive query is not allowed"); + } + if (stmt.sortClause) { + throw ParserException("ORDER BY in a recursive query is not allowed"); + } + return node; } - case duckdb_libpgquery::PG_SETOP_EXCEPT: - case duckdb_libpgquery::PG_SETOP_INTERSECT: - default: { + default: // This CTE is not recursive. Fallback to regular query transformation. - auto node = TransformSelectNode(*cte.ctequery); - auto result = make_uniq(); - result->node = std::move(node); - return result; + return TransformSelectNode(*cte.ctequery); } - } - - if (stmt.limitCount || stmt.limitOffset) { - throw ParserException("LIMIT or OFFSET in a recursive query is not allowed"); - } - if (stmt.sortClause) { - throw ParserException("ORDER BY in a recursive query is not allowed"); - } - return select; } } // namespace duckdb diff --git a/src/planner/binder/query_node/bind_cte_node.cpp b/src/planner/binder/query_node/bind_cte_node.cpp index 22c9338e64e4..803862d0aac5 100644 --- a/src/planner/binder/query_node/bind_cte_node.cpp +++ b/src/planner/binder/query_node/bind_cte_node.cpp @@ -18,10 +18,28 @@ struct BoundCTEData { shared_ptr cte_bind_state; }; +static bool IsDMLQueryNode(const CommonTableExpressionInfo &cte) { + if (!cte.query_node) { + return false; + } + auto t = cte.query_node->type; + return t == QueryNodeType::INSERT_QUERY_NODE || t == QueryNodeType::UPDATE_QUERY_NODE || + t == QueryNodeType::DELETE_QUERY_NODE; +} + BoundStatement Binder::BindNode(QueryNode &node) { reference current_binder(*this); vector bound_ctes; + idx_t dml_cte_count = 0; for (auto &cte : node.cte_map.map) { + if (IsDMLQueryNode(*cte.second)) { + if (parent) { + throw BinderException("WITH clause containing a data-modifying statement must be at the top level"); + } + if (++dml_cte_count > 1) { + throw BinderException("Only a single DML statement (INSERT/UPDATE/DELETE) is allowed per WITH clause"); + } + } bound_ctes.push_back(current_binder.get().PrepareCTE(cte.first, *cte.second)); current_binder = *bound_ctes.back().child_binder; } @@ -117,7 +135,7 @@ BoundCTEData Binder::PrepareCTE(const string &ctename, CommonTableExpressionInfo // first recursively visit the materialized CTE operations // the left side is visited first and is added to the BindContext of the right side - D_ASSERT(statement.query); + D_ASSERT(statement.query_node); result.ctename = ctename; result.materialized = statement.materialized; @@ -125,7 +143,7 @@ BoundCTEData Binder::PrepareCTE(const string &ctename, CommonTableExpressionInfo // instead of eagerly binding the CTE here we add the CTE bind state to the list of CTE bindings // the CTE is bound lazily - when referenced for the first time we perform the binding - result.cte_bind_state = make_shared_ptr(*this, *statement.query->node, statement.aliases); + result.cte_bind_state = make_shared_ptr(*this, *statement.query_node, statement.aliases); result.child_binder = Binder::CreateBinder(context, this); @@ -139,9 +157,19 @@ BoundCTEData Binder::PrepareCTE(const string &ctename, CommonTableExpressionInfo BoundStatement Binder::FinishCTE(BoundCTEData &bound_cte, BoundStatement child) { if (!bound_cte.cte_bind_state->IsBound()) { - // CTE was not bound - just ignore it - MoveCorrelatedExpressions(*bound_cte.child_binder); - return child; + auto node_type = bound_cte.cte_bind_state->cte_def.type; + bool is_dml = node_type == QueryNodeType::INSERT_QUERY_NODE || node_type == QueryNodeType::UPDATE_QUERY_NODE || + node_type == QueryNodeType::DELETE_QUERY_NODE; + if (is_dml) { + // DML CTEs always execute even if not referenced - force bind now + auto dummy_binding = + make_uniq(BindingAlias(bound_cte.ctename), bound_cte.cte_bind_state, bound_cte.setop_index); + bound_cte.cte_bind_state->Bind(*dummy_binding); + } else { + // Non-DML CTE was not referenced - just ignore it + MoveCorrelatedExpressions(*bound_cte.child_binder); + return child; + } } auto &bind_state = *bound_cte.cte_bind_state; for (auto &c : bind_state.query_binder->correlated_columns) { diff --git a/src/planner/binder/statement/bind_create.cpp b/src/planner/binder/statement/bind_create.cpp index 06f66d06d85f..aee066c1276e 100644 --- a/src/planner/binder/statement/bind_create.cpp +++ b/src/planner/binder/statement/bind_create.cpp @@ -195,6 +195,19 @@ void Binder::BindCreateViewInfo(CreateViewInfo &base) { if (base.binding_mode == CreateViewBindingMode::SKIP_BINDING) { return; } + // DML statements (INSERT/UPDATE/DELETE) are not allowed as CTE bodies inside a view, + // because the DML would execute every time the view is queried. + for (auto &kv : base.query->node->cte_map.map) { + auto &cte = *kv.second; + if (!cte.query_node) { + continue; + } + auto t = cte.query_node->type; + if (t == QueryNodeType::INSERT_QUERY_NODE || t == QueryNodeType::UPDATE_QUERY_NODE || + t == QueryNodeType::DELETE_QUERY_NODE) { + throw BinderException("DML statements (INSERT/UPDATE/DELETE) are not allowed as CTE bodies inside a VIEW"); + } + } optional_ptr dependencies; if (Settings::Get(context)) { dependencies = base.dependencies; diff --git a/src/storage/serialization/serialize_nodes.cpp b/src/storage/serialization/serialize_nodes.cpp index e8f45fa86192..62f8001a43f1 100644 --- a/src/storage/serialization/serialize_nodes.cpp +++ b/src/storage/serialization/serialize_nodes.cpp @@ -259,19 +259,29 @@ ColumnList ColumnList::Deserialize(Deserializer &deserializer) { void CommonTableExpressionInfo::Serialize(Serializer &serializer) const { serializer.WritePropertyWithDefault>(100, "aliases", aliases); - serializer.WritePropertyWithDefault>(101, "query", query); + if (!serializer.ShouldSerialize(8)) { + serializer.WritePropertyWithDefault>(101, "query", GetQueryForSerialization(serializer)); + } serializer.WriteProperty(102, "materialized", GetMaterializedForSerialization(serializer)); serializer.WritePropertyWithDefault>>(103, "key_targets", key_targets); serializer.WritePropertyWithDefault>>(104, "payload_aggregates", payload_aggregates); + if (serializer.ShouldSerialize(8)) { + serializer.WritePropertyWithDefault>(105, "query_node", query_node); + } } unique_ptr CommonTableExpressionInfo::Deserialize(Deserializer &deserializer) { - auto result = duckdb::unique_ptr(new CommonTableExpressionInfo()); - deserializer.ReadPropertyWithDefault>(100, "aliases", result->aliases); - deserializer.ReadPropertyWithDefault>(101, "query", result->query); - deserializer.ReadProperty(102, "materialized", result->materialized); - deserializer.ReadPropertyWithDefault>>(103, "key_targets", result->key_targets); - deserializer.ReadPropertyWithDefault>>(104, "payload_aggregates", result->payload_aggregates); + auto aliases = deserializer.ReadPropertyWithDefault>(100, "aliases"); + auto query = deserializer.ReadPropertyWithDefault>(101, "query"); + auto materialized = deserializer.ReadProperty(102, "materialized"); + auto key_targets = deserializer.ReadPropertyWithDefault>>(103, "key_targets"); + auto payload_aggregates = deserializer.ReadPropertyWithDefault>>(104, "payload_aggregates"); + auto query_node = deserializer.ReadPropertyWithDefault>(105, "query_node"); + auto result = duckdb::unique_ptr(new CommonTableExpressionInfo(std::move(query), std::move(query_node))); + result->aliases = std::move(aliases); + result->materialized = materialized; + result->key_targets = std::move(key_targets); + result->payload_aggregates = std::move(payload_aggregates); return result; } diff --git a/test/configs/enable_verification.json b/test/configs/enable_verification.json index 872cf8cd7276..438f4a132b3f 100755 --- a/test/configs/enable_verification.json +++ b/test/configs/enable_verification.json @@ -250,6 +250,12 @@ "paths": [ "test/geoparquet/with_crs.test" ] + }, + { + "reason": "DML CTE re-execution under verification causes incorrect row counts", + "paths": [ + "test/sql/cte/dml_cte.test" + ] } ] } diff --git a/test/sql/cte/dml_cte.test b/test/sql/cte/dml_cte.test new file mode 100644 index 000000000000..a207c0ca3f3e --- /dev/null +++ b/test/sql/cte/dml_cte.test @@ -0,0 +1,487 @@ +# name: test/sql/cte/dml_cte.test +# description: Test DML statements (INSERT/UPDATE/DELETE) as CTE bodies, including RETURNING, unreferenced CTEs, side-effect semantics, snapshot isolation, nested WITH, transactions, prepared statements, and rejection of DML CTEs inside views +# group: [cte] + +statement ok +CREATE TABLE t (i INTEGER, j INTEGER); + +# Basic INSERT CTE with RETURNING +query I rowsort +WITH ins AS ( + INSERT INTO t VALUES (1, 10), (2, 20) RETURNING i +) +SELECT * FROM ins; +---- +1 +2 + +query II rowsort +SELECT * FROM t; +---- +1 10 +2 20 + +# DELETE CTE with RETURNING +query I +WITH del AS ( + DELETE FROM t WHERE i = 1 RETURNING i +) +SELECT * FROM del; +---- +1 + +query II rowsort +SELECT * FROM t; +---- +2 20 + +# UPDATE CTE with RETURNING +query II rowsort +WITH upd AS ( + UPDATE t SET j = 99 WHERE i = 2 RETURNING i, j +) +SELECT * FROM upd; +---- +2 99 + +query II rowsort +SELECT * FROM t; +---- +2 99 + +# Unreferenced INSERT CTE - side effect executes even though ins is not referenced by the outer SELECT +statement ok +TRUNCATE t; + +statement ok +INSERT INTO t VALUES (10, 100); + +query II rowsort +WITH ins AS ( + INSERT INTO t VALUES (20, 200) RETURNING i, j +) +SELECT t.i, t.j FROM t; +---- +10 100 +20 200 + +# Multiple CTEs where one is DML and one is SELECT +# The DELETE side effect always executes; verify via post-query state +statement ok +TRUNCATE t; + +statement ok +INSERT INTO t VALUES (1, 1), (2, 2), (3, 3); + +statement ok +WITH + del AS (DELETE FROM t WHERE i = 1 RETURNING i), + remaining AS (SELECT i FROM t) +SELECT * FROM remaining; + +query II rowsort +SELECT * FROM t; +---- +2 2 +3 3 + +# DML CTE that is not referenced - side effect still executes (RETURNING used by outer) +statement ok +TRUNCATE t; + +query I +WITH ins AS ( + INSERT INTO t VALUES (42, 42) RETURNING i +) +SELECT * FROM ins; +---- +42 + +query II rowsort +SELECT * FROM t; +---- +42 42 + +# Unreferenced DML CTE without RETURNING - side effect must still execute +# This exercises the ref_count == 0 path in the CTE inlining optimizer +statement ok +TRUNCATE t; + +statement ok +WITH ins AS (INSERT INTO t VALUES (99, 99)) +SELECT 1; + +query II +SELECT * FROM t WHERE i = 99; +---- +99 99 + +# Unreferenced DELETE CTE without RETURNING - side effect must still execute +statement ok +INSERT INTO t VALUES (77, 77); + +statement ok +WITH del AS (DELETE FROM t WHERE i = 77) +SELECT 1; + +query I +SELECT count(*) FROM t WHERE i = 77; +---- +0 + +# Referenced DML CTE without RETURNING - exposes 0 columns but 1 row per affected row +# The outer query can join against it but cannot project any of its columns. +statement ok +TRUNCATE t; + +query I +WITH ins AS (INSERT INTO t VALUES (55, 55)) +SELECT 1 AS x FROM ins; +---- +1 + +query II +SELECT * FROM t WHERE i = 55; +---- +55 55 + +# DML CTE inside an UPDATE statement +statement ok +TRUNCATE t; + +statement ok +INSERT INTO t VALUES (1, 1), (2, 2), (3, 3); + +statement ok +CREATE TABLE multiplier (k INTEGER); + +statement ok +INSERT INTO multiplier VALUES (10); + +query II rowsort +WITH m AS (INSERT INTO multiplier VALUES (20) RETURNING k) +UPDATE t SET j = t.i * m.k FROM m WHERE t.i <= 2 +RETURNING i, j; +---- +1 20 +2 40 + +query I rowsort +SELECT * FROM multiplier; +---- +10 +20 + +# DML CTE inside a DELETE statement +statement ok +TRUNCATE t; + +statement ok +INSERT INTO t VALUES (1, 10), (2, 20), (3, 30); + +statement ok +CREATE TABLE to_delete (v INTEGER); + +statement ok +INSERT INTO to_delete VALUES (2); + +query I rowsort +WITH extra AS (INSERT INTO to_delete VALUES (3) RETURNING v) +DELETE FROM t WHERE i IN (SELECT v FROM extra) +RETURNING i; +---- +3 + +query II rowsort +SELECT * FROM t; +---- +1 10 +2 20 + +# Prepared statement with a parameterized DML CTE +statement ok +TRUNCATE t; + +statement ok +PREPARE dml_prepared AS + WITH ins AS (INSERT INTO t VALUES ($1, $2) RETURNING i) + SELECT * FROM ins; + +query I +EXECUTE dml_prepared(7, 77); +---- +7 + +query II +SELECT * FROM t WHERE i = 7; +---- +7 77 + +# INSERT CTE from a subquery +statement ok +CREATE TABLE src (v INTEGER); + +statement ok +INSERT INTO src VALUES (100), (200); + +statement ok +TRUNCATE t; + +query I rowsort +WITH ins AS ( + INSERT INTO t SELECT v, v * 2 FROM src RETURNING i +) +SELECT * FROM ins; +---- +100 +200 + +query II rowsort +SELECT * FROM t; +---- +100 200 +200 400 + +# Error cases: statements not allowed as CTE bodies + +# DDL (CREATE TABLE, DROP TABLE, ALTER TABLE) is rejected by the parser grammar or transformer +statement error +WITH cte AS (CREATE TABLE forbidden (i INT)) SELECT 1; +---- +:.*(syntax error|A CTE body must be a SELECT, INSERT, UPDATE, or DELETE statement).* + +statement error +WITH cte AS (DROP TABLE forbidden) SELECT 1; +---- +:.*(syntax error|A CTE body must be a SELECT, INSERT, UPDATE, or DELETE statement).* + +statement error +WITH cte AS (ALTER TABLE t ADD COLUMN x INT) SELECT 1; +---- +:.*(syntax error|A CTE body must be a SELECT, INSERT, UPDATE, or DELETE statement).* + +# COPY is parsed by the grammar but rejected as an invalid CTE body type +statement error +WITH cte AS (COPY t TO 'x.csv') SELECT 1; +---- +:.*A CTE body must be a SELECT, INSERT, UPDATE, or DELETE statement.* + +# Recursive CTEs with DML bodies are not supported +statement error +WITH RECURSIVE ins AS (INSERT INTO t VALUES (1, 2) RETURNING i) SELECT * FROM ins; +---- +:.*Recursive CTEs with DML statements are not supported.* + +statement error +WITH RECURSIVE upd AS (UPDATE t SET j = 0 RETURNING i) SELECT * FROM upd; +---- +:.*Recursive CTEs with DML statements are not supported.* + +statement error +WITH RECURSIVE del AS (DELETE FROM t RETURNING i) SELECT * FROM del; +---- +:.*Recursive CTEs with DML statements are not supported.* + +# Multiple DML CTEs in one WITH clause are not supported +statement error +WITH ins AS (INSERT INTO t VALUES (1, 2)), del AS (DELETE FROM t WHERE i = 1) SELECT 1; +---- +:.*Only a single DML statement.*is allowed per WITH clause.* + +statement error +WITH ins AS (INSERT INTO t VALUES (1, 2) RETURNING i), upd AS (UPDATE t SET j = 0 RETURNING i) SELECT * FROM ins; +---- +:.*Only a single DML statement.*is allowed per WITH clause.* + +statement error +WITH upd AS (UPDATE t SET j = 0 RETURNING i), del AS (DELETE FROM t WHERE i = 1 RETURNING i) SELECT * FROM upd; +---- +:.*Only a single DML statement.*is allowed per WITH clause.* + +# DML CTEs must be at the top level (not inside subqueries) + +statement error +SELECT (WITH ins AS (INSERT INTO t VALUES (1, 2) RETURNING i) SELECT * FROM ins) AS x; +---- +:.*data-modifying statement must be at the top level.* + +statement error +SELECT * FROM (WITH ins AS (INSERT INTO t VALUES (1, 2) RETURNING i) SELECT * FROM ins) AS sub; +---- +:.*data-modifying statement must be at the top level.* + +statement error +SELECT * FROM t WHERE i IN (WITH del AS (DELETE FROM t WHERE i = 1 RETURNING i) SELECT i FROM del); +---- +:.*data-modifying statement must be at the top level.* + +# Self-referential INSERT + SELECT from the same table. +# The INSERT CTE always executes (side effect), so after the query t2 has rows. +# Whether sel sees the inserted rows depends on execution order, which is not +# guaranteed — so we only verify the post-query state of t2. +statement ok +CREATE TABLE t2 (i INTEGER, j INTEGER); + +statement ok +WITH ins AS MATERIALIZED ( + INSERT INTO t2 VALUES (1, 10), (2, 20) RETURNING i, j +), sel AS MATERIALIZED (SELECT * FROM t2) +SELECT ins.i, sel.i FROM ins, sel; + +# After the query, t2 has the inserted rows +query II rowsort +SELECT * FROM t2; +---- +1 10 +2 20 + +# DML CTE with a view +statement ok +CREATE TABLE base (i INTEGER); + +statement ok +INSERT INTO base VALUES (10), (20), (30); + +statement ok +CREATE VIEW base_view AS SELECT * FROM base WHERE i >= 20; + +# INSERT via a view-filtered subquery in CTE +query I rowsort +WITH ins AS ( + INSERT INTO base SELECT i + 1 FROM base_view RETURNING i +) +SELECT * FROM ins; +---- +21 +31 + +query I rowsort +SELECT * FROM base; +---- +10 +20 +21 +30 +31 + +# UPDATE via CTE referencing a view +query II rowsort +WITH upd AS ( + UPDATE base SET i = i * 2 WHERE i IN (SELECT i FROM base_view) RETURNING i, i / 2 +) +SELECT * FROM upd; +---- +40 20 +42 21 +60 30 +62 31 + +# DELETE via CTE using a view +statement ok +TRUNCATE base; + +statement ok +INSERT INTO base VALUES (1), (2), (3), (4), (5); + +statement ok +CREATE VIEW small_base AS SELECT * FROM base WHERE i <= 3; + +query I rowsort +WITH del AS ( + DELETE FROM base WHERE i IN (SELECT i FROM small_base) RETURNING i +) +SELECT * FROM del; +---- +1 +2 +3 + +query I rowsort +SELECT * FROM base; +---- +4 +5 + +# Nested DML CTEs inside subqueries are rejected (Postgres-compatible behavior): +# WITH clause containing a data-modifying statement must be at the top level. +statement ok +TRUNCATE t; + +statement ok +INSERT INTO t VALUES (1, 1), (2, 2), (3, 3); + +statement error +WITH outer_del AS ( + DELETE FROM t WHERE i = 1 RETURNING i +) +SELECT * FROM ( + WITH inner_ins AS (INSERT INTO t VALUES (99, 99) RETURNING i) + SELECT * FROM inner_ins +) sub; +---- +:.*data-modifying statement must be at the top level.* + +# Transaction rollback: DML CTE side effects roll back with the transaction +statement ok +TRUNCATE t; + +statement ok +INSERT INTO t VALUES (1, 1); + +statement ok +BEGIN; + +statement ok +WITH ins AS (INSERT INTO t VALUES (42, 42) RETURNING i) +SELECT * FROM ins; + +statement ok +ROLLBACK; + +query II +SELECT * FROM t; +---- +1 1 + +# DML CTEs are not allowed as CTE bodies inside a VIEW +statement error +CREATE VIEW v_bad AS WITH ins AS (INSERT INTO t VALUES (1, 2) RETURNING i) SELECT * FROM ins; +---- +:.*DML statements.*are not allowed.*VIEW.* + +statement error +CREATE VIEW v_bad AS WITH upd AS (UPDATE t SET j = 0 RETURNING i) SELECT * FROM upd; +---- +:.*DML statements.*are not allowed.*VIEW.* + +statement error +CREATE VIEW v_bad AS WITH del AS (DELETE FROM t RETURNING i) SELECT * FROM del; +---- +:.*DML statements.*are not allowed.*VIEW.* + +# Serialization: verify that views containing SELECT CTEs survive a DB restart +# (exercises the v8 query_node field serialization path) +load __TEST_DIR__/dml_cte_storage.db + +statement ok +CREATE TABLE persist_t (i INTEGER); + +statement ok +INSERT INTO persist_t VALUES (1), (2), (3); + +statement ok +CREATE VIEW persist_v AS WITH cte AS (SELECT i * 10 AS j FROM persist_t) SELECT j FROM cte WHERE j > 10; + +query I rowsort +FROM persist_v; +---- +20 +30 + +restart + +query I rowsort +FROM persist_v; +---- +20 +30 diff --git a/test/sql/cte/insert_cte_bug_3417.test b/test/sql/cte/insert_cte_bug_3417.test index 1b74377821bf..152181f9362b 100644 --- a/test/sql/cte/insert_cte_bug_3417.test +++ b/test/sql/cte/insert_cte_bug_3417.test @@ -1,5 +1,5 @@ # name: test/sql/cte/insert_cte_bug_3417.test -# description: Test for a crash reported in issue #3417 +# description: Test for a crash reported in issue #3417 - INSERT with DML CTE now supported # group: [cte] statement ok @@ -11,7 +11,23 @@ CREATE TABLE table1 (id INTEGER, a INTEGER); statement ok CREATE TABLE table2 (table1_id INTEGER); +# DML CTEs must use the WITH ... INSERT INTO syntax (WITH before the INSERT keyword). +# The INSERT INTO t WITH ... SELECT form is not supported (matches Postgres). statement error INSERT INTO table2 WITH cte AS (INSERT INTO table1 SELECT 1, 2 RETURNING id) SELECT id FROM cte; ---- -:.*Parser Error.*A CTE needs a SELECT.* \ No newline at end of file +:.*data-modifying statement must be at the top level.* + +# Correct syntax: WITH before INSERT +statement ok +WITH cte AS (INSERT INTO table1 SELECT 1, 2 RETURNING id) INSERT INTO table2 SELECT id FROM cte; + +query II +SELECT * FROM table1; +---- +1 2 + +query I +SELECT * FROM table2; +---- +1