MDEV-16094 Crash when using AS OF with a stored function

MDEV-16100 FOR SYSTEM_TIME erroneously resolves string user variables as transaction IDs

Problem:

Vers_history_point::resolve_unit() tested item->result_type() before
item->fix_fields() was called.

- Item_func_get_user_var::result_type() returned REAL_RESULT by default.
  This caused MDEV-16100.
- Item_func_sp::result_type() crashed on assert.
  This caused MDEV-16094

Changes:
1. Adding item->fix_fields() into Vers_history_point::resolve_unit()
   before using data type specific properties of the history point
   expression.

2. Adding a new virtual method Type_handler::Vers_history_point_resolve_unit()

3. Implementing type-specific
   Type_handler_xxx::Type_handler::Vers_history_point_resolve_unit()
    in the way to:
    a. resolve temporal and general purpose string types to TIMESTAMP
    b. resolve BIT and general purpose INT types to TRANSACTION
    c. disallow use of non-relevant data type expressions in FOR SYSTEM_TIME

    Note, DOUBLE and DECIMAL data types are disallowed intentionally.
    - DOUBLE does not have enough precision to hold huge BIGINT UNSIGNED values
    - DECIMAL rounds on conversion to INT
    Both lack of precision and rounding might potentionally lead to
    very unpredictable results when a wrong transaction ID would be chosen.
    If one really wants dangerous use of DOUBLE and DECIMAL, explicit CAST
    can be used:

      FOR SYSTEM_TIME AS OF CAST(double_or_decimal AS UNSIGNED)

    QQ: perhaps DECIMAL(N,0) could still be allowed.

4. Adding a new virtual method Item::type_handler_for_system_time(),
   to make HEX hybrids and bit literals work as TRANSACTION rather
   than TIMESTAMP.

5. sql_yacc.yy: replacing the rule temporal_literal to "TIMESTAMP TEXT_STRING".
   Other temporal literals now resolve to TIMESTAMP through the new
   Type_handler methods. No special grammar needed. This removed
   a few shift/resolve conflicts.
   (TIMESTAMP related conflicts in "history_point:" will be removed separately)

6. Removing the "timestamp_only" parameter from
   vers_select_conds_t::resolve_units() and Vers_history_point::resolve_unit().
   It was a hint telling that a table did not have any TRANSACTION-aware
   system time columns, so it's OK to resolve to TIMESTAMP in case of uncertainty.
   In the new reduction it works as follows:
   - the decision between TIMESTAMP and TRANSACTION is first made
     based only on the expression data type only
   - then, in case if the expression resolved to TRANSACTION, the table
     is checked if TRANSACTION-aware columns really exist.
   This way is safer against possible ALTER TABLE statements changing
   ROW START and ROW END columns from "BIGINT UNSIGNED" to "TIMESTAMP(x)"
   or the other way around.

7 files changed, 121 insertions, 21 deletions

diff --git a/sql/item.h b/sql/item.h
index a9750e0619b..3ebb92b3afb 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -845,6 +845,10 @@ public:
   {
     return type_handler();
   }
+  virtual const Type_handler *type_handler_for_system_time() const
+  {
+    return real_type_handler();
+  }
   /* result_type() of an item specifies how the value should be returned */
   Item_result result_type() const
   {
@@ -4213,6 +4217,10 @@ public:
   {
     return &type_handler_longlong;
   }
+  const Type_handler *type_handler_for_system_time() const
+  {
+    return &type_handler_longlong;
+  }
   void print(String *str, enum_query_type query_type);
   Item *get_copy(THD *thd)
   { return get_item_copy<Item_hex_hybrid>(thd, this); }
diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index f658b78c33c..987415550fa 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -835,11 +835,13 @@ int SELECT_LEX::vers_setup_conds(THD *thd, TABLE_LIST *tables)
 
     if (vers_conditions.is_set())
     {
+      thd->where= "FOR SYSTEM_TIME";
       /* TODO: do resolve fix_length_and_dec(), fix_fields(). This requires
         storing vers_conditions as Item and make some magic related to
         vers_system_time_t/VERS_TRX_ID at stage of fix_fields()
         (this is large refactoring). */
-      vers_conditions.resolve_units(timestamps_only);
+      if (vers_conditions.resolve_units(thd))
+        DBUG_RETURN(-1);
       if (timestamps_only && (vers_conditions.start.unit == VERS_TRX_ID ||
         vers_conditions.end.unit == VERS_TRX_ID))
       {
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
index fe5ce3f8b9f..22eebaf6a38 100644
--- a/sql/sql_type.cc
+++ b/sql/sql_type.cc
@@ -5786,3 +5786,62 @@ void Type_handler_geometry::Item_param_set_param_func(Item_param *param,
 #endif
 
 /***************************************************************************/
+
+bool Type_handler::Vers_history_point_resolve_unit(THD *thd,
+                                                   Vers_history_point *point)
+                                                   const
+{
+  /*
+    Disallow using non-relevant data types in history points.
+    Even expressions with explicit TRANSACTION or TIMESTAMP units.
+  */
+  point->bad_expression_data_type_error(name().ptr());
+  return true;
+}
+
+
+bool Type_handler_typelib::
+       Vers_history_point_resolve_unit(THD *thd,
+                                       Vers_history_point *point) const
+{
+  /*
+    ENUM/SET have dual type properties (string and numeric).
+    Require explicit CAST to avoid ambiguity.
+  */
+  point->bad_expression_data_type_error(name().ptr());
+  return true;
+}
+
+
+bool Type_handler_general_purpose_int::
+       Vers_history_point_resolve_unit(THD *thd,
+                                       Vers_history_point *point) const
+{
+  return point->resolve_unit_trx_id(thd);
+}
+
+
+bool Type_handler_bit::
+       Vers_history_point_resolve_unit(THD *thd,
+                                       Vers_history_point *point) const
+{
+  return point->resolve_unit_trx_id(thd);
+}
+
+
+bool Type_handler_temporal_result::
+       Vers_history_point_resolve_unit(THD *thd,
+                                       Vers_history_point *point) const
+{
+  return point->resolve_unit_timestamp(thd);
+}
+
+
+bool Type_handler_general_purpose_string::
+       Vers_history_point_resolve_unit(THD *thd,
+                                       Vers_history_point *point) const
+{
+  return point->resolve_unit_timestamp(thd);
+}
+
+/***************************************************************************/
diff --git a/sql/sql_type.h b/sql/sql_type.h
index 16db421a4d3..1ddcef2da61 100644
--- a/sql/sql_type.h
+++ b/sql/sql_type.h
@@ -72,6 +72,7 @@ class handler;
 struct Schema_specification_st;
 struct TABLE;
 struct SORT_FIELD_ATTR;
+class Vers_history_point;
 
 
 /**
@@ -1409,6 +1410,9 @@ public:
   Item_func_div_fix_length_and_dec(Item_func_div *func) const= 0;
   virtual bool
   Item_func_mod_fix_length_and_dec(Item_func_mod *func) const= 0;
+
+  virtual bool
+  Vers_history_point_resolve_unit(THD *thd, Vers_history_point *point) const;
 };
 
 
@@ -2105,6 +2109,7 @@ public:
   virtual const Type_limits_int *
     type_limits_int_by_unsigned_flag(bool unsigned_flag) const= 0;
   uint32 max_display_length(const Item *item) const;
+  bool Vers_history_point_resolve_unit(THD *thd, Vers_history_point *p) const;
 };
 
 
@@ -2174,6 +2179,7 @@ public:
   bool Item_func_mul_fix_length_and_dec(Item_func_mul *) const;
   bool Item_func_div_fix_length_and_dec(Item_func_div *) const;
   bool Item_func_mod_fix_length_and_dec(Item_func_mod *) const;
+  bool Vers_history_point_resolve_unit(THD *thd, Vers_history_point *p) const;
 };
 
 
@@ -2298,6 +2304,7 @@ class Type_handler_general_purpose_string: public Type_handler_string_result
 {
 public:
   bool is_general_purpose_string_type() const { return true; }
+  bool Vers_history_point_resolve_unit(THD *thd, Vers_history_point *p) const;
 };
 
 
@@ -2570,6 +2577,7 @@ public:
                           const Record_addr &addr,
                           const Type_all_attributes &attr,
                           TABLE *table) const;
+  bool Vers_history_point_resolve_unit(THD *thd, Vers_history_point *p) const;
 };
 
 
@@ -3384,6 +3392,7 @@ public:
                                          const;
   void Item_param_set_param_func(Item_param *param,
                                  uchar **pos, ulong len) const;
+  bool Vers_history_point_resolve_unit(THD *thd, Vers_history_point *p) const;
 };
 
 
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index 157bca1def2..2b44c325739 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -892,10 +892,10 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize);
 %parse-param { THD *thd }
 %lex-param { THD *thd }
 /*
-  Currently there are 139 shift/reduce conflicts.
+  Currently there are 127 shift/reduce conflicts.
   We should not introduce new conflicts any more.
 */
-%expect 139
+%expect 127
 
 /*
    Comments for TOKENS.
@@ -9251,9 +9251,13 @@ opt_history_unit:
         ;
 
 history_point:
-          temporal_literal
+          TIMESTAMP TEXT_STRING
           {
-            $$= Vers_history_point(VERS_TIMESTAMP, $1);
+            Item *item;
+            if (!(item= create_temporal_literal(thd, $2.str, $2.length, YYCSCL,
+                                                MYSQL_TYPE_DATETIME, true)))
+              MYSQL_YYABORT;
+            $$= Vers_history_point(VERS_TIMESTAMP, item);
           }
         | function_call_keyword_timestamp
           {
diff --git a/sql/table.cc b/sql/table.cc
index b99f1a0cbdd..dd0bff062d7 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -8879,12 +8879,12 @@ bool TR_table::check(bool error)
   return false;
 }
 
-void vers_select_conds_t::resolve_units(bool timestamps_only)
+bool vers_select_conds_t::resolve_units(THD *thd)
 {
   DBUG_ASSERT(type != SYSTEM_TIME_UNSPECIFIED);
   DBUG_ASSERT(start.item);
-  start.resolve_unit(timestamps_only);
-  end.resolve_unit(timestamps_only);
+  return start.resolve_unit(thd) ||
+         end.resolve_unit(thd);
 }
 
 bool vers_select_conds_t::eq(const vers_select_conds_t &conds) const
@@ -8907,20 +8907,25 @@ bool vers_select_conds_t::eq(const vers_select_conds_t &conds) const
   return false;
 }
 
-void Vers_history_point::resolve_unit(bool timestamps_only)
+
+bool Vers_history_point::resolve_unit(THD *thd)
 {
-  if (item && unit == VERS_UNDEFINED)
-  {
-    if (item->type() == Item::FIELD_ITEM || timestamps_only)
-      unit= VERS_TIMESTAMP;
-    else if (item->result_type() == INT_RESULT ||
-             item->result_type() == REAL_RESULT)
-      unit= VERS_TRX_ID;
-    else
-      unit= VERS_TIMESTAMP;
-  }
+  if (!item)
+    return false;
+  if (!item->fixed && item->fix_fields(thd, &item))
+    return true;
+  return item->this_item()->type_handler_for_system_time()->
+           Vers_history_point_resolve_unit(thd, this);
 }
 
+
+void Vers_history_point::bad_expression_data_type_error(const char *type) const
+{
+  my_error(ER_ILLEGAL_PARAMETER_DATA_TYPE_FOR_OPERATION, MYF(0),
+           type, "FOR SYSTEM_TIME");
+}
+
+
 void Vers_history_point::fix_item()
 {
   if (item && item->decimals == 0 && item->type() == Item::FUNC_ITEM &&
diff --git a/sql/table.h b/sql/table.h
index 6fd3f219914..977c247d1a1 100644
--- a/sql/table.h
+++ b/sql/table.h
@@ -1828,7 +1828,20 @@ public:
   }
   void empty() { unit= VERS_UNDEFINED; item= NULL; }
   void print(String *str, enum_query_type, const char *prefix, size_t plen) const;
-  void resolve_unit(bool timestamps_only);
+  bool resolve_unit(THD *thd);
+  bool resolve_unit_trx_id(THD *thd)
+  {
+    if (unit == VERS_UNDEFINED)
+      unit= VERS_TRX_ID;
+    return false;
+  }
+  bool resolve_unit_timestamp(THD *thd)
+  {
+    if (unit == VERS_UNDEFINED)
+      unit= VERS_TIMESTAMP;
+    return false;
+  }
+  void bad_expression_data_type_error(const char *type) const;
   bool eq(const vers_history_point_t &point) const;
 };
 
@@ -1866,7 +1879,7 @@ struct vers_select_conds_t
   {
     return type != SYSTEM_TIME_UNSPECIFIED;
   }
-  void resolve_units(bool timestamps_only);
+  bool resolve_units(THD *thd);
   bool user_defined() const
   {
     return !from_query && type != SYSTEM_TIME_UNSPECIFIED;