maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei,

On Tue, Feb 15, 2022 at 4:35 PM Sergei Golubchik <serg@xxxxxxxxxxx> 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_ONLY    NO
> >  COMMAND_LINE_ARGUMENT        REQUIRED
> > +VARIABLE_NAME        FORCE_FIELDS_VISIBLE
> > +VARIABLE_SCOPE       SESSION
> > +VARIABLE_TYPE        BOOLEAN
> > +VARIABLE_COMMENT     Make invisible fields visible on next table open
> > +NUMERIC_MIN_VALUE    NULL
> > +NUMERIC_MAX_VALUE    NULL
> > +NUMERIC_BLOCK_SIZE   NULL
> > +ENUM_VALUE_LIST      OFF,ON
> > +READ_ONLY    NO
> > +COMMAND_LINE_ARGUMENT        OPTIONAL
>
> apparently, forgot to update embedded result after renaming the variable

Fixed.

>
> >  VARIABLE_NAME        FOREIGN_KEY_CHECKS
> >  VARIABLE_SCOPE       SESSION
> >  VARIABLE_TYPE        BOOLEAN
> > 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(&ltime, Datetime::Options(
> > +            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").

>
> > +      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);
> > diff --git a/sql/item.cc b/sql/item.cc
> > index a016f04953c..cf259ebd53c 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -5986,6 +5986,13 @@ bool Item_field::fix_fields(THD *thd, Item **reference)
> >      else if (!from_field)
> >        goto error;
> >
> > +    if (thd->column_usage == MARK_COLUMNS_WRITE &&
> > +        from_field != view_ref_found &&
> > +        thd->vers_insert_history(from_field))
>
> Could you try to avoid invoking THD::vers_insert_history()
> (which isn't inlined) for every fix_fields on every field that's
> going to be modified (so every INSERT and every UPDATE)?

It turns out this compiles as inline. Fixed.

>
> > +    {
> > +      DBUG_ASSERT(from_field->table->versioned());
> > +      from_field->table->vers_write= false;
> > +    }
> >      table_list= (cached_table ? cached_table :
> >                   from_field != view_ref_found ?
> >                   from_field->table->pos_in_table_list : 0);
> > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > index 51e4bcad16b..bf65cd4a279 100644
> > --- a/sql/sql_base.cc
> > +++ b/sql/sql_base.cc
> > @@ -6039,7 +6039,8 @@ find_field_in_table(THD *thd, TABLE *table, const char *name, size_t length,
> >
> >      if (field->invisible == INVISIBLE_SYSTEM &&
> >          thd->column_usage != MARK_COLUMNS_READ &&
> > -        thd->column_usage != COLUMNS_READ)
> > +        thd->column_usage != COLUMNS_READ &&
> > +        !thd->vers_insert_history(field))
>
> it's okay to invoke THD::vers_insert_history here, as INVISIBLE_SYSTEM
> fields are rare
>
> >        DBUG_RETURN((Field*)0);
> >    }
> >    else
> > @@ -8534,7 +8535,8 @@ fill_record(THD *thd, TABLE *table_arg, List<Item> &fields, List<Item> &values,
> >      if (table->next_number_field &&
> >          rfield->field_index ==  table->next_number_field->field_index)
> >        table->auto_increment_field_not_null= TRUE;
> > -    const bool skip_sys_field= rfield->vers_sys_field(); // TODO: && !thd->vers_modify_history() [MDEV-16546]
> > +    const bool skip_sys_field= rfield->vers_sys_field() &&
> > +      (update || !thd->vers_insert_history(rfield));
>
> same. it's behind rfield->vers_sys_field() check, so ok.
>
> >      if ((rfield->vcol_info || skip_sys_field) &&
> >          !value->vcol_assignment_allowed_value() &&
> >          table->s->table_category != TABLE_CATEGORY_TEMPORARY)
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx



-- 
@midenok