Hello Sergei,
I have a couple of suggestions:
> commit 7b8304045272111a6f4d44196d6b37cbfef06f37
> Author: Sergei Golubchik <serg(a)mariadb.org>
> Date: Wed Jul 20 17:31:48 2022 +0200
>
> MDEV-29075 Changing explicit_defaults_for_timestamp within stored procedure works inconsistently
>
> move the OPTION_EXPLICIT_DEF_TIMESTAMP check from the parsing step
> to the execution
>
<cut>
> > diff --git a/sql/field.cc b/sql/field.cc
> index 2e0c70d3d13..23ebb07b7f7 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -10824,6 +10824,7 @@ Column_definition::Column_definition(THD *thd, Field *old_field,
> comment= old_field->comment;
> vcol_info= old_field->vcol_info;
> option_list= old_field->option_list;
> + explicitly_nullable= !(old_field->flags & NOT_NULL_FLAG);
This introduces asymmetry in how NULL and NOT NULL attributes are handled.
I suggest considering these ways:
1. Change "bool explicitly_nullable" to "bool explicit_nullability",
which will be:
- false if nothing was specified
- true if NULL or NOT NULL was specified.
2. Or don't store NOT_NULL_FLAG in Column_definition::flags at all.
Add a new Column_definition enum member with three values for:
- not specified
- explicit NULL
- explicit NOT NULL
As discussed on slack, the latter will need some changes.
But I think not that much, as only Column_definition needs changes,
while Field does not seem to need at this point.
> compression_method_ptr= 0;
> versioning= VERSIONING_NOT_SET;
> invisible= old_field->invisible;
> diff --git a/sql/field.h b/sql/field.h
> index 5d5be5204cd..87a30bd9f95 100644
> --- a/sql/field.h
> +++ b/sql/field.h
> @@ -5281,7 +5281,7 @@ class Column_definition: public Sql_alloc,
> uint flags, pack_length;
> List<String> interval_list;
> engine_option_value *option_list;
> -
> + bool explicitly_nullable;
>
> /*
> This is additinal data provided for any computed(virtual) field.
> @@ -5303,7 +5303,7 @@ class Column_definition: public Sql_alloc,
> comment(null_clex_str),
> on_update(NULL), invisible(VISIBLE), char_length(0),
> flags(0), pack_length(0),
> - option_list(NULL),
> + option_list(NULL), explicitly_nullable(false),
> vcol_info(0), default_value(0), check_constraint(0),
> versioning(VERSIONING_NOT_SET), period(NULL)
> {
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index 3eeabe20c71..8c31f988a05 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -2316,6 +2316,8 @@ void promote_first_timestamp_column(List<Create_field> *column_definitions)
> if (column_definition.is_timestamp_type() || // TIMESTAMP
> column_definition.unireg_check == Field::TIMESTAMP_OLD_FIELD) // Legacy
> {
> + if (!column_definition.explicitly_nullable)
> + column_definition.flags|= NOT_NULL_FLAG;
I think this does not work well with PS.
On the very first execution the field becomes NOT NULL
forever, if the current OPTION_EXPLICIT_DEF_TIMESTAMP says so.
> DBUG_PRINT("info", ("field-ptr:%p", column_definition.field));
> if (first &&
> (column_definition.flags & NOT_NULL_FLAG) != 0 && // NOT NULL,
> diff --git a/sql/sql_type.cc b/sql/sql_type.cc
> index 44a51428280..dc0c0b1e0cf 100644
> --- a/sql/sql_type.cc
> +++ b/sql/sql_type.cc
> @@ -4258,18 +4258,6 @@ void Type_handler_temporal_with_date::Item_update_null_value(Item *item) const
> (void) item->get_date(thd, <ime, Datetime::Options(thd));
> }
>
> -bool
> -Type_handler_timestamp_common::
> -Column_definition_set_attributes(THD *thd,
> - Column_definition *def,
> - const Lex_field_type_st &attr,
> - column_definition_type_t type) const
> -{
> - Type_handler::Column_definition_set_attributes(thd, def, attr, type);
> - if (!(thd->variables.option_bits & OPTION_EXPLICIT_DEF_TIMESTAMP))
> - def->flags|= NOT_NULL_FLAG;
> - return false;
> -
>
> void Type_handler_string_result::Item_update_null_value(Item *item) const
> {
> diff --git a/sql/sql_type.h b/sql/sql_type.h
> index 7ff4bc64679..e5f54535ece 100644
> --- a/sql/sql_type.h
> +++ b/sql/sql_type.h
> @@ -6655,11 +6655,6 @@ class Type_handler_timestamp_common: public Type_handler_temporal_with_date
> bool Item_func_min_max_get_date(THD *thd, Item_func_min_max*,
> MYSQL_TIME *, date_mode_t fuzzydate)
> const override;
> - bool Column_definition_set_attributes(THD *thd,
> - Column_definition *def,
> - const Lex_field_type_st &attr,
> - column_definition_type_t type)
> - const override;
> };
>
>
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 5416cec49b5..f326d8c0cc5 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -5831,7 +5831,6 @@ field_def:
> | opt_generated_always AS virtual_column_func
> {
> Lex->last_field->vcol_info= $3;
> - Lex->last_field->flags&= ~NOT_NULL_FLAG; // undo automatic NOT NULL for timestamps
> }
> vcol_opt_specifier vcol_opt_attribute
> {
> @@ -6313,7 +6312,12 @@ attribute_list:
> ;
>
> attribute:
> - NULL_SYM { Lex->last_field->flags&= ~ NOT_NULL_FLAG; $$.init(); }
> + NULL_SYM
> + {
> + Lex->last_field->flags&= ~NOT_NULL_FLAG;
> + Lex->last_field->explicitly_nullable= true;
> + $$.init();
> + }
> | DEFAULT column_default_expr { Lex->last_field->default_value= $2; $$.init(); }
> | ON UPDATE_SYM NOW_SYM opt_default_time_precision
> {
