Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backport: support unsigned/zerofill #1716

Closed
4 tasks done
Tracked by #1661
hustjieke opened this issue May 6, 2023 · 3 comments
Closed
4 tasks done
Tracked by #1661

backport: support unsigned/zerofill #1716

hustjieke opened this issue May 6, 2023 · 3 comments
Assignees
Labels
A-porting back port code from 5.7/8.0 C-stonedb-8.0 associated with stonedb 8.0

Comments

@hustjieke
Copy link
Collaborator

hustjieke commented May 6, 2023

Is your feature request related to a problem? Please describe.

working issue: #1013, ##1218

Describe the solution you'd like

Describe alternatives you've considered

Additional context

@hustjieke hustjieke added C-stonedb-8.0 associated with stonedb 8.0 A-porting back port code from 5.7/8.0 labels May 6, 2023
@hustjieke hustjieke added this to the StoneDB_8.0_v1.0.2 milestone May 6, 2023
@hustjieke hustjieke moved this to In Progress in StoneDB for MySQL 8.0 May 19, 2023
@hustjieke hustjieke self-assigned this May 19, 2023
@hustjieke
Copy link
Collaborator Author

assigned me!

@hustjieke
Copy link
Collaborator Author

crash happend when insert data out of range:

mysql> create table tiny(a int, b tinyint);

mysql> insert into tiny values(0,128);
ERROR 2013 (HY000): Lost connection to MySQL server during query
No connection. Trying to reconnect...
ERROR 2002 (HY000): Can't connect to local MySQL server through socket '/github/stonedb/build/install8/tmp/mysql.sock' (111)
ERROR: 
Can't connect to the server

@hustjieke
Copy link
Collaborator Author

crash happend when insert data out of range:

mysql> create table tiny(a int, b tinyint);

mysql> insert into tiny values(0,128);
ERROR 2013 (HY000): Lost connection to MySQL server during query
No connection. Trying to reconnect...
ERROR 2002 (HY000): Can't connect to local MySQL server through socket '/github/stonedb/build/install8/tmp/mysql.sock' (111)
ERROR: 
Can't connect to the server

Root cause:
In non-strict sql_mode, insert sucess with warning.
in strict sql_mode, crashed in function Sql_cmd_insert_values::execute_inner(THD *thd) in file sql/sql_insert.cc:

      if (write_record(thd, insert_table, &info, &update)) {
        has_error = true;
        break;
      }
      thd->get_stmt_da()->inc_current_row_for_condition();
    }
  }  // Statement plan is available within these braces

  assert(has_error == thd->get_stmt_da()->is_error());

here has_error = false, thd->get_stmt_da()->is_error() = true.

When insert data out of range, tianmu engine push a warning into mysql, but in strict sql_mode, the
Sql_condition::SL_WARNING will be changed to Sql_condition::SL_ERROR in function:

/**
  Implementation of STRICT mode.
  Upgrades a set of given conditions from warning to error.
*/
bool Strict_error_handler::handle_condition(
...
...
switch (sql_errno) {
    case ER_TRUNCATED_WRONG_VALUE:
    case ER_WRONG_VALUE_FOR_TYPE:
    case ER_WARN_DATA_OUT_OF_RANGE:
    case ER_WARN_DATA_OUT_OF_RANGE_FUNCTIONAL_INDEX:
    case ER_DIVISION_BY_ZERO:
    case ER_TRUNCATED_WRONG_VALUE_FOR_FIELD:
    case WARN_DATA_TRUNCATED:
    case ER_WARN_DATA_TRUNCATED_FUNCTIONAL_INDEX:
    case ER_DATA_TOO_LONG:
    case ER_BAD_NULL_ERROR:
    case ER_NO_DEFAULT_FOR_FIELD:
    case ER_TOO_LONG_KEY:
    case ER_NO_DEFAULT_FOR_VIEW_FIELD:
    case ER_WARN_NULL_TO_NOTNULL:
    case ER_CUT_VALUE_GROUP_CONCAT:
    case ER_DATETIME_FUNCTION_OVERFLOW:
    case ER_WARN_TOO_FEW_RECORDS:
    case ER_WARN_TOO_MANY_RECORDS:
    case ER_INVALID_ARGUMENT_FOR_LOGARITHM:
    case ER_NUMERIC_JSON_VALUE_OUT_OF_RANGE:
    case ER_INVALID_JSON_VALUE_FOR_CAST:
    case ER_WARN_ALLOWED_PACKET_OVERFLOWED:
      if ((*level == Sql_condition::SL_WARNING) &&
          (!thd->get_transaction()->cannot_safely_rollback(
               Transaction_ctx::STMT) ||
           (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES))) {
        (*level) = Sql_condition::SL_ERROR;
      }
      break;
    default:
      break;
  }
  return false;

So, write_record(thd, insert_table, &info, &update) should return error and then set has_error = true;, after debug, I found ret value not set 1 when execption get in function:

int Engine::InsertRow(const std::string &table_path, [[maybe_unused]] Transaction *trans_, TABLE *table,
                      std::shared_ptr<TableShare> &share) {
  int ret = 0; 
  try {
    if (tianmu_sysvar_insert_delayed && table->s->tmp_table == NO_TMP_TABLE) {
      if (tianmu_sysvar_enable_rowstore) {
        InsertMemRow(table_path, share, table);
      } else {
        InsertDelayed(table_path, share->TabID(), table);
      }    
      tianmu_stat.delayinsert++;
    } else {
      current_txn_->SetLoadSource(common::LoadSource::LS_Direct);
      auto rct = current_txn_->GetTableByPath(table_path);
      ret = rct->Insert(table);
    }    
    return ret; 
  } catch (common::Exception &e) {
    TIANMU_LOG(LogCtl_Level::ERROR, "delayed inserting failed. %s %s", e.what(), e.trace().c_str());
  } catch (std::exception &e) {
    TIANMU_LOG(LogCtl_Level::ERROR, "delayed inserting failed. %s", e.what());
    ret = 1; 
  } catch (...) {
    TIANMU_LOG(LogCtl_Level::ERROR, "delayed inserting failed.");
  }

  if (tianmu_sysvar_insert_delayed) {
    tianmu_stat.failed_delayinsert++;
    ret = 1; 
  }

  return ret; 
}

we should set ret = 1 in strict sql_mode.

hustjieke added a commit to hustjieke/stonedb-8.0.30-upgrade that referenced this issue May 23, 2023
[summary]
1. delete unsigned/zerofill limit.
2. zerofill is deprecated and will be removed in a future release, Use the LPAD function to zero-pad numbers, or store the formatted numbers in a CHAR column.
hustjieke added a commit to hustjieke/stonedb-8.0.30-upgrade that referenced this issue May 23, 2023
hustjieke added a commit to hustjieke/stonedb-8.0.30-upgrade that referenced this issue May 23, 2023
[summary]
In non-strict sql_mode, insert sucess with warning.
in strict sql_mode, crashed in function `Sql_cmd_insert_values::execute_inner(THD *thd)` in file `sql/sql_insert.cc`:
```
      if (write_record(thd, insert_table, &info, &update)) {
        has_error = true;
        break;
      }
      thd->get_stmt_da()->inc_current_row_for_condition();
    }
  }  // Statement plan is available within these braces

  assert(has_error == thd->get_stmt_da()->is_error());
```

here `has_error` = false, `thd->get_stmt_da()->is_error()` = true, that
caused crashed.

When insert data out of range, `tianmu` engine push a warning into mysql,  but in strict sql_mode, the
`Sql_condition::SL_WARNING` will be changed to `Sql_condition::SL_ERROR` in function:

```
/**
  Implementation of STRICT mode.
  Upgrades a set of given conditions from warning to error.
*/
bool Strict_error_handler::handle_condition(
...
...
switch (sql_errno) {
    ...
    case ER_WARN_DATA_OUT_OF_RANGE:
    case ER_WARN_DATA_OUT_OF_RANGE_FUNCTIONAL_INDEX:
      if ((*level == Sql_condition::SL_WARNING) &&
          (!thd->get_transaction()->cannot_safely_rollback(
               Transaction_ctx::STMT) ||
           (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES))) {
        (*level) = Sql_condition::SL_ERROR;
      }
      break;
    default:
      break;
  }
  return false;
```

So, `write_record(thd, insert_table, &info, &update)` should return error and then set ` has_error = true;`, after debug, I found `ret` value not set `1` when execption get in function:

```
int Engine::InsertRow(const std::string &table_path, [[maybe_unused]] Transaction *trans_, TABLE *table,
                      std::shared_ptr<TableShare> &share) {
  int ret = 0;
  try {
    ret = rct->Insert(table);
    return ret;
  } catch (...) {
    TIANMU_LOG(LogCtl_Level::ERROR, "delayed inserting failed.");
  }

  return ret;
}
```

we should set `ret` = 1 in strict sql_mode in catch block.
mergify bot pushed a commit that referenced this issue May 24, 2023
[summary]
1. delete unsigned/zerofill limit.
2. zerofill is deprecated and will be removed in a future release, Use the LPAD function to zero-pad numbers, or store the formatted numbers in a CHAR column.
mergify bot pushed a commit that referenced this issue May 24, 2023
mergify bot pushed a commit that referenced this issue May 24, 2023
[summary]
In non-strict sql_mode, insert sucess with warning.
in strict sql_mode, crashed in function `Sql_cmd_insert_values::execute_inner(THD *thd)` in file `sql/sql_insert.cc`:
```
      if (write_record(thd, insert_table, &info, &update)) {
        has_error = true;
        break;
      }
      thd->get_stmt_da()->inc_current_row_for_condition();
    }
  }  // Statement plan is available within these braces

  assert(has_error == thd->get_stmt_da()->is_error());
```

here `has_error` = false, `thd->get_stmt_da()->is_error()` = true, that
caused crashed.

When insert data out of range, `tianmu` engine push a warning into mysql,  but in strict sql_mode, the
`Sql_condition::SL_WARNING` will be changed to `Sql_condition::SL_ERROR` in function:

```
/**
  Implementation of STRICT mode.
  Upgrades a set of given conditions from warning to error.
*/
bool Strict_error_handler::handle_condition(
...
...
switch (sql_errno) {
    ...
    case ER_WARN_DATA_OUT_OF_RANGE:
    case ER_WARN_DATA_OUT_OF_RANGE_FUNCTIONAL_INDEX:
      if ((*level == Sql_condition::SL_WARNING) &&
          (!thd->get_transaction()->cannot_safely_rollback(
               Transaction_ctx::STMT) ||
           (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES))) {
        (*level) = Sql_condition::SL_ERROR;
      }
      break;
    default:
      break;
  }
  return false;
```

So, `write_record(thd, insert_table, &info, &update)` should return error and then set ` has_error = true;`, after debug, I found `ret` value not set `1` when execption get in function:

```
int Engine::InsertRow(const std::string &table_path, [[maybe_unused]] Transaction *trans_, TABLE *table,
                      std::shared_ptr<TableShare> &share) {
  int ret = 0;
  try {
    ret = rct->Insert(table);
    return ret;
  } catch (...) {
    TIANMU_LOG(LogCtl_Level::ERROR, "delayed inserting failed.");
  }

  return ret;
}
```

we should set `ret` = 1 in strict sql_mode in catch block.
@github-project-automation github-project-automation bot moved this from In Progress to Done in StoneDB for MySQL 8.0 May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-porting back port code from 5.7/8.0 C-stonedb-8.0 associated with stonedb 8.0
Projects
Development

No branches or pull requests

1 participant