Re: b508107c7a3: MDEV-31631 Adding auto-increment to table with history online misbehaves
Hi, Nikita, Looks good. One comment below: On Aug 04, Nikita Malyavin wrote:
revision-id: b508107c7a3 (mariadb-11.0.1-189-gb508107c7a3) parent(s): e9a75c98781 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-07-29 00:13:48 +0400 message:
MDEV-31631 Adding auto-increment to table with history online misbehaves
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index eabee13e8d3..3a70ed672a6 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -9953,6 +9953,37 @@ const char *online_alter_check_supported(const THD *thd, } }
+ for (auto &c: alter_info->create_list) + { + *online= c.field || !(c.flags & AUTO_INCREMENT_FLAG); + if (!*online) + return "ADD COLUMN ... AUTO_INCREMENT"; + + auto *def= c.default_value; + *online= !(def && def->flags & VCOL_NEXTVAL + // either it's a new field, or a NULL -> NOT NULL change + && (!c.field || (!(c.field->flags & NOT_NULL_FLAG) + && (c.flags & NOT_NULL_FLAG)))); + if (!*online) + { + const char *fmt= ER_THD(thd, ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED); + + Item::vcol_func_processor_result res; + IF_DBUG(bool ret=,) + def->expr->walk(&Item::check_vcol_func_processor, 0, &res);
I don't think you need to walk here, res.name is always "nextval()" isn't it?
+ DBUG_ASSERT(!ret); + DBUG_ASSERT(res.errors & VCOL_NEXTVAL); + + LEX_CSTRING dflt{STRING_WITH_LEN("DEFAULT")}; + size_t len= strlen(fmt) + strlen(res.name) + c.field_name.length + + dflt.length; + char *resp= (char*)thd->alloc(len); + // expression %s cannot be used in the %s clause of %`s + my_snprintf(resp, len, fmt, res.name, dflt.str, c.field_name.str); + return resp; + } + }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
On Fri, 4 Aug 2023 at 16:25, Sergei Golubchik
Hi, Nikita,
Looks good. One comment below:
On Aug 04, Nikita Malyavin wrote:
revision-id: b508107c7a3 (mariadb-11.0.1-189-gb508107c7a3) parent(s): e9a75c98781 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-07-29 00:13:48 +0400 message:
MDEV-31631 Adding auto-increment to table with history online misbehaves
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index eabee13e8d3..3a70ed672a6 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -9953,6 +9953,37 @@ const char *online_alter_check_supported(const THD *thd, } }
+ for (auto &c: alter_info->create_list) + { + *online= c.field || !(c.flags & AUTO_INCREMENT_FLAG); + if (!*online) + return "ADD COLUMN ... AUTO_INCREMENT"; + + auto *def= c.default_value; + *online= !(def && def->flags & VCOL_NEXTVAL + // either it's a new field, or a NULL -> NOT NULL change + && (!c.field || (!(c.field->flags & NOT_NULL_FLAG) + && (c.flags & NOT_NULL_FLAG)))); + if (!*online) + { + const char *fmt= ER_THD(thd, ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED); + + Item::vcol_func_processor_result res; + IF_DBUG(bool ret=,) + def->expr->walk(&Item::check_vcol_func_processor, 0, &res);
I don't think you need to walk here, res.name is always "nextval()" isn't it?
It can be NEXTVAL() 🙂 Seriously -- I didn't look at the text result of the error message [that close], so I didn't notice that it's not nextval(s). Removing the walk.
+ DBUG_ASSERT(!ret); + DBUG_ASSERT(res.errors & VCOL_NEXTVAL); + + LEX_CSTRING dflt{STRING_WITH_LEN("DEFAULT")}; + size_t len= strlen(fmt) + strlen(res.name) + c.field_name.length + + dflt.length; + char *resp= (char*)thd->alloc(len); + // expression %s cannot be used in the %s clause of %`s + my_snprintf(resp, len, fmt, res.name, dflt.str, c.field_name.str); + return resp; + } + }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik