XXX10.3-serg

9 files changed, 142 insertions, 6 deletions

diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
index deeda0bbe6c..b3ee6e3067d 100644
--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -2035,6 +2035,7 @@ int ha_partition::copy_partitions(ulonglong * const copied,
         result= new_file->ha_write_row(m_rec0);
         reenable_binlog(thd);
         if (new_file->ha_external_lock(thd, F_UNLCK) || new_file->ha_external_lock(thd, F_RDLCK) || result)
+        // XXX and here
           goto error;
       }
     }
diff --git a/sql/log_event.cc b/sql/log_event.cc
index 4a48ebce077..35a0ce5657f 100644
--- a/sql/log_event.cc
+++ b/sql/log_event.cc
@@ -12801,6 +12801,9 @@ static bool record_compare(TABLE *table)
   {
     if (table->versioned_by_engine() && *ptr == table->vers_start_field())
     {
+      // XXX hmm. I wonder whether it's possible to replicate
+      // these trx id values too. It'd be a very good feature. Currently
+      // the slave and the master have essentially different data.
       continue;
     }
     /**
@@ -13004,6 +13007,8 @@ int Rows_log_event::find_row(rpl_group_info *rgi)
     DBUG_ASSERT(table->read_set);
     bitmap_set_bit(table->read_set, sys_trx_end->field_index);
     // master table is unversioned
+    // XXX do you mean *if* master table is unversioned?
+    // or that it's always guaranteed to be unversioned here?
     if (sys_trx_end->val_int() == 0)
     {
       DBUG_ASSERT(table->write_set);
@@ -13011,6 +13016,9 @@ int Rows_log_event::find_row(rpl_group_info *rgi)
       sys_trx_end->set_max();
       table->vers_start_field()->set_notnull();
     }
+    // XXX second question, if the master table is versioned,
+    // sys_trx_end column is guaranteed to be MAX, no?
+    // history values can't be replicated, can they?
   }
 
   DBUG_PRINT("info",("looking for the following record"));
diff --git a/sql/mysqld.h b/sql/mysqld.h
index e8a72b0b063..d07c9cf7cab 100644
--- a/sql/mysqld.h
+++ b/sql/mysqld.h
@@ -177,6 +177,7 @@ extern const char *log_output_str;
 extern const char *log_backup_output_str;
 
 enum vers_range_type_t
+// XXX better put these definitions in sql_class.h, not in mysqld.h
 {
   FOR_SYSTEM_TIME_UNSPECIFIED = 0,
   FOR_SYSTEM_TIME_AS_OF,
@@ -186,8 +187,12 @@ enum vers_range_type_t
   FOR_SYSTEM_TIME_BEFORE
 };
 
+// XXX add a comment, saying where this structure is used
+// (only for @@vers_current_time sysvar)
 struct st_vers_current_time
 { // This struct must be POD, so no virtual-anything!
+  // XXX why? I mean, the comment must explain why it must be POD
+  // (on the other hand, you don't have to explain what POD is)
   char *str_value; // must be first
   vers_range_type_t type;
   MYSQL_TIME ltime;
@@ -198,6 +203,7 @@ struct st_vers_current_time
 };
 
 enum vers_hide_enum {
+  // XXX looks like it's unused?
   VERS_HIDE_AUTO= 0,
   VERS_HIDE_IMPLICIT,
   VERS_HIDE_FULL,
@@ -344,6 +350,7 @@ extern PSI_rwlock_key key_rwlock_LOCK_grant, key_rwlock_LOCK_logger,
   key_rwlock_LOCK_sys_init_connect, key_rwlock_LOCK_sys_init_slave,
   key_rwlock_LOCK_system_variables_hash, key_rwlock_query_cache_query_lock,
   key_rwlock_LOCK_vers_stats, key_rwlock_LOCK_stat_serial;
+// XXX last two aren't used anywhere
 
 #ifdef HAVE_MMAP
 extern PSI_cond_key key_PAGE_cond, key_COND_active, key_COND_pool;
diff --git a/sql/partition_element.h b/sql/partition_element.h
index 72413146887..61784ab5aab 100644
--- a/sql/partition_element.h
+++ b/sql/partition_element.h
@@ -91,8 +91,17 @@ typedef struct p_elem_val
 struct st_ddl_log_memory_entry;
 
 class Vers_field_stats : public Sql_alloc
+// XXX "stats"? what does that mean?
+// please add a comment explaining what this structure is for
+// or (better) rename it to something that doesn't need explaining
 {
   static const uint buf_size= 4 + (TIME_SECOND_PART_DIGITS + 1) / 2;
+  // XXX please, avoid static const class members, use, for example,
+  //
+  //    enum { buf_size = 4 + (TIME_SECOND_PART_DIGITS + 1) / 2};
+  //
+  // this is much more gdb-friendly. Just print Alter_inplace_info
+  // object in gdb to see what I mean.
   uchar min_buf[buf_size];
   uchar max_buf[buf_size];
   Field_timestampf min_value;
@@ -169,8 +178,8 @@ public:
   bool has_null_value;
   bool signed_flag;                          // Range value signed
   bool max_value;                            // MAXVALUE range
-  uint32 id;
-  bool empty;
+  uint32 id;  // XXX why is that?
+  bool empty; // XXX why is that?
 
   enum elem_type
   {
@@ -180,6 +189,10 @@ public:
   };
 
   elem_type type;
+  // XXX I don't think this is needed. 'max_value' flag means
+  // MAXVALUE for RANGE partitioning, DEFAULT for LIST partitioning.
+  // I don't see why it cannot mean AS_OF_NOW for SYSTEM_TIME partitioning.
+  // (in a sense AS_OF_NOW *is* max_value of the second timestamp column)
 
   partition_element()
   : part_max_rows(0), part_min_rows(0), range_value(0),
@@ -215,8 +228,12 @@ public:
   part_column_list_val& get_col_val(uint idx)
   {
     DBUG_ASSERT(type != CONVENTIONAL);
+    // XXX I don't see why it should be type != CONVENTIONAL.
+    // The method is fairly generic
     DBUG_ASSERT(list_val_list.elements == 1);
     part_elem_value *ev= static_cast<part_elem_value*>(list_val_list.first_node()->info);
+    // XXX That's not how one uses List.
+    // Better: ev= list_val_list->head();
     DBUG_ASSERT(ev);
     DBUG_ASSERT(ev->col_val_array);
     return ev->col_val_array[idx];
diff --git a/sql/partition_info.cc b/sql/partition_info.cc
index ac4e4a162c7..b439c381387 100644
--- a/sql/partition_info.cc
+++ b/sql/partition_info.cc
@@ -889,11 +889,15 @@ partition_info::vers_part_rotate(THD * thd)
 
 bool partition_info::vers_setup_1(THD * thd, uint32 added)
 {
+  // XXX generally, the more cryptic function name is, the longer and more
+  // detailed function comment must be. "vers_setup_1" is very cryptic :)
   DBUG_ASSERT(part_type == VERSIONING_PARTITION);
 
   if (!table->versioned())
   {
     my_error(ER_VERSIONING_REQUIRED, MYF(0), "`BY SYSTEM_TIME` partitioning");
+    // XXX backticks are only used to quote identifiers, don't use them
+    // like this.
     return true;
   }
 
@@ -910,6 +914,7 @@ bool partition_info::vers_setup_1(THD * thd, uint32 added)
     Field *sys_trx_end= table->vers_end_field();
     part_field_list.empty();
     part_field_list.push_back(const_cast<char *>(sys_trx_end->field_name), thd->mem_root);
+    // XXX what was the content of part_field_list before you repopulated it?
     DBUG_ASSERT(part_field_list.elements == num_columns);
     // needed in handle_list_of_fields()
     sys_trx_end->flags|= GET_FIXED_FIELDS_FLAG;
@@ -1361,6 +1366,12 @@ error:
 */
 
 bool partition_info::check_range_constants(THD *thd, bool init)
+  // XXX what are you trying to do by introducing this 'init' argument?
+  // This function "allocates an array..." and "also check that the
+  // range constants are defined in increasing order".
+  // One could expect init=false to mean that the caller is only
+  // interestd in "check" part, not in "allocate" part. But in the only place
+  // where init=false, you ignore the return value. So, what's the point?
 {
   partition_element* part_def;
   bool first= TRUE;
diff --git a/sql/partition_info.h b/sql/partition_info.h
index 2f36cf18a59..e1e5dfd2ec2 100644
--- a/sql/partition_info.h
+++ b/sql/partition_info.h
@@ -68,9 +68,17 @@ struct Vers_part_info : public Sql_alloc
   }
   my_time_t interval;
   ulonglong limit;
-  partition_element *now_part;
-  partition_element *hist_part;
-  ulonglong stat_serial;
+  partition_element *now_part; // XXX I thought it's always the last partition?
+  partition_element *hist_part; // XXX What if there're many historical partitions?
+  ulonglong stat_serial; // XXX this needs a comment
+  // XXX also, I don't know if that's a good idea here. It works best for
+  // read-often-modify-rarely data. Partitioned tables aren't always (or even
+  // mostly) these kinds of thing.
+  //
+  // XXX by the way, a somewhat unrelated question. InnoDB. Transactions.
+  // One does a DELETE within a transaction. That queries historical
+  // data - before the DELETE but within the same transaction. How
+  // does that work now? And how does it work with partitioning?
 };
 
 class partition_info : public Sql_alloc
@@ -401,6 +409,8 @@ public:
   bool vers_set_limit(ulonglong limit);
   partition_element* vers_part_rotate(THD *thd);
   bool vers_setup_1(THD *thd, uint32 added= 0);
+  // XXX better to avoid default arguments in this case.
+  // it doesn't add many key presses to type ,0 in just one place.
   bool vers_setup_2(THD *thd, bool is_create_table_ind);
   bool vers_scan_min_max(THD *thd, partition_element *part);
   void vers_update_col_vals(THD *thd, partition_element *el0, partition_element *el1);
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index 3201c0e2d33..346d45b8d23 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -5122,6 +5122,7 @@ part_definition:
             }
             p_elem->part_state= PART_NORMAL;
             p_elem->id= part_info->partitions.elements - 1;
+            // XXX why? ^^^
             part_info->curr_part_elem= p_elem;
             part_info->current_partition= p_elem;
             part_info->use_default_partitions= FALSE;
@@ -5550,6 +5551,8 @@ opt_versioning_interval:
            INTERVAL interval;
            if (get_interval_value($2, $3, &interval))
              my_yyabort_error((ER_VERS_WRONG_PARAMS, MYF(0), "BY SYSTEM_TIME", "wrong INTERVAL value"));
+           // XXX I won't longer comment on hard-coded English phrases in
+           // error messages, but still it'd be better to get rid of them
            if (part_info->vers_set_interval(interval))
              MYSQL_YYABORT;
          }
@@ -5935,6 +5938,19 @@ create_table_option:
 	  }
         | WITH_SYSTEM_SYM VERSIONING_SYM
           {
+            /* XXX strangely enough, this violates the standard.
+            the standard says:
+
+            <table definition> ::=
+            CREATE [ <table scope> ] TABLE <table name> <table contents source>
+            [ WITH <system versioning clause> ]
+
+            that is WITH SYSTEM VERSIONING comes *after* SELECT clause.
+            It's really strange, if you ask me. But it's the standard.
+            Please, check in your copy of the standard too. Perhaps
+            what I see in a draft is a bug that was fixed in the final version?
+            */
+
             const char *table_name=
                 Lex->create_last_non_select_table->table_name;
             Vers_parse_info &info= Lex->vers_get_info();
@@ -5943,6 +5959,11 @@ create_table_option:
               my_yyabort_error(
                   (ER_VERS_WRONG_PARAMS, MYF(0), table_name,
                    "Versioning specified more than once for the same table"));
+            /* XXX that's... inconsistent. No other create_table_option
+            has that, so why suddenly here?
+            Not an issue, if you move WITH SYSTEM VERSIONING where the standard
+            wants it, there will be no way to specify it twice, so this
+            check will disappear. */
 
             info.declared_with_system_versioning= true;
             Lex->create_info.options|= HA_VERSIONED_TABLE;
@@ -5957,6 +5978,7 @@ create_table_option:
               my_yyabort_error(
                   (ER_VERS_WRONG_PARAMS, MYF(0), table_name,
                    "Versioning specified more than once for the same table"));
+            // XXX again, inconsistent...
 
             info.declared_without_system_versioning= true;
           }
@@ -6162,6 +6184,7 @@ constraint_def:
 period_for_system_time:
           // If FOR_SYM is followed by SYSTEM_TIME_SYM then they are merged to: FOR_SYSTEM_TIME_SYM .
           PERIOD_SYM FOR_SYSTEM_TIME_SYM '(' period_for_system_time_column_id ',' period_for_system_time_column_id ')'
+          // XXX why you're not using ident here?
           {
             Vers_parse_info &info= Lex->vers_get_info();
             if (!my_strcasecmp(system_charset_info, $4->c_ptr(), $6->c_ptr()))
@@ -6169,6 +6192,10 @@ period_for_system_time:
               my_yyabort_error((ER_VERS_WRONG_PARAMS, MYF(0),
                 Lex->create_last_non_select_table->table_name,
                 "'PERIOD FOR SYSTEM_TIME' columns must be different"));
+            /* XXX why do you need this additional check in the parser?
+            you'll check later that the first column is AS ROW START and the
+            second is AS ROW END, so they'll be different automatically.
+            This check looks completely redundant and artificial */
             }
             info.set_period_for_system_time($4, $6);
           }
@@ -6274,6 +6301,7 @@ field_def:
             Vers_parse_info &info= lex->vers_get_info();
             String *field_name= new (thd->mem_root)
               String((const char*)lex->last_field->field_name, system_charset_info);
+            // XXX again, abusing String where LEX_STRING would do
             if (!field_name)
               MYSQL_YYABORT;
 
@@ -6784,6 +6812,9 @@ serial_attribute:
                   (ER_VERS_WRONG_PARAMS, MYF(0),
                    Lex->create_last_non_select_table->table_name,
                    "Versioning specified more than once for the same field"));
+                   // XXX please, no more checks than with other
+                   // field attributes. We want consistent behavior.
+                   // no other attribute checks whether it's specified twice.
 
             Lex->last_field->versioning = Column_definition::WITH_VERSIONING;
             Lex->create_info.vers_info.has_versioned_fields= true;
@@ -8812,6 +8843,10 @@ opt_query_for_system_time_clause:
           {}
         | QUERY_FOR_SYM SYSTEM_TIME_SYM for_system_time_expr
           {
+            // XXX why Why QUERY FOR and not simply FOR? What does it conflict with?
+            // Why QUERY_FOR_SYM and not QUERY_SYM FOR_SYM? What does it conflict with?
+            // -- ok, QUERY_FOR_SYM was removed in a recent commit,
+            // just keeping my comment for the reference...
             DBUG_ASSERT(Select);
             Select->vers_conditions= Lex->vers_conditions;
           }
@@ -8831,6 +8866,12 @@ opt_for_system_time_clause:
 for_system_time_expr:
           AS OF_SYM trans_or_timestamp simple_expr
           {
+            // XXX standard syntax must work. In the standard there is
+            // no trans_or_timestamp. Our options:
+            // * use TIMESTAMP by default
+            // * use whatever column type the table uses for versioning
+            //   (TIMESTAMP for MyISAM, BIGINT for InnoDB)
+            // * use the result type of the expression
             Lex->vers_conditions.init(FOR_SYSTEM_TIME_AS_OF, $3, $4);
           }
         | AS OF_SYM NOW_SYM
@@ -8849,6 +8890,9 @@ for_system_time_expr:
           TO_SYM trans_or_timestamp simple_expr
           {
             if ($2 != $5)
+            // XXX I simply wouldn't let the user to specify
+            // trans_or_timestamp twice. Just have it once (after FROM)
+            // and, of course, optional, see above about the standard.
             {
               Lex->parse_error(ER_VERS_RANGE_UNITS_MISMATCH);
               MYSQL_YYABORT;
@@ -8859,6 +8903,25 @@ for_system_time_expr:
           FROM simple_expr
           TO_SYM simple_expr
           {
+            // XXX or that one. trans_or_timestamp is only specified once.
+            // but doesn't it look very strange?
+            // SELECT * FROM t1 TIMESTAMP FROM xxx TO yyy
+            //
+            // why did you implement both styles? I'd say
+            // either trans_or_timestamp should be always before the standard
+            // clause:
+            //    opt_trans_or_timestamp AS OF ...
+            //    opt_trans_or_timestamp FROM ... TO ...
+            //    opt_trans_or_timestamp BETWEEN ...AND
+            // or it's inside the standard clause:
+            //    AS OF opt_trans_or_timestamp ...
+            //    FROM opt_trans_or_timestamp ... TO ...
+            //    BETWEEN opt_trans_or_timestamp ...AND ...
+            // but please not both.
+            //    SELECT * FROM t1 FOR SYSTEM_TIME AS OF TIMESTAMP xxx
+            //    SELECT * FROM t1 FOR SYSTEM_TIME TIMESTAMP AS OF xxx
+            // Or another option. Remove trans_or_timestamp at all and always
+            // use type of the column or type of the expression.
             Lex->vers_conditions.init(FOR_SYSTEM_TIME_FROM_TO, $1, $3, $5);
           }
         | BETWEEN_SYM trans_or_timestamp simple_expr
@@ -8881,6 +8944,7 @@ for_system_time_expr:
           trans_or_timestamp
           simple_expr
           {
+          // XXX this is non-standard too. Why is it here?
             Lex->vers_conditions.init(FOR_SYSTEM_TIME_BEFORE, $2, $3);
           }
         ;
@@ -11295,6 +11359,14 @@ table_primary_ident:
           }
           table_ident opt_use_partition opt_table_alias opt_key_definition opt_for_system_time_clause
           {
+            // XXX again, standard.
+            // opt_for_system_time_clause goes before opt_table_alias
+            // other clauses here are non-standard extensions, but
+            // check 7.6 <table reference>, it has
+            //   <table primary> ::=
+            //      <table or query name>
+            //      [ <query system time period specification> ]
+            //      [ <correlation or recognition> ]
             if (!($$= Select->add_table_to_list(thd, $2, $4,
                                                 Select->get_table_join_options(),
                                                 YYPS->m_lock_type,
@@ -16203,6 +16275,7 @@ column_list:
 period_for_system_time_column_id:
           ident
           {
+          // XXX not needed, use ident and LEX_STRING directly
             String *new_str= new (thd->mem_root) String((const char*) $1.str,$1.length,system_charset_info);
             if (new_str == NULL)
               MYSQL_YYABORT;
diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
index b3fc7fa4383..417bc3842ff 100644
--- a/sql/sys_vars.cc
+++ b/sql/sys_vars.cc
@@ -390,6 +390,7 @@ static Sys_var_vers_asof Sys_vers_current_time(
 // applications without modifications". And if you use 0 instead of "NOW",
 // there will be no need for a special class Sys_var_vers_asof.
 // Did you read a comment at the beginning of the file? :)
+// And don't abbrev user visible names
        SESSION_VAR(vers_current_time), CMD_LINE(REQUIRED_ARG, OPT_VERS_CURRENT_TIME),
 // XXX and I wouldn't create a command line option for it. it'll only make
 // --help output longer
diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic
index 27987a8d8b8..4aa9caf028d 100644
--- a/sql/sys_vars.ic
+++ b/sql/sys_vars.ic
@@ -2493,7 +2493,14 @@ public:
 
 
 class Sys_var_vers_asof: public sys_var
-{
+// XXX now I'm really starting to dislike your ... QUERY FOR SYSTEM_TIME
+// extension. A *third* way to do the same thing? It's SQL, not Perl.
+//
+//   SELECT * FROM (SELECT ....) FOR SYSTEM_TIME AS OF xxx
+//   SET STATEMENT vers_current_time=xxx SELECT ...
+//   SELECT ... QUERY FOR SYSTEM_TIME AS OF xxx
+//
+// that's a bit too many alternatives.
 public:
   Sys_var_vers_asof(
     const char *name_arg,
@@ -2539,6 +2546,7 @@ public:
         in.charset(),
         "ALL",
         in.ptr()))
+      // XXX please, use the coding style as in the rest of the server!
     {
       out.type= FOR_SYSTEM_TIME_ALL;
     }