Hi, Aleksey,

On Jun 23, Aleksey Midenkov wrote:
> commit 0ddb9744cea
> Author: Aleksey Midenkov <midenok@mariadb.com>
> Date:   Sun May 25 23:23:29 2025 +0300
>
>     MDEV-33957 UPDATE fails on replica replicating blob virtual column in
>                NOBLOB mode when replica logging is off

please, put the whole commit subject in one line. various tools
assume the subject is one line only and don't handle line wrapping

>     
>     The sequence that causes the issue:
>     
>     1. file->row_logging is false because slave-bin was not open;
>     2. TABLE::mark_columns_per_binlog_row_image() didn't mark column for
>        read because file->row_logginbg is false. This was implemented in
>        e53ad95b733 (MDEV-6877);
>     3. TABLE::update_virtual_fields() didn't update virtual field value
>        because column is not marked for read;
>     4. calc_row_difference() sees o_len as UNIV_SQL_NULL, but new row
>        value is "1". The virtual column is added to update vector;
>     5. row_upd() tries to update secondary index, but row_upd_sec_step()
>        doesn't see old value in the index.
>     
>     The patch does mark_virtual_column_with_deps() in case of rgi_slave in
>     mark_columns_per_binlog_row_image() so that non-stored virtual columns
>     are marked for update in slave thread.
>
> diff --git a/sql/ha_partition.h b/sql/ha_partition.h
> index 01d15ca70ac..d926bd03137 100644
> --- a/sql/ha_partition.h
> +++ b/sql/ha_partition.h
> @@ -576,6 +576,17 @@ class ha_partition final :public handler
>    {
>      m_file[part_id]->update_create_info(create_info);
>    }
> +
> +  void column_bitmaps_signal() override
> +  {
> +    for (uint i= bitmap_get_first_set(&m_opened_partitions);
> +        i < m_tot_parts;
> +        i= bitmap_get_next_set(&m_opened_partitions, i))
> +    {
> +      m_file[i]->column_bitmaps_signal();
> +    }
> +  }

That's a good one. Although it's not really related to the bug in question.
Please at least mention this in the commit comment, like "also fixed..."

> +
>  private:
>    int copy_partitions(ulonglong * const copied, ulonglong * const deleted);
>    void cleanup_new_partition(uint part_count);
> diff --git a/sql/table.cc b/sql/table.cc
> index 3a51228d1f3..b48aaadb6e1 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -8019,6 +8024,12 @@ void TABLE::mark_columns_per_binlog_row_image()
>      }
>      file->column_bitmaps_signal();
>    }
> +#if MYSQL_VERSION_ID<110201
> +  else if (thd->rgi_slave)
> +  {
> +    file->column_bitmaps_signal();
> +  }

1. This doesn't match the commit comment, please update the latter to
describe the actual code change.

2. why MYSQL_VERSION_ID<110201 ?

3. you can also move `rpl_write_set= write_set` from the beginning
of the function here, inside the else branch. This makes the code
clearer: rpl_write_set was changed, so we have to signal it.

> +#endif
> 
>    DBUG_VOID_RETURN;
>  }

Regards,
Sergei
Chief Architect, MariaDB Server
and security@mariadb.org
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-leave@lists.mariadb.org