Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi, Aleksey!
On Jun 08, Aleksey Midenkov wrote:
> revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287)
> parent(s): 27d66d644cf
> author: Aleksey Midenkov
> committer: Aleksey Midenkov
> 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 @@ xy
> 99001
> 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;
> +TableCreate 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 000..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 000..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 >id)
> #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()
> {
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi Sergei! On Tue, Jun 8, 2021 at 3:04 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Jun 08, Aleksey Midenkov wrote: > > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > > parent(s): 27d66d644cf > > author: Aleksey Midenkov > > committer: Aleksey Midenkov > > 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 @@ xy > > 99001 > > 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; > > +TableCreate 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 000..a74d68957ef > > --- /dev/null > > +++ b/mysql-test/suite/versioning/t/insert.opt > > @@ -0,
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi, Aleksey!
On Jun 28, Aleksey Midenkov wrote:
> >
> > * 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).
system_versioning_direct_history looks strange, I wouldn't understand
what it means if I hadn't known it already.
system_versioning_direct_insert is kind of ok, "allow_insert" might've
been clearer.
system_versioning_modify_history looks reasonable too.
But I think we're reaching the point of diminishing return.
There were a bunch of names that were clearly bad, but the difference
between good ones is rather minimal. I don't think I can say "this one
is much better than all others".
So, system_versioning_insert_history, system_versioning_direct_insert,
system_versioning_allow_insert, system_versioning_modify_history - I'd
say they all can do. If you have a strong preference - use the name you
prefer.
> > 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.
I meant that DELETE part in REPLACE should update row_end, not actually
delete a versioned row. Now sure why I wrote that, seems kind of
obvious.
> 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?
Yes, right. We want mysqldump to be used to save/restore history.
So adding new rows with arbitrary start/end should be allowed.
But *modifying* existing history rows shouldn't.
> > > 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
> > > +{
> > > + 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.
Hmm. I need to see it in the context, will comment on the new patch.
> --
> All the best,
> Aleksey Midenkov
> @midenok
Thanks, will do the next review asap!
(but likely after some maintenance bugfixing)
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi, Aleksey,
I've took a look at the whole diff bad1440325ba 9af2883e2692.
(but without MDEV-16029)
I have a couple of comments about minor issues, and a suggestion,
please, see below. Nothing big, really.
> diff --git a/include/my_base.h b/include/my_base.h
> index 053bf3fbb69..f1c0d8dbc99 100644
> --- a/include/my_base.h
> +++ b/include/my_base.h
> @@ -527,7 +527,8 @@ enum ha_base_keytype {
> #define HA_ERR_TABLESPACE_MISSING 194 /* Missing Tablespace */
> #define HA_ERR_SEQUENCE_INVALID_DATA 195
> #define HA_ERR_SEQUENCE_RUN_OUT 196
> -#define HA_ERR_LAST 196 /* Copy of last error nr * */
> +#define HA_ERR_WRONG_ROW_TIMESTAMP 197 /* System Versioning row_end <=
> row_start */
> +#define HA_ERR_LAST 197 /* Copy of last error nr * */
As you like. But I'd _suggest_ not to introduce a new HA_ERR_ code for this.
You only use it in sql/, it's not something that a storage engine
will use, as far as I understand. So there is no need to pollute the API,
I would say.
>
> /* Number of different errors */
> #define HA_ERR_ERRORS(HA_ERR_LAST - HA_ERR_FIRST + 1)
> diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result
> b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result
> index b28f3c567ff..6c09e0a7f23 100644
> --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result
> +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result
> @@ -982,6 +982,16 @@ NUMERIC_BLOCK_SIZE 1
> ENUM_VALUE_LIST NULL
> READ_ONLYNO
> COMMAND_LINE_ARGUMENTREQUIRED
> +VARIABLE_NAMEFORCE_FIELDS_VISIBLE
> +VARIABLE_SCOPE SESSION
> +VARIABLE_TYPEBOOLEAN
> +VARIABLE_COMMENT Make invisible fields visible on next table open
> +NUMERIC_MIN_VALUENULL
> +NUMERIC_MAX_VALUENULL
> +NUMERIC_BLOCK_SIZE NULL
> +ENUM_VALUE_LIST OFF,ON
> +READ_ONLYNO
> +COMMAND_LINE_ARGUMENTOPTIONAL
apparently, forgot to update embedded result after renaming the variable
> VARIABLE_NAMEFOREIGN_KEY_CHECKS
> VARIABLE_SCOPE SESSION
> VARIABLE_TYPEBOOLEAN
> diff --git a/sql/log_event.h b/sql/log_event.h
> index 3adc7a26d93..dc269955c5f 100644
> --- a/sql/log_event.h
> +++ b/sql/log_event.h
> @@ -535,16 +535,12 @@ class String;
> */
> #define OPTIONS_WRITTEN_TO_BIN_LOG \
>(OPTION_AUTO_IS_NULL | OPTION_NO_FOREIGN_KEY_CHECKS | \
> - OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | OPTION_IF_EXISTS)
> + OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | OPTION_IF_EXISTS |
> \
> + OPTION_INSERT_HISTORY)
>
> -/* Shouldn't be defined before */
> -#define EXPECTED_OPTIONS \
> - ((1ULL << 14) | (1ULL << 26) | (1ULL << 27) | (1ULL << 19) | (1ULL << 28))
> -
> -#if OPTIONS_WRITTEN_TO_BIN_LOG != EXPECTED_OPTIONS
> -#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT change their values!
> +#if OPTIONS_WRITTEN_TO_BIN_LOG >= (1ULL << 32)
> +#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT exceed 32 bits!
Well, no. This check makes sure that OPTION_RELAXED_UNIQUE_CHECKS
is 1<<27, that OPTION_AUTO_IS_NULL is 1<<14, etc.
For example, you've changed OPTION_SETUP_TABLES_DONE from bit 30 to 44.
The EXPECTED_OPTIONS check was preventing somebody inadvertenly moving
one of OPTIONS_WRITTEN_TO_BIN_LOG flags to a different bit.
You can refactor it, if you don't like it.
But please keep it in in some form.
> #endif
> -#undef EXPECTED_OPTIONS /* You shouldn't use this one */
>
> #define CHECKSUM_CRC32_SIGNATURE_LEN 4
> /**
> diff --git a/sql/handler.cc b/sql/handler.cc
> index 393f6234653..4bbb40abb5b 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -7493,6 +7496,26 @@ int handler::ha_write_row(const uchar *buf)
>DBUG_RETURN(error);
>}
>
> + if (table->versioned() && !table->vers_write)
> + {
> +Field *row_start= table->vers_start_field();
> +Field *row_end= table->vers_end_field();
> +MYSQL_TIME ltime;
> +
> +bitmap_set_bit(table->read_set, row_start->field_index);
> +bitmap_set_bit(table->read_set, row_end->field_index);
> +
> +/*
> + Inserting the history row directly, check ROW_START <= ROW_END and
> + ROW_START is non-zero.
> +*/
> +if (!row_end->is_max() && (
> + (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) ||
> + row_start->get_date( +TIME_NO_ZERO_DATE,
> time_round_mode_t(time_round_mode_t::FRAC_NONE)
I don't quite understand this condition.
checking for row_end->is_max() is redundant, because row_start->cmp()
on itself is sufficient. So, I suppose you've done it as an optimization?
to avoid expensive cmp()?
But in that case you also allow zero date in the row_start, and this looks
like a bug.
I'd suggest to simplify the code and to remove is_max check.
Unless you've run benchmarks and it really helps. And unless
I've misunderstood its purpose.
> + DBUG_RETURN(HA_ERR_WRONG_ROW_TIMESTAMP);
> + }
> +
>MYSQL_INSERT_ROW_
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi Sergei,
On Tue, Feb 15, 2022 at 4:35 PM Sergei Golubchik wrote:
>
> Hi, Aleksey,
>
> I've took a look at the whole diff bad1440325ba 9af2883e2692.
> (but without MDEV-16029)
>
> I have a couple of comments about minor issues, and a suggestion,
> please, see below. Nothing big, really.
>
> > diff --git a/include/my_base.h b/include/my_base.h
> > index 053bf3fbb69..f1c0d8dbc99 100644
> > --- a/include/my_base.h
> > +++ b/include/my_base.h
> > @@ -527,7 +527,8 @@ enum ha_base_keytype {
> > #define HA_ERR_TABLESPACE_MISSING 194 /* Missing Tablespace */
> > #define HA_ERR_SEQUENCE_INVALID_DATA 195
> > #define HA_ERR_SEQUENCE_RUN_OUT 196
> > -#define HA_ERR_LAST 196 /* Copy of last error nr * */
> > +#define HA_ERR_WRONG_ROW_TIMESTAMP 197 /* System Versioning row_end
> > <= row_start */
> > +#define HA_ERR_LAST 197 /* Copy of last error nr * */
>
> As you like. But I'd _suggest_ not to introduce a new HA_ERR_ code for this.
>
> You only use it in sql/, it's not something that a storage engine
> will use, as far as I understand. So there is no need to pollute the API,
> I would say.
Replaced with ER_WRONG_VALUE.
>
> >
> > /* Number of different errors */
> > #define HA_ERR_ERRORS(HA_ERR_LAST - HA_ERR_FIRST + 1)
> > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result
> > b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result
> > index b28f3c567ff..6c09e0a7f23 100644
> > --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result
> > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result
> > @@ -982,6 +982,16 @@ NUMERIC_BLOCK_SIZE 1
> > ENUM_VALUE_LIST NULL
> > READ_ONLYNO
> > COMMAND_LINE_ARGUMENTREQUIRED
> > +VARIABLE_NAMEFORCE_FIELDS_VISIBLE
> > +VARIABLE_SCOPE SESSION
> > +VARIABLE_TYPEBOOLEAN
> > +VARIABLE_COMMENT Make invisible fields visible on next table open
> > +NUMERIC_MIN_VALUENULL
> > +NUMERIC_MAX_VALUENULL
> > +NUMERIC_BLOCK_SIZE NULL
> > +ENUM_VALUE_LIST OFF,ON
> > +READ_ONLYNO
> > +COMMAND_LINE_ARGUMENTOPTIONAL
>
> apparently, forgot to update embedded result after renaming the variable
Fixed.
>
> > VARIABLE_NAMEFOREIGN_KEY_CHECKS
> > VARIABLE_SCOPE SESSION
> > VARIABLE_TYPEBOOLEAN
> > diff --git a/sql/log_event.h b/sql/log_event.h
> > index 3adc7a26d93..dc269955c5f 100644
> > --- a/sql/log_event.h
> > +++ b/sql/log_event.h
> > @@ -535,16 +535,12 @@ class String;
> > */
> > #define OPTIONS_WRITTEN_TO_BIN_LOG \
> >(OPTION_AUTO_IS_NULL | OPTION_NO_FOREIGN_KEY_CHECKS | \
> > - OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | OPTION_IF_EXISTS)
> > + OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | OPTION_IF_EXISTS
> > | \
> > + OPTION_INSERT_HISTORY)
> >
> > -/* Shouldn't be defined before */
> > -#define EXPECTED_OPTIONS \
> > - ((1ULL << 14) | (1ULL << 26) | (1ULL << 27) | (1ULL << 19) | (1ULL <<
> > 28))
> > -
> > -#if OPTIONS_WRITTEN_TO_BIN_LOG != EXPECTED_OPTIONS
> > -#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT change their values!
> > +#if OPTIONS_WRITTEN_TO_BIN_LOG >= (1ULL << 32)
> > +#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT exceed 32 bits!
>
> Well, no. This check makes sure that OPTION_RELAXED_UNIQUE_CHECKS
> is 1<<27, that OPTION_AUTO_IS_NULL is 1<<14, etc.
Reverted.
>
> For example, you've changed OPTION_SETUP_TABLES_DONE from bit 30 to 44.
> The EXPECTED_OPTIONS check was preventing somebody inadvertenly moving
> one of OPTIONS_WRITTEN_TO_BIN_LOG flags to a different bit.
And that helps to keep consistency between different versions of
master and slave, doesn't it? The above doesn't protect from the
swapped values between two flags.
>
> You can refactor it, if you don't like it.
> But please keep it in in some form.
>
> > #endif
> > -#undef EXPECTED_OPTIONS /* You shouldn't use this one */
> >
> > #define CHECKSUM_CRC32_SIGNATURE_LEN 4
> > /**
> > diff --git a/sql/handler.cc b/sql/handler.cc
> > index 393f6234653..4bbb40abb5b 100644
> > --- a/sql/handler.cc
> > +++ b/sql/handler.cc
> > @@ -7493,6 +7496,26 @@ int handler::ha_write_row(const uchar *buf)
> >DBUG_RETURN(error);
> >}
> >
> > + if (table->versioned() && !table->vers_write)
> > + {
> > +Field *row_start= table->vers_start_field();
> > +Field *row_end= table->vers_end_field();
> > +MYSQL_TIME ltime;
> > +
> > +bitmap_set_bit(table->read_set, row_start->field_index);
> > +bitmap_set_bit(table->read_set, row_end->field_index);
> > +
> > +/*
> > + Inserting the history row directly, check ROW_START <= ROW_END and
> > + ROW_START is non-zero.
> > +*/
> > +if (!row_end->is_max() && (
> > + (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) ||
> > + row_start->get_date( > +TIME_NO_ZERO_DATE,
> > time_round_mode_t(time_round_mode_t::FRAC_NONE)
>
> I don't quite understa
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi, Aleksey,
On Aug 03, Aleksey Midenkov wrote:
> > > diff --git a/sql/log_event.h b/sql/log_event.h
> > > index 3adc7a26d93..dc269955c5f 100644
> > > --- a/sql/log_event.h
> > > +++ b/sql/log_event.h
> > > @@ -535,16 +535,12 @@ class String;
> > > */
> > > #define OPTIONS_WRITTEN_TO_BIN_LOG \
> > >(OPTION_AUTO_IS_NULL | OPTION_NO_FOREIGN_KEY_CHECKS | \
> > > - OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT |
> > > OPTION_IF_EXISTS)
> > > + OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT |
> > > OPTION_IF_EXISTS | \
> > > + OPTION_INSERT_HISTORY)
> > >
> > > -/* Shouldn't be defined before */
> > > -#define EXPECTED_OPTIONS \
> > > - ((1ULL << 14) | (1ULL << 26) | (1ULL << 27) | (1ULL << 19) | (1ULL <<
> > > 28))
> > > -
> > > -#if OPTIONS_WRITTEN_TO_BIN_LOG != EXPECTED_OPTIONS
> > > -#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT change their values!
> > > +#if OPTIONS_WRITTEN_TO_BIN_LOG >= (1ULL << 32)
> > > +#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT exceed 32 bits!
> >
> > Well, no. This check makes sure that OPTION_RELAXED_UNIQUE_CHECKS
> > is 1<<27, that OPTION_AUTO_IS_NULL is 1<<14, etc.
>
> Reverted.
I'm sorry, but that my comment above is now obsolete.
For explicit_defaults_for_timestamp feature I've removed this whole
EXPECTED_OPTIONS define. Its purpose was to ensure that the set of
"expected options" never changes, it's hard-coded for replication to
work. Obviously, assuming it'll never change was a bit... optimistic.
Monty already changed it by adding OPTION_IF_EXISTS, you changed it for
OPTION_INSERT_HISTORY, and so did I for explicit_defaults_for_timestamp.
So now there is no EXPECTED_OPTIONS define, the set of
OPTIONS_WRITTEN_TO_BIN_LOG can grow. You can add new flags to it, but
you need to record the version when you did it in the method
Format_description_log_event::deduct_options_written_to_bin_log().
Like
if (server_version_split < Version(10,11,0))
return;
options_written_to_bin_log|= OPTION_INSERT_HISTORY;
See commit 7b500f04fb0b.
Rebase on top of the latest 10.6 to get 7b500f04fb0b and the
deduct_options_written_to_bin_log() method.
> > > diff --git a/sql/handler.cc b/sql/handler.cc
> > > index 393f6234653..4bbb40abb5b 100644
> > > --- a/sql/handler.cc
> > > +++ b/sql/handler.cc
> > > @@ -7493,6 +7496,26 @@ int handler::ha_write_row(const uchar *buf)
> > >DBUG_RETURN(error);
> > >}
> > >
> > > + if (table->versioned() && !table->vers_write)
> > > + {
> > > +Field *row_start= table->vers_start_field();
> > > +Field *row_end= table->vers_end_field();
> > > +MYSQL_TIME ltime;
> > > +
> > > +bitmap_set_bit(table->read_set, row_start->field_index);
> > > +bitmap_set_bit(table->read_set, row_end->field_index);
> > > +
> > > +/*
> > > + Inserting the history row directly, check ROW_START <= ROW_END and
> > > + ROW_START is non-zero.
> > > +*/
> > > +if (!row_end->is_max() && (
> > > + (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) ||
> > > + row_start->get_date( > > +TIME_NO_ZERO_DATE,
> > > time_round_mode_t(time_round_mode_t::FRAC_NONE)
> >
> > I don't quite understand this condition.
> > checking for row_end->is_max() is redundant, because row_start->cmp()
> > on itself is sufficient. So, I suppose you've done it as an optimization?
> > to avoid expensive cmp()?
> > But in that case you also allow zero date in the row_start, and this looks
> > like a bug.
> > I'd suggest to simplify the code and to remove is_max check.
> > Unless you've run benchmarks and it really helps. And unless
> > I've misunderstood its purpose.
>
> No, you're right. We can insert not only history but current data and
> row_start there must be checked too. Also, please note this comment:
>
> # NOTE: having row_start=0 might be useful and can mean
> # "there is no information on when the history was started" (an opposite to
> row_end=MAX_TIMESTAMP)
>
> Maybe we should allow it? Just to make the user not invent some values
> for this purpose (like "1970-01-01 00:00:00").
This would violate the concept that direct inserts are *only* a
usability shortcut and does not allow to do anything new. One cannot
have row_start=0 with
set timestampt=xxx;
insert ... values ...();
any other value of row_start can be faked, but not 0.
> > > + DBUG_RETURN(HA_ERR_WRONG_ROW_TIMESTAMP);
> > > + }
> > > +
> > >MYSQL_INSERT_ROW_START(table_share->db.str,
> > > table_share->table_name.str);
> > >mark_trx_read_write();
> > >increment_statistics(&SSV::ha_write_count);
Also I've looked at your "Review 2" commits in the bb-10.6-midenok branch,
no new comments.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help
