maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

On Jun 08, Aleksey Midenkov wrote:
> revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287)
> parent(s): 27d66d644cf
> author: Aleksey Midenkov <midenok@xxxxxxxxx>
> committer: Aleksey Midenkov <midenok@xxxxxxxxx>
> timestamp: 2021-03-01 17:43:34 +0300
> message:
> 
> MDEV-16546 System versioning setting to allow history modification
> 
> 1. force_fields_visible session variable makes system-invisible and
> user-invisible fields visible on next table open. FLUSH TABLES
> required for already opened tables.

* system_versioning_insert_history
* allows pseudocolumns ROW_START and ROW_END be specified in INSERT
* doesn't affect invisible columns
* FLUSH TABLES requirememnt is rather confusing, but let's have it,
  at least until I could suggest something better

> 2. If secure_timestamp allows to modify timestamp variable then
> following DML commands: INSERT, INSERT..SELECT and LOAD DATA can
> specify row_start and row_end system field values.

when system_versioning_insert_history=1

> 3. Cleaned up select_insert::send_data() from setting vers_write as
> this parameter is now set on TABLE initialization.

I don't see where it's set. This commit removes two

  table->vers_write= table->versioned();

and adds one

  from_field->table->vers_write= false;

in Item_field::fix_fields. But where is vers_write set to true?
Was it done in some other commit?

> diff --git a/mysql-test/suite/versioning/r/insert.result b/mysql-test/suite/versioning/r/insert.result
> index 2645d0184e8..00d1bb705c4 100644
> --- a/mysql-test/suite/versioning/r/insert.result
> +++ b/mysql-test/suite/versioning/r/insert.result
> @@ -112,3 +112,95 @@ x	y
>  9	9001
>  drop table t1;
>  drop table t2;
> +#
> +# MDEV-16546 System versioning setting to allow history modification
> +#
> +# restart: --secure-timestamp=NO
> +set @@session.time_zone='+00:00';
> +create table t1(x int) with system versioning;
> +insert into t1(x) values (1);
> +insert into t1(x, row_start, row_end) values (2, '1980-01-01 00:00:00', '1980-01-01 00:00:01');
> +ERROR 42S22: Unknown column 'row_start' in 'field list'
> +set session force_fields_visible= 1;
> +flush tables;
> +show create table t1;
> +Table	Create Table
> +t1	CREATE TABLE `t1` (
> +  `x` int(11) DEFAULT NULL,
> +  `row_start` timestamp(6) GENERATED ALWAYS AS ROW START,
> +  `row_end` timestamp(6) GENERATED ALWAYS AS ROW END,

shouldn't be visible here, only in insert and load.

> +  PERIOD FOR SYSTEM_TIME (`row_start`, `row_end`)
> +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING
> +insert into t1(x, row_start, row_end) values (3, '1980-01-01 00:00:00', '1980-01-01 00:00:01');

please, add tests for

   insert into t1 values (4)

this should work, row_start/row_end must be mentioned explicitly.
Also

   insert into t1 set x=5, row_start=..., row_end=...

this should work too.
And when only one row_start or row_end is mentioned as well.
But

  update t1 set row_start=...
  insert into t1 (x, row_start, row_end) values (...)
     on duplicate key update row_start=...

this should fail.
While

  replace
  load data ... replace

should work, but as DELETE+INSERT (I mean that DELETE in versioned
tables doesn't actually delete history). If that's too difficult, then
an error too.

> +update t1 set x= x + 1;
> +select x, check_row_ts(row_start, row_end) from t1 for system_time all order by x;
...
> diff --git a/mysql-test/suite/versioning/t/insert.opt b/mysql-test/suite/versioning/t/insert.opt
> new file mode 100644
> index 00000000000..a74d68957ef
> --- /dev/null
> +++ b/mysql-test/suite/versioning/t/insert.opt
> @@ -0,0 +1 @@
> +--secure-timestamp=yes

why is this?

>  -- source suite/versioning/common_finish.inc
> diff --git a/mysql-test/suite/versioning/t/insert2.opt b/mysql-test/suite/versioning/t/insert2.opt
> new file mode 100644
> index 00000000000..a74d68957ef
> --- /dev/null
> +++ b/mysql-test/suite/versioning/t/insert2.opt
> @@ -0,0 +1 @@
> +--secure-timestamp=yes

and this?

> diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> index d23b190b2c5..93f8a8434e4 100644
> --- a/sql/sql_class.cc
> +++ b/sql/sql_class.cc
> @@ -7604,6 +7604,20 @@ void THD::set_last_commit_gtid(rpl_gtid &gtid)
>  #endif
>  }
>  
> +bool THD::vers_modify_sys_field() const

don't call it vers_modify_sys_field, your previous name
vers_modify_history was better. There may be other sys fields that
will never be directly writable (like ROWID, for example).

> +{
> +  if (lex->sql_command != SQLCOM_INSERT &&
> +      lex->sql_command != SQLCOM_INSERT_SELECT &&
> +      lex->sql_command != SQLCOM_LOAD)
> +    return false;

I don't think this is enough to reject

  insert ... on duplicate key update row_start=xxx

> +  if (opt_secure_timestamp >= SECTIME_REPL ||

this is fixed in a later commit, so ok

> +      (opt_secure_timestamp == SECTIME_SUPER &&
> +       !(security_ctx->master_access & SUPER_ACL)))
> +    return false;
> +  return true;
> +}
> +
> +
>  void
>  wait_for_commit::reinit()
>  {
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index 2913a4a8350..520ea1d63bf 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -820,6 +820,7 @@ typedef struct system_variables
>  
>    vers_asof_timestamp_t vers_asof_timestamp;
>    ulong vers_alter_history;
> +  my_bool force_fields_visible;

system_versioning_insert_history

>  } SV;
>  
>  /**
> @@ -5400,6 +5401,7 @@ class THD: public THD_count, /* this must be first */
>    Item *sp_prepare_func_item(Item **it_addr, uint cols= 1);
>    bool sp_eval_expr(Field *result_field, Item **expr_item_ptr);
>  
> +  bool vers_modify_sys_field() const;

vers_modify_history

>  };
>  
>  
> diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
> index f0b84ab126f..84c2ae4ab36 100644
> --- a/sql/sql_insert.cc
> +++ b/sql/sql_insert.cc
> @@ -2014,7 +2014,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink)
>              !table->file->referenced_by_foreign_key() &&
>              (!table->triggers || !table->triggers->has_delete_triggers()))
>          {
> -          if (table->versioned(VERS_TRX_ID))
> +          if (table->versioned(VERS_TRX_ID) && table->vers_write)

does it even make sense? I mean direct insertion of transaction ids.

>            {
>              bitmap_set_bit(table->write_set, table->vers_start_field()->field_index);
>              table->file->column_bitmaps_signal();
> @@ -2029,7 +2029,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink)
>              info->deleted++;
>              if (!table->file->has_transactions())
>                thd->transaction->stmt.modified_non_trans_table= TRUE;
> -            if (table->versioned(VERS_TIMESTAMP))
> +            if (table->versioned(VERS_TIMESTAMP) && table->vers_write)

This (and the previous hunk) are in the DUP_REPLACE branch.
There should be tests for these changes.

>              {
>                store_record(table, record[2]);
>                error= vers_insert_history_row(table);
> diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
> index f817f9086a9..4429308a097 100644
> --- a/sql/sys_vars.cc
> +++ b/sql/sys_vars.cc
> @@ -6737,3 +6737,7 @@ static Sys_var_ulonglong Sys_max_rowid_filter_size(
>         SESSION_VAR(max_rowid_filter_size), CMD_LINE(REQUIRED_ARG),
>         VALID_RANGE(1024, (ulonglong)~(intptr)0), DEFAULT(128*1024),
>         BLOCK_SIZE(1));
> +
> +static Sys_var_mybool Sys_force_fields_visible(
> +       "force_fields_visible", "Make invisible fields visible on next table open",

"system_versioning_insert_history"
"Allows direct inserts into ROW_START and ROW_END columns"

> +       SESSION_VAR(force_fields_visible), CMD_LINE(OPT_ARG), DEFAULT(FALSE));
> diff --git a/sql/table.cc b/sql/table.cc
> index f4bdbdeac5a..520f3738c9e 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -4024,6 +4024,9 @@ enum open_frm_error open_table_from_share(THD *thd, TABLE_SHARE *share,
>    {
>      if (!((*field_ptr)= share->field[i]->clone(&outparam->mem_root, outparam)))
>        goto err;
> +    if (thd->variables.force_fields_visible &&
> +        (*field_ptr)->invisible <= INVISIBLE_SYSTEM)
> +      (*field_ptr)->invisible= VISIBLE;

Not really, system_versioning_insert_history should not affect
visibility of anything besides ROW_START and ROW_END pseudocolumns
in the INSERT and LOAD DATA statements.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx