maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita,

Sorry, this comes very late. But the bug is still present in 10.10,
and the filter sorts by the date precisely to highlight these old bugs.
So, here you are.

First comment, please, port it to the latest 10.3. I wasn't able to find
the repository with the fix, but the patch itself is avaliable from the
pull request as https://github.com/MariaDB/server/commit/c83f87ece4a.patch
so it can be salvaged.

More comments - see below.

On Oct 15, Nikita Malyavin wrote:
> From: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> Date: Fri, 20 Jul 2018 00:24:40 +1000
> Subject: [PATCH] MDEV-15990: REPLACE on a precise-versioned table returns
> * handle zero value in `row_start` `trx_id`
> * refuse to do more than one versioned insert on the same transaction and same row
> * generalize versioned cases with and without triggers
> 
> diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
> index dda469a1426d5..cd2690128de67 100644
> --- a/sql/sql_insert.cc
> +++ b/sql/sql_insert.cc
> @@ -1928,32 +1928,15 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info)
>            For system versioning wa also use path through delete since we would
>            save nothing through this cheating.
>          */
> -        if (last_uniq_key(table,key_nr) &&
> +        if (last_uniq_key(table,key_nr) && !table->versioned() &&

ok!

>              !table->file->referenced_by_foreign_key() &&
>              (!table->triggers || !table->triggers->has_delete_triggers()))
>          {
> -          if (table->versioned(VERS_TRX_ID) && table->vers_write)
> -          {
> -            bitmap_set_bit(table->write_set, table->vers_start_field()->field_index);
> -            table->vers_start_field()->store(0, false);
> -          }
> -          if (unlikely(error= table->file->ha_update_row(table->record[1],
> -                                                         table->record[0])) &&
> -              error != HA_ERR_RECORD_IS_THE_SAME)
> +          error= table->file->ha_update_row(table->record[1], table->record[0]);
> +          if (unlikely(error && error != HA_ERR_RECORD_IS_THE_SAME))
>              goto err;
>            if (likely(!error))
> -          {
>              info->deleted++;
> -            if (table->versioned(VERS_TIMESTAMP) && table->vers_write)
> -            {
> -              store_record(table, record[2]);
> -              error= vers_insert_history_row(table);
> -              restore_record(table, record[2]);
> -              if (unlikely(error))
> -                goto err;
> -            }
> -          }
> -          else
>              error= 0;   // error was HA_ERR_RECORD_IS_THE_SAME
>            thd->record_first_successful_insert_id_in_cur_stmt(table->file->insert_id_for_cur_row);
>            /*
> @@ -1969,23 +1952,22 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info)
>                                                  TRG_ACTION_BEFORE, TRUE))
>              goto before_trg_err;
>  
> -          if (!table->versioned(VERS_TIMESTAMP))
> +          if (table->versioned(VERS_TRX_ID) && table->vers_write)
> +          {
> +            bitmap_set_bit(table->write_set, table->vers_start_field()->field_index);

this is conceptually wrong, you cannot set table->write_set in the middle
of statement execution. that is you can and it might even work for some
engines, but it breaks the API.

> +            table->vers_start_field()->store(0, false);
> +          }
> +
> +          if (!table->versioned())
>              error= table->file->ha_delete_row(table->record[1]);
>            else
> -          {
> -            store_record(table, record[2]);
> -            restore_record(table, record[1]);
> -            table->vers_update_end();

why? is the row_end updated somewhere else?

>              error= table->file->ha_update_row(table->record[1],
>                                                table->record[0]);
> -            restore_record(table, record[2]);
> -          }
> -          if (unlikely(error))
> +          if (unlikely(error && error != HA_ERR_RECORD_IS_THE_SAME))
>              goto err;
> -          if (!table->versioned(VERS_TIMESTAMP))
> +          if (likely(!error))
>              info->deleted++;
> -          else
> -            info->updated++;
> +          error= 0;
>            if (!table->file->has_transactions())
>              thd->transaction.stmt.modified_non_trans_table= TRUE;
>            if (table->triggers &&
> @@ -1995,6 +1977,17 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info)
>              trg_error= 1;
>              goto ok_or_after_trg_err;
>            }
> +          if (table->versioned(VERS_TIMESTAMP) && table->vers_write)
> +          {
> +            store_record(table, record[2]);
> +            error = vers_insert_history_row(table);

if you do that, you shouldn't have been doing ha_update_row() above,
as far as I can see. What is that update above for?

> +            restore_record(table, record[2]);
> +            if (unlikely(error))
> +              goto err;
> +          }
> +          if (table->versioned())
> +            goto after_trg_n_copied_inc;
> +
>            /* Let us attempt do write_row() once more */
>          }
>        }

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