Skip to content

Commit f6418ff

Browse files
committed
fix: MDEV-40651 ALTER TABLE and DDL in general is now executed in an atomic manner b/c previously MariaDB DDL triggered multiple autocommit DDL statements in DuckDB some of those can fail leaving table in unsable state.
1 parent 16ddaf8 commit f6418ff

10 files changed

Lines changed: 148 additions & 28 deletions

File tree

storage/duckdb/CLAUDE.md

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ Tests use MariaDB's MTR (MySQL Test Runner) framework. Test files live in `mysql
4646

4747
All engine code is in the `myduck` namespace (except `ha_duckdb` which is in global scope per MariaDB handler convention).
4848

49+
> **Doc convention:** when this file (or a code comment) names a specific API to describe control flow (e.g. `SELECT_LEX::print()`), it must also name the file/function where that API is actually called. If no call site can be pointed to, describe the behavior instead of naming the API — do not imply a code path that isn't there.
50+
4951
### Key components
5052

5153
- **`ha_duckdb`** (`ha_duckdb.cc/h`) — MariaDB `handler` subclass. Entry point for all storage engine operations (open, close, read, write, DDL). Implements row-at-a-time interface for MariaDB, translating to DuckDB batch operations.
@@ -66,7 +68,16 @@ All engine code is in the `myduck` namespace (except `ha_duckdb` which is in glo
6668

6769
### SQL generation conventions
6870

69-
All generated SQL must use **double quotes** for identifiers (DuckDB follows SQL standard), not backticks. The `SELECT_LEX::print()` output from MariaDB uses backticks and must be post-processed. See `docs/mariadb-duckdb-incompatibilities.md` for known function name rewrites and type mapping issues.
71+
DuckDB follows the SQL standard and delimits identifiers with **double quotes**, not backticks.
72+
73+
SQL reaches DuckDB by two routes:
74+
75+
- **Whole queries are forwarded verbatim.** SELECT and INSERT … SELECT are pushed down as the raw `thd->query()` text — see `extract_source_query()` in `ha_duckdb_pushdown.cc`. The engine does **not** re-print the whole query via `SELECT_LEX::print()`.
76+
- **Only fragments are printed.** `Item`/`COND::print()` is used for per-table WHERE conditions in cross-engine scan (`ha_duckdb_pushdown.cc`) and for DDL default / `nextval` expressions (`ddl_convertor.cc`). DDL/DML convertors also build identifier strings directly from `Field`/`TABLE` metadata.
77+
78+
Both routes then pass through `backticks_to_double_quotes()` (`runtime/duckdb_query.cc`), which is where identifier requoting actually happens: backtick-delimited identifiers are rewritten to double-quoted ones and any embedded double quote is escaped (MDEV-40653). Raw forwarding additionally goes through `mariadb_query_has_lexical_mismatch()`, which refuses to forward SQL whose backslash-escape semantics differ between MariaDB and DuckDB.
79+
80+
See `docs/mariadb-duckdb-incompatibilities.md` for known function name rewrites and type mapping issues.
7081

7182
### DuckDB source and patches
7283

storage/duckdb/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ DuckDB handles the join, aggregation, and sorting; InnoDB rows are produced on d
137137
- **Some MariaDB functions are yet not pushdown-compatible**`GROUP_CONCAT()`, `DATE_FORMAT()`, `JSON_CONTAINS()`, `FOUND_ROWS()`, `LAST_INSERT_ID()`, and a few others have no DuckDB equivalent or differ in syntax. Such queries fall back to MariaDB execution.
138138
- **Strict GROUP BY** — DuckDB rejects `SELECT` columns not in `GROUP BY` and not aggregated, even when MariaDB's `sql_mode` allows it.
139139
- **XA transactions**`XA PREPARE` is not supported by the engine.
140+
- **Table partitioning**`CREATE TABLE ... PARTITION BY` and converting a DuckDB table with `ALTER TABLE ... PARTITION BY` are not supported; the engine declares `HTON_NO_PARTITION`.
140141
- **Collations** — MariaDB UCA-based collation rules are approximated via DuckDB's built-in `NOCASE`/`NOACCENT` collations for UTF-8 charsets; non-UTF8 charsets fall back to binary comparison. See [`docs/collation-mapping.md`](docs/collation-mapping.md) for the full mapping and known gaps.
141142
- **Cross-engine scan is yet single-threaded** — each external (non-DuckDB) table is produced by a single fiber-driven MariaDB query (`_mdb_scan` reports `MaxThreads() == 1`); only the DuckDB side of the query is parallelized.
142143
- **ALTER COLUMN DROP DEFAULT** — not propagated to DuckDB catalog.

storage/duckdb/convertor/ddl_convertor.cc

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -259,17 +259,19 @@ static std::string autoinc_nextval_expr(const std::string &schema_name,
259259
Read the literal default value of a field from the default record and
260260
return it as a string suitable for DuckDB SQL.
261261
262-
BIT fields are converted to DuckDB blob literal format: '\xHH...'::BLOB.
263-
Other fields use standard quoted literal format: 'value'.
262+
BIT fields are converted to DuckDB blob literal format, optionally with an
263+
explicit BLOB cast. Other fields use standard quoted literal format.
264264
265265
@param field Field whose default value to read (must not be at offset)
266266
@param offset Offset from record[0] to default_values (s->default_values -
267267
record[0])
268+
@param cast_bit Add an explicit BLOB cast for BIT fields
268269
@return Default value string, or "NULL" if field is null at default
269270
record
270271
*/
271272
static std::string get_field_default_for_duckdb(Field *field,
272-
my_ptrdiff_t offset)
273+
my_ptrdiff_t offset,
274+
bool cast_bit= true)
273275
{
274276
field->move_field_offset(offset);
275277

@@ -295,15 +297,17 @@ static std::string get_field_default_for_duckdb(Field *field,
295297
ss << hx;
296298
}
297299
}
298-
ss << "'::BLOB";
300+
ss << "'";
301+
if (cast_bit)
302+
ss << "::BLOB";
299303
default_value= ss.str();
300304
}
301305
else
302306
{
303307
char buf[MAX_FIELD_WIDTH];
304308
String str(buf, sizeof(buf), system_charset_info);
305309
String *val= field->val_str(&str);
306-
if (val && val->length() > 0)
310+
if (val)
307311
{
308312
/*
309313
Escape the literal by doubling any embedded single quote so a crafted
@@ -312,11 +316,14 @@ static std::string get_field_default_for_duckdb(Field *field,
312316
charset aware and performs exactly this doubling.
313317
*/
314318
std::string escaped(2 * val->length(), '\0');
315-
my_bool overflow;
316-
size_t escaped_len= escape_quotes_for_mysql(val->charset(), &escaped[0],
317-
0, val->ptr(), val->length(),
318-
&overflow);
319-
escaped.resize(escaped_len);
319+
if (val->length())
320+
{
321+
my_bool overflow;
322+
size_t escaped_len= escape_quotes_for_mysql(
323+
val->charset(), &escaped[0], 0, val->ptr(), val->length(),
324+
&overflow);
325+
escaped.resize(escaped_len);
326+
}
320327
default_value= "'" + escaped + "'";
321328
}
322329
else
@@ -848,7 +855,11 @@ void AddColumnConvertor::prepare_columns()
848855
m_columns_to_add.emplace_back(new_field, field);
849856

850857
if ((new_field->flags & NOT_NULL_FLAG) != 0)
858+
{
851859
m_columns_to_set_not_null.emplace_back(new_field, field);
860+
if ((field->flags & NO_DEFAULT_VALUE_FLAG) != 0)
861+
m_columns_to_drop_default.emplace_back(new_field, field);
862+
}
852863
}
853864
}
854865

@@ -896,9 +907,16 @@ std::string AddColumnConvertor::translate()
896907
my_ptrdiff_t offset=
897908
field->table->s->default_values - field->table->record[0];
898909
has_default= true;
899-
default_value= get_field_default_for_duckdb(field, offset);
910+
default_value= get_field_default_for_duckdb(field, offset, false);
900911
}
901912
}
913+
else if (field->flags & NOT_NULL_FLAG)
914+
{
915+
my_ptrdiff_t offset=
916+
field->table->s->default_values - field->table->record[0];
917+
has_default= true;
918+
default_value= get_field_default_for_duckdb(field, offset, false);
919+
}
902920

903921
append_stmt_column_add(result, m_schema_name, m_table_name,
904922
new_field->field_name.str, type, has_default,
@@ -913,6 +931,13 @@ std::string AddColumnConvertor::translate()
913931
new_field->field_name.str);
914932
}
915933

934+
for (auto &pair : m_columns_to_drop_default)
935+
{
936+
Create_field *new_field= pair.first;
937+
append_stmt_column_drop_default(result, m_schema_name, m_table_name,
938+
new_field->field_name.str);
939+
}
940+
916941
return result.str();
917942
}
918943

storage/duckdb/convertor/ddl_convertor.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,9 @@ class AddColumnConvertor : public AlterTableConvertor
245245
/** Columns to set not null */
246246
Columns m_columns_to_set_not_null;
247247

248+
/** Columns whose temporary default must be dropped */
249+
Columns m_columns_to_drop_default;
250+
248251
/** Prepare columns to add and set not null. */
249252
void prepare_columns();
250253
};

storage/duckdb/docs/architecture.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,10 +190,13 @@ check_if_supported_inplace_alter() → HA_ALTER_INPLACE_NO_LOCK
190190
commit_inplace_alter_table()
191191
→ AddColumnConvertor / DropColumnConvertor / ChangeColumnConvertor /
192192
ChangeColumnDefaultConvertor / ChangeColumnForPrimaryKeyConvertor
193-
→ each operation executes in a separate auto-commit context
194-
(DuckDB v1.5+ disallows compound DDL mixing structural + constraint changes)
193+
→ all generated operations execute in one explicit DuckDB transaction
195194
```
196195

196+
Table partitioning is disabled with `HTON_NO_PARTITION`; both
197+
`CREATE TABLE ... PARTITION BY` and `ALTER TABLE ... PARTITION BY` are rejected
198+
before MariaDB creates a partition handler or starts the table-copy protocol.
199+
197200
DROP DATABASE: `duckdb_drop_database()``DROP SCHEMA IF EXISTS "db"`.
198201

199202
### Path 3: Row-by-Row DML

storage/duckdb/ha_duckdb.cc

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ static int duckdb_init_func(void *p)
229229
duckdb_hton= (handlerton *) p;
230230
duckdb_hton->db_type= DB_TYPE_AUTOASSIGN;
231231
duckdb_hton->create= duckdb_create_handler;
232-
duckdb_hton->flags= HTON_TEMPORARY_NOT_SUPPORTED;
232+
duckdb_hton->flags= HTON_TEMPORARY_NOT_SUPPORTED | HTON_NO_PARTITION;
233233
duckdb_hton->prepare= duckdb_prepare;
234234
duckdb_hton->commit= duckdb_commit;
235235
duckdb_hton->rollback= duckdb_rollback;
@@ -1274,6 +1274,9 @@ ha_duckdb::check_if_supported_inplace_alter(TABLE *altered_table,
12741274
if (ha_alter_info->alter_info->flags & ALTER_COLUMN_ORDER)
12751275
DBUG_RETURN(HA_ALTER_INPLACE_NOT_SUPPORTED);
12761276

1277+
if (ha_alter_info->error_if_not_empty)
1278+
DBUG_RETURN(HA_ALTER_INPLACE_NOT_SUPPORTED);
1279+
12771280
/* Reject ALTER on tables without PK when require_primary_key is ON */
12781281
if (myduck::require_primary_key && table->s->primary_key == MAX_KEY)
12791282
{
@@ -1353,29 +1356,53 @@ bool ha_duckdb::commit_inplace_alter_table(TABLE *altered_table,
13531356
if (convertors.empty())
13541357
DBUG_RETURN(false);
13551358

1356-
/* Execute each ALTER operation in its own auto-commit context.
1357-
DuckDB v1.5+ does not allow compound DDL that mixes structural
1358-
changes (ADD COLUMN) with constraint updates (SET DEFAULT)
1359-
within the same transaction. */
1360-
auto con= myduck::DuckdbManager::CreateConnection();
1361-
1359+
std::vector<std::string> statements;
13621360
for (auto &conv : convertors)
13631361
{
13641362
if (!conv || conv->check())
13651363
DBUG_RETURN(true);
13661364

13671365
std::string sql= conv->translate();
1368-
if (sql.empty())
1369-
continue;
1366+
if (!sql.empty())
1367+
statements.push_back(std::move(sql));
1368+
}
1369+
1370+
if (statements.empty())
1371+
DBUG_RETURN(false);
1372+
1373+
/* A single MariaDB ALTER TABLE can produce multiple DuckDB statements.
1374+
Execute the generated operations atomically on a dedicated connection. */
1375+
auto con= myduck::DuckdbManager::CreateConnection();
1376+
auto query_result= myduck::duckdb_query(*con, "BEGIN");
1377+
if (query_result->HasError())
1378+
{
1379+
my_error(ER_GET_ERRMSG, MYF(0), HA_ERR_GENERIC,
1380+
query_result->GetError().c_str(), "DuckDB");
1381+
DBUG_RETURN(true);
1382+
}
13701383

1371-
auto query_result= myduck::duckdb_query(*con, sql);
1384+
for (const auto &sql : statements)
1385+
{
1386+
query_result= myduck::duckdb_query(*con, sql);
13721387
if (query_result->HasError())
13731388
{
1374-
my_error(ER_GET_ERRMSG, MYF(0), HA_ERR_GENERIC, query_result->GetError().c_str(), "DuckDB");
1389+
std::string error= query_result->GetError();
1390+
myduck::duckdb_query(*con, "ROLLBACK");
1391+
my_error(ER_GET_ERRMSG, MYF(0), HA_ERR_GENERIC, error.c_str(), "DuckDB");
13751392
DBUG_RETURN(true);
13761393
}
13771394
}
13781395

1396+
query_result= myduck::duckdb_query(*con, "COMMIT");
1397+
if (query_result->HasError())
1398+
{
1399+
std::string error= query_result->GetError();
1400+
if (con->HasActiveTransaction())
1401+
myduck::duckdb_query(*con, "ROLLBACK");
1402+
my_error(ER_GET_ERRMSG, MYF(0), HA_ERR_GENERIC, error.c_str(), "DuckDB");
1403+
DBUG_RETURN(true);
1404+
}
1405+
13791406
DBUG_RETURN(false);
13801407
}
13811408

storage/duckdb/mysql-test/duckdb/r/alter_duckdb_column.result

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -625,7 +625,7 @@ VARCHAR VARCHAR VARCHAR VARCHAR VARCHAR VARCHAR VARCHAR
625625
db_alter_col t id NULL NO INTEGER NULL
626626
db_alter_col t B0 CAST('\x00' AS "BLOB") NO BLOB NULL
627627
db_alter_col t B1 CAST('\x00\x00\x0D\x05' AS "BLOB") NO BLOB NULL
628-
db_alter_col t B2 CAST('\x00\x00\x00\x00\x00\x00\x00\x1F' AS "BLOB") NO BLOB NULL
628+
db_alter_col t B2 '\x00\x00\x00\x00\x00\x00\x00\x1F' NO BLOB NULL
629629

630630

631631
SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, COLUMN_DEFAULT, IS_NULLABLE, DATA_TYPE, COLUMN_COMMENT FROM information_schema.columns WHERE TABLE_NAME = 't';
@@ -1503,7 +1503,7 @@ VARCHAR VARCHAR VARCHAR VARCHAR VARCHAR VARCHAR VARCHAR
15031503
db_alter_col t id NULL NO INTEGER NULL
15041504
db_alter_col t B0 CAST('\x00' AS "BLOB") NO BLOB NULL
15051505
db_alter_col t B1 CAST('\x00\x00\x0D\x05' AS "BLOB") NO BLOB NULL
1506-
db_alter_col t B2 CAST('\x00\x00\x00\x00\x00\x00\x00\x1F' AS "BLOB") NO BLOB NULL
1506+
db_alter_col t B2 '\x00\x00\x00\x00\x00\x00\x00\x1F' NO BLOB NULL
15071507

15081508

15091509
SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, COLUMN_DEFAULT, IS_NULLABLE, DATA_TYPE, COLUMN_COMMENT FROM information_schema.columns WHERE TABLE_NAME = 't';
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
CREATE TABLE t (c1 INT KEY) ENGINE=DuckDB;
2+
INSERT INTO t VALUES (1),(1);
3+
ALTER TABLE t ADD c2 INT NOT NULL;
4+
SELECT * FROM t ORDER BY c1, c2;
5+
c1 c2
6+
1 0
7+
1 0
8+
INSERT INTO t (c1) VALUES (2);
9+
ERROR HY000: Field 'c2' doesn't have a default value
10+
ALTER TABLE t PARTITION BY HASH (c1) (PARTITION p1, PARTITION p2);
11+
ERROR HY000: Engine cannot be used in partitioned tables
12+
SELECT * FROM t ORDER BY c1, c2;
13+
c1 c2
14+
1 0
15+
1 0
16+
DROP TABLE t;
17+
CREATE TABLE t (id INT PRIMARY KEY, c1 INT) ENGINE=DuckDB;
18+
INSERT INTO t VALUES (1, NULL);
19+
ALTER TABLE t ADD c2 INT, MODIFY c1 INT NOT NULL;
20+
ERROR HY000: Got error 168 'Constraint Error: NOT NULL constraint failed: t.c1' from DuckDB
21+
ALTER TABLE t ADD c2 INT;
22+
SELECT * FROM t ORDER BY id;
23+
id c1 c2
24+
1 NULL NULL
25+
DROP TABLE t;
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
--source ../include/have_duckdb.inc
2+
--source include/have_partition.inc
3+
--source include/not_msan.inc
4+
5+
# MDEV-40651: keep DuckDB DDL atomic and preserve implicit defaults.
6+
CREATE TABLE t (c1 INT KEY) ENGINE=DuckDB;
7+
INSERT INTO t VALUES (1),(1);
8+
ALTER TABLE t ADD c2 INT NOT NULL;
9+
SELECT * FROM t ORDER BY c1, c2;
10+
--error ER_NO_DEFAULT_FOR_FIELD
11+
INSERT INTO t (c1) VALUES (2);
12+
--error ER_PARTITION_MERGE_ERROR
13+
ALTER TABLE t PARTITION BY HASH (c1) (PARTITION p1, PARTITION p2);
14+
SELECT * FROM t ORDER BY c1, c2;
15+
DROP TABLE t;
16+
17+
# A later DuckDB DDL error rolls back preceding generated statements.
18+
CREATE TABLE t (id INT PRIMARY KEY, c1 INT) ENGINE=DuckDB;
19+
INSERT INTO t VALUES (1, NULL);
20+
--error ER_GET_ERRMSG
21+
ALTER TABLE t ADD c2 INT, MODIFY c1 INT NOT NULL;
22+
ALTER TABLE t ADD c2 INT;
23+
SELECT * FROM t ORDER BY id;
24+
DROP TABLE t;

storage/duckdb/runtime/duckdb_query.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,8 @@ bool mariadb_query_has_unsafe_quote_escape(THD *thd, const char *query,
135135
}
136136

137137
/*
138-
Convert MariaDB's printed SQL (backtick-quoted identifiers) into DuckDB SQL
138+
Convert forwarded MariaDB SQL (the raw thd->query() text, plus any
139+
Item::print() fragments) from backtick-quoted identifiers into DuckDB SQL
139140
(double-quoted identifiers).
140141
141142
MariaDB delimits identifiers with backticks and doubles an embedded backtick;

0 commit comments

Comments
 (0)