maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei!

On Tue, Jun 8, 2021 at 3:04 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> 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

Done. Probably the name "system_versioning_insert_history" may be
confused with something else. How about
"system_versioning_direct_history" or
"system_versioning_direct_insert"? Even
"system_versioning_modify_history" looks less confusing (and has space
for future extension).

> * FLUSH TABLES requirememnt is rather confusing, but let's have it,
>   at least until I could suggest something better

That was the requirement for force_fields_visible. For
system_versioning_insert_history there is no such requirement (as we
do not modify anything in TABLE_SHARE).

>
> > 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

Edited.

>
> > 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?

That was the commit 00a254cc for MDEV-20186 which sets vers_write in
TABLE::init().

>
> > 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.

I removed force_fields_visible and hence this effect.

>
> > +  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.

Added.

> Also
>
>    insert into t1 set x=5, row_start=..., row_end=...
>
> this should work too.

Added.

> 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.

Ok.

> While
>
>   replace
>   load data ... replace
>
> should work, but as DELETE+INSERT (I mean that DELETE in versioned
> tables doesn't actually delete history).

Sorry, I don't understand the bracket part (why is it here). Normal
REPLACE is DELETE+INSERT, versioned REPLACE is UPDATE+INSERT. For
history replace only "normal REPLACE" is possible, so it is
DELETE+INSERT obviously.

Added REPLACE, but modifying row_end leads to a new row insert. See
this comment:

--echo # Changing row_end via REPLACE is NOT possible, we just insert new row:
# NOTE: because multiple versions of history row with a=1 may exist,
so what REPLACE should change?

> 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?

Looks like a needless artifact. Removed.

>
> >  -- 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?

Looks like a needless artifact. Removed.

>
> > 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).

Renamed.

>
> > +{
> > +  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

I set thd->lex->sql_command for check_update_fields() so it is
possible now to distinguish fix_fields() of INSERT from fix_fields()
of ODKU. I used the value of SQLCOM_UPDATE though it is probably good
to add something like SQLCOM_DUPKEY_UPDATE. What do you think?
Probably there would be no need in `bool update` for fill_record() if
we have different sql_command for that.

While I did that I noticed different error codes for implicit and
explicit row_start/row_end which I don't like:

--error ER_BAD_FIELD_ERROR
insert t1 (x) values (2) on duplicate key update x= 3, row_end=
'1970-01-01 00:00:00';
--error ER_WARNING_NON_DEFAULT_VALUE_FOR_GENERATED_COLUMN
insert t2 (y) values (1) on duplicate key update y= 3, row_end=
'1970-01-01 00:00:00';

That is the result of the pseudocolumn concept.

>
> > +  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

Renamed to vers_insert_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.

Probably not. This will be deprecated by MDEV-16226. I disabled direct
insertion for VERS_TRX_ID.

>
> >            {
> >              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.

There are tests for REPLACE now.

>
> >              {
> >                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"

Updated.

>
> > +       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



--
All the best,

Aleksey Midenkov
@midenok