diff options
| -rw-r--r-- | sql/ha_partition.cc | 1 | ||||
| -rw-r--r-- | sql/log_event.cc | 8 | ||||
| -rw-r--r-- | sql/mysqld.h | 7 | ||||
| -rw-r--r-- | sql/partition_element.h | 21 | ||||
| -rw-r--r-- | sql/partition_info.cc | 11 | ||||
| -rw-r--r-- | sql/partition_info.h | 16 | ||||
| -rw-r--r-- | sql/sql_yacc.yy | 73 | ||||
| -rw-r--r-- | sql/sys_vars.cc | 1 | ||||
| -rw-r--r-- | sql/sys_vars.ic | 10 |
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; } |