[Maria-developers] Review: MDEV-29075 Changing explicit_defaults_for_timestamp within stored procedure works inconsistently

   Hello Sergei,

I have a couple of suggestions:

> commit 7b8304045272111a6f4d44196d6b37cbfef06f37
> Author: Sergei Golubchik <serg@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
>            {