maria-developers team mailing list archive

[andrei.elkin@xxxxxxxxxx]

Sujatha, salute!

Thanks for a detailed description! I would add up to it a minor piece

> revision-id: 53d0e9217943719b806ef9fa1cac0a699df4839c (mariadb-10.1.39-31-g53d0e921794)
> parent(s): 47637a3dd13d19e897a7cbfd1499f1bf3b2fdb2a
> author: Sujatha
> committer: Sujatha
> timestamp: 2019-05-17 13:11:49 +0530
> message:
>
> MDEV-11094: Blackhole table updates on slave fail when row annotation is enabled
>
> Problem:
> =======
> rpl_blackhole.test fails when executed with following options
> mysqld=--binlog_annotate_row_events=1, mysqld=--replicate_annotate_row_events=1
>
> Test output:
> ------------
> worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 16000..16019
> rpl.rpl_blackhole_bug 'mix'              [ pass ]    791
> rpl.rpl_blackhole_bug 'row'              [ fail ]
> Replicate_Wild_Ignore_Table
> Last_Errno	1032
> Last_Error	Could not execute Update_rows_v1 event on table test.t1; Can't find
> record in 't1', Error_code: 1032; handler error HA_ERR_END_OF_FILE; the event's
> master log master-bin.000001, end_log_pos 1510
>
> Analysis:
> =========
> Enabling "replicate_annotate_row_events" on slave, Tells the slave to write
> annotate rows events received from the master to its own binary log. The
> received annotate events are applied after the Gtid event as shown below.
> thd->query() will be set to the actual query received from the master, through
> annotate event. Annotate_rows event should not be deleted after the event is
> applied as the thd->query will be used to generate new Annotate_rows event
> during applying the subsequent Rows events.

[ here ]
  The Table_map and Rows_log_event that follow do not touch thd->query().

Notice that the being suggested sentence is sufficient to explain the
not-NULL without the sentence below (fill free to remove then):

> After the last Rows event has been
> applied, the saved Annotate_rows event (if any) will be deleted.
>
> master-bin.000001 | 457 | Gtid          | BEGIN GTID 0-1-2
> master-bin.000001 | 495 | Annotate_rows | insert into t values (10)
> master-bin.000001 | 539 | Table_map     | table_id: 19 (test.t)
> master-bin.000001 | 579 | Write_rows_v1 | table_id: 19 flags: STMT_END_F
> master-bin.000001 | 613 | Xid           | COMMIT /* xid=7 */
>
> In balckhole engine all the DML operations are noops as they donot store any
> data. They simply return success without doing any operation. But the existing
> strictly expects thd->query() to be 'NULL' to identify that row based
> replication is in use. This assumption will fail when row annotations are
> enabled as the query is not 'NULL'. Hence various row based operations like
> 'update', 'delete', 'index lookup' will fail when row annotations are enabled.
>

> Fix:
> ===
> Extend the row based replication check to include row annotations as well.
> i.e Either the thd->query() is NULL or thd->query() points to query and row
> annotations are in use.

This works, but the check does not really refers to the Annotate. So in
principle thd->query() may turn in not-NULL of different reasons.
Besides the check is multiplied over few places (again hopefully
covering all).

We can do it simpler actually:

Rows_log_event::do_apply_event()
{
    ...
    do
    {
      /* in_use can have been set to NULL in close_tables_for_reopen */
      THD* old_thd= table->in_use;
      if (!table->in_use)
        table->in_use= thd;

 --> add a check whether Annotate has been done and when so store/restore
 --> thd->query() like `old_thd` above/below, and pass it as NULL (thd->reset_query()) to the following:
  
      error= do_exec_row(rgi);

      if (error)
        DBUG_PRINT("info", ("error: %s", HA_ERR(error)));
      DBUG_ASSERT(error != HA_ERR_RECORD_DELETED);

      table->in_use = old_thd;


I also suggest to check out mysqlbinlog run. There seems to be no test
for that though, so one needs to be written. The goal would be to prove
a created binlog output is as expected (e.g to include Annotate).

Cheers,

Andrei

>
> ---
>  mysql-test/suite/rpl/t/rpl_blackhole-master.opt |  1 +
>  mysql-test/suite/rpl/t/rpl_blackhole-slave.opt  |  1 +
>  storage/blackhole/ha_blackhole.cc               | 28 +++++++++++++++++++------
>  3 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/mysql-test/suite/rpl/t/rpl_blackhole-master.opt b/mysql-test/suite/rpl/t/rpl_blackhole-master.opt
> new file mode 100644
> index 00000000000..1ad0b884c60
> --- /dev/null
> +++ b/mysql-test/suite/rpl/t/rpl_blackhole-master.opt
> @@ -0,0 +1 @@
> +--binlog_annotate_row_events
> diff --git a/mysql-test/suite/rpl/t/rpl_blackhole-slave.opt b/mysql-test/suite/rpl/t/rpl_blackhole-slave.opt
> new file mode 100644
> index 00000000000..7ac6a84faa7
> --- /dev/null
> +++ b/mysql-test/suite/rpl/t/rpl_blackhole-slave.opt
> @@ -0,0 +1 @@
> +--binlog_annotate_row_events --replicate_annotate_row_events
> diff --git a/storage/blackhole/ha_blackhole.cc b/storage/blackhole/ha_blackhole.cc
> index 01aaa9ea15f..43bcdc541a1 100644
> --- a/storage/blackhole/ha_blackhole.cc
> +++ b/storage/blackhole/ha_blackhole.cc
> @@ -25,6 +25,16 @@
>  #include "ha_blackhole.h"
>  #include "sql_class.h"                          // THD, SYSTEM_THREAD_SLAVE_SQL
>  
> +static bool is_row_based_replication(THD *thd)
> +{
> +  /*
> +    A row event which has its thd->query() == NULL or a row event which has
> +    replicate_annotate_row_events enabled. In the later case the thd->query()
> +    will be pointing to the query, received through replicated annotate event
> +    from master.
> +  */
> +  return ((thd->query() == NULL) || thd->variables.binlog_annotate_row_events);
> +}
>  /* Static declarations for handlerton */
>  
>  static handler *blackhole_create_handler(handlerton *hton,
> @@ -109,7 +119,8 @@ int ha_blackhole::update_row(const uchar *old_data, uchar *new_data)
>  {
>    DBUG_ENTER("ha_blackhole::update_row");
>    THD *thd= ha_thd();
> -  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query() == NULL)
> +  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL &&
> +      is_row_based_replication(thd))
>      DBUG_RETURN(0);
>    DBUG_RETURN(HA_ERR_WRONG_COMMAND);
>  }
> @@ -118,7 +129,8 @@ int ha_blackhole::delete_row(const uchar *buf)
>  {
>    DBUG_ENTER("ha_blackhole::delete_row");
>    THD *thd= ha_thd();
> -  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query() == NULL)
> +  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL &&
> +      is_row_based_replication(thd))
>      DBUG_RETURN(0);
>    DBUG_RETURN(HA_ERR_WRONG_COMMAND);
>  }
> @@ -135,7 +147,8 @@ int ha_blackhole::rnd_next(uchar *buf)
>    int rc;
>    DBUG_ENTER("ha_blackhole::rnd_next");
>    THD *thd= ha_thd();
> -  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query() == NULL)
> +  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL &&
> +      is_row_based_replication(thd))
>      rc= 0;
>    else
>      rc= HA_ERR_END_OF_FILE;
> @@ -220,7 +233,8 @@ int ha_blackhole::index_read_map(uchar * buf, const uchar * key,
>    int rc;
>    DBUG_ENTER("ha_blackhole::index_read");
>    THD *thd= ha_thd();
> -  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query() == NULL)
> +  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL &&
> +      is_row_based_replication(thd))
>      rc= 0;
>    else
>      rc= HA_ERR_END_OF_FILE;
> @@ -235,7 +249,8 @@ int ha_blackhole::index_read_idx_map(uchar * buf, uint idx, const uchar * key,
>    int rc;
>    DBUG_ENTER("ha_blackhole::index_read_idx");
>    THD *thd= ha_thd();
> -  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query() == NULL)
> +  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL &&
> +      is_row_based_replication(thd))
>      rc= 0;
>    else
>      rc= HA_ERR_END_OF_FILE;
> @@ -249,7 +264,8 @@ int ha_blackhole::index_read_last_map(uchar * buf, const uchar * key,
>    int rc;
>    DBUG_ENTER("ha_blackhole::index_read_last");
>    THD *thd= ha_thd();
> -  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query() == NULL)
> +  if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL &&
> +      is_row_based_replication(thd))
>      rc= 0;
>    else
>      rc= HA_ERR_END_OF_FILE;
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits