maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Andrei!

Few comments/questions below.

I still have troubles understanding the logic of your solution,
so I did not ask any questions about those your replies - as I'd be just
repeating the same thing over and over. After I'll start getting it
finally, I'll go over your reply again.

Meanwhile - minor comments/questions, not related to the main algorithm.

On Mar 04, Andrei Elkin wrote:
> >> To facilitate the failover on the slave side conditions to accept
> >> own events (having been discarded by the above recovery) are
> >> relaxed to let so for the semisync slave that connects to master in
> >> gtid mode. gtid_strict_mode is further recommended to secure from
> >> inadvertent re-applying out of order gtids in general. Non-gtid
> >> mode connected semisync slave would require
> >> --replicate-same-server-id (mind --log-slave-updates must be OFF
> >> then).
> >
> > Sorry, I failed to understand this paragraph :(
> 
> 'gtid_strict_mode is further recommended to secure
>  from inadvertent re-applying out of order gtids in general.'
> 
> attempted to promote GTID connection with master,
> which of course should not be of this patch's
> business, but in a way it is 'cos the non-gtid's mode
> (CHANGE MASTER TO ... master_use_gtid = NO) is covered by the patch
> with a limitation of '--log-slave-updates must be OFF'.
> 
> The GTID-ON connection is covered, and without any `--replicate-same-server-id'.

This doesn't seem to clarify anything.
I mean the whole paragraph, starting from "To facilitate" and to "then)."

Could you try to rephrase it to make it clearer, please?

> >> +--let $query1 = INSERT INTO t VALUES (20)
> >> +--let $query2 = DELETE FROM t2 WHERE f = 666 /* no such record */
> >> +--source binlog_truncate_active_log.inc
> >> +
> >> +--echo # Case B.
> >> +# The inverted sequence ends up to truncate only $query2
> >> +--let $this_search_pattern = Successfully truncated.*to remove transactions starting from GTID 0-1-10
> >> +--let $query1 = DELETE FROM t2 WHERE f = 0
> >
> > why not `= 666 /* no such record */` ?
> > is `= 0` important here?
> 
> Yes, there's a fine detail here.
> 
> 'The inverted sequence ends up' to leave in binlog
> 
>   $query1 = DELETE FROM t2 WHERE f = 0
> 
> which is as ineffective as 666. The patch recognizes the ineffectiveness
> through a special value of engines involved counter in Gtid event.
> In principle the truncation could
> start from it rather than the following $query2.
> But that would mean more complicated coding.
> So the semantics of the zero is to be good, as we're so used to :-)!

Sorry, I didn't understand anything. What does the value in the WHERE
clause has to do with engines and where the binlog is truncated?

> >
> >> +--let $query2 = INSERT INTO t VALUES (20)
> >> +--source binlog_truncate_active_log.inc
...
> >> +--connection master2
> >> +SET DEBUG_SYNC= "now WAIT_FOR master1_ready";
> >> +if ($pre_q2)
> >
> > is $pre_q2 a flag or a query string?
> > it's used as a flag, it's set as a query string.
> 
> It's actually both with the idea execute it when defined.
> Legit, right?

legit, but it's not both. Were it both, you'd have

  if ($pre_q2) {
    eval $pre_q2;
  }

but you have, 

  if ($pre_q2) {
    CALL sp_blank_xa;
  }

> >> +{
> >> +  CALL sp_blank_xa;
> >> +}
...
> >> diff --git a/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test
> >> new file mode 100644
> >> index 00000000000..fe153e5703c
> >> --- /dev/null
> >> +++ b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test
> >> +#
> >> +# ==== Implementation ====
> >> +#
> >> +# Steps:
> >> +#    0 - Create two tables in innodb and rocksdb engines,
> >> +#
> >> +# In loop for A,B,C cases (below) do 1-5:
> >
> > there's no loop now (which is good, don't add it please :)

[x] ?

> >> +# The 1st trx binlogs, rotate binlog and hold on before committing at engine
> >> +SET DEBUG_SYNC= "commit_after_release_LOCK_after_binlog_sync SIGNAL master1_ready WAIT_FOR master1_go";
> >
> > you don't wait for master1_ready anywhere. intentional?
> 
> No, actually
> 
>   SET DEBUG_SYNC= "now WAIT_FOR master1_ready";
> 
> must be run by below master2 (sim to how master3 waits for master2).

meaning [x] ?

> >> +--source include/have_binlog_format_row.inc
> >> +--let $rpl_topology=1->2
> >> +--source include/rpl_init.inc
> >
> > isn't this a normal master-slave.inc topology?
> 
> Initially yes.
> 
> I am guessing you mean why not to
>   --source it.
> The test heavily uses 'server_`index`' format to automate
> replication direction.

as far as I remember, master-slave.inc also defines server_1 and
server_2 connections.

> >> diff --git a/sql/sql_class.h b/sql/sql_class.h
> >> index 140394fefc1..87fa88d4c89 100644
> >> --- a/sql/sql_class.h
> >> +++ b/sql/sql_class.h
> >> @@ -4632,7 +4632,7 @@ class THD :public Statement,
> >>    LF_PINS *tdc_hash_pins;
> >>    LF_PINS *xid_hash_pins;
> >>    bool fix_xid_hash_pins();
> >> -
> >> +  bool is_1pc_ro_trans;
> >
> > I don't like how it looks, it's in THD (for every single connection
> > and more) and it's only used in some exotic cases. It's set in two
> > places around ha_commit_one_phase(), when it would be better to set
> > it inside ha_commit_one_phase(), in one place only.
> >
> > But I cannot suggest how to get rid of it yet as I don't understand
> > what it's for, I've asked a question about it below, in
> > Gtid_log_event::Gtid_log_event
> 
> The flag is to help sorting out ineffective (no engine data change)
> STATEMENT binlog-format logged transaction. I thought it's practical
> enough to face and important to handle.
> Through an example,
> let the table `t' be initially empty, and
> let binlog contain
> 
> trx1: INSERT INTO t SET   a = 1;   /* slave acked, trx got committed in engine */
> trx2: INSERT INTO t SET   a = 2;   /* not acked, trx remains prepared in engine */
> trx3: DELETE FROM t WHERE a = 0;   /* ineffective, does not have Xid event */
> 
> prior a crash with their states as commented.
> The truncate trx should be trx2 as the trx3's status
> suggests it's harmless for involving into
> truncation.
> However we have yet to distinguish it from unsafe (to truncate)
> event group ("transaction") that also ends with COMMIT query not Xid event.

Hmm. I see. But thd isn't a universal dumpster for passing
arguments to the function, don't use it like that.

Please, pass this "read-only transaction with no xid" flag to the
Gtid_log_event constructor as an argument. Or recalculate it inside the
constructor, it's not such an expensive thing to do.

> One rather drastic solution could be to not binlog DELETE altogether.
> I did not look into this with much of interest earlier, but how about it?
> Perhaps it'd more like a bug fixes than breaking legacy.

No, I suspect it's too drastic

> >>  /* Members related to temporary tables. */
> >>  public:
> >>    /* Opened table states. */
> >> diff --git a/sql/slave.cc b/sql/slave.cc
> >> index 28e08e32346..9b3f73e5341 100644
> >> --- a/sql/slave.cc
> >> +++ b/sql/slave.cc
> >> @@ -6944,7 +6945,9 @@ static int queue_event(Master_info* mi,const char* buf, ulong event_len)
> >>    }
> >>    else
> >>    if ((s_id == global_system_variables.server_id &&
> >> -       !mi->rli.replicate_same_server_id) ||
> >> +       (!mi->rli.replicate_same_server_id &&
> >> +        !(semisync_recovery= (rpl_semi_sync_slave_enabled &&
> >> +                              mi->using_gtid != Master_info::USE_GTID_NO)))) ||
> >
> > How can "semisync recovery" code path reach queue_event()?
> 
> This is a failover now, after recovery.
> The slave which is ex-master tries to adopt "own" (generated by it) events.

1. don't call it "semisync recovery" then
2. add a comment explaining this case (it's a rare and not obvious one)
3. why does it have to be specially recognized anyway?

> >
> >>        event_that_should_be_ignored(buf) ||
> >>        /*
> >>          the following conjunction deals with IGNORE_SERVER_IDS, if set
> >> diff --git a/sql/log_event.h b/sql/log_event.h
> >> index 8a342cb5cd3..1588ab85104 100644
> >> --- a/sql/log_event.h
> >> +++ b/sql/log_event.h
> >> @@ -482,6 +482,16 @@ class String;
> >>  */
> >>  #define LOG_EVENT_IGNORABLE_F 0x80
> >>  
> >> +/**
> >> +   @def LOG_EVENT_ACCEPT_OWN_F
> >> +
> >> +   Flag sets by the gtid-mode connected semisync slave for
> >> +   the same server_id ("own") events which the slave must not have
> >> +   in its state. Typically such events were never committed by
> >> +   their originator (this server) and discared at its crash recovery
> >
> > sorry, I failed to understand that
> 
> This should've been explained above.
> The slave which is ex-master truncated at recovery some of its ("own") transactions.
> The flag serves to help recognize that at execution of the replication events.

1. yes, this is clearer, thanks. may be you should rewrite your comment
putting this explanation in it?

2. why does it have to be specially recognized anyway?

...
> > 3. also it's somewhat an unfounded assumption that only r/w engines
> > will have the transaction prepared. But we can make it a fact, if we
> > won't prepare r/o engines at all (in ha_prepare).
> 
> [?]
> 
> Sorry, I don't get it.
> Do you mean that an r/o engine (branch) might need to be counted by
> `ha_count_rw()`? But then it would be contributing with binlogged
> events of its branch... I can't imagine that.

I mean the following. Say, you have two engines participating in a
transaction, like in  INSERT innodb_table SELECT * FROM rocksdb_table;
One is r/w, the other is r/o. It's a 2pc transaction, the server does
prepare for everyone, then commit for everyone.

I don't think there is any guarantee that prepare for an r/o engine will
be a no-op. It is completely up to the engine what to do in this case,
it's possible that the engine will optimize it to a no-op, but it is not
a certainty.

What I think we should do is not to call prepare() for r/o engines at
all. Just skip straight to commit/rollback on the second phase.

> >> diff --git a/sql/handler.cc b/sql/handler.cc
> >> index 6792e80b8fe..247ab7267bf 100644
> >> --- a/sql/handler.cc
> >> +++ b/sql/handler.cc
> >> @@ -1245,6 +1245,24 @@ int ha_prepare(THD *thd)
> >>    DBUG_RETURN(error);
> >>  }
> >>  
> >> +/*
> >> +  Returns counted number of
> >> +  read-write recoverable transaction participants.
> >> +*/
> >> +uint ha_count_rw(THD *thd, bool all)
> >
> > the name doesn't match the implementation, please, rename
> 
> [?]
> 
> Please help here.
> I could not find what specifically you mean.
> It
>    returns rw_ha_count
> 
> which I think as 'read-write recoverable transaction participants.'

I read the name is "a number of rw engines in the ha_info",
it does not mention "recoverable", and I from the name I woulnd't have
guessed. May be ha_count_rw_2pc ?

> >> +{
> >> +  unsigned rw_ha_count= 0;
> >> +  THD_TRANS *trans=all ? &thd->transaction.all : &thd->transaction.stmt;
> >> +
> >> +  for (Ha_trx_info * ha_info= trans->ha_list; ha_info;
> >> +       ha_info= ha_info->next())
> >> +  {
> >> +    if (ha_info->is_trx_read_write() && ha_info->ht()->recover)
> >> +      ++rw_ha_count;
> >> +  }
> >> +  return rw_ha_count;
> >> +}
> >> +
> >>  /**
> >>    Check if we can skip the two-phase commit.
> >>  
> >> @@ -1960,8 +1982,122 @@ struct xarecover_st
> >>    XID *list;
> >>    HASH *commit_list;
> >>    bool dry_run;
> >> +  MEM_ROOT *mem_root;
> >> +  bool error;
> >>  };
> >>  
> >> +/**
> >> +  Inserts a new hash member.
> >> +
> >> +  returns a successfully created and inserted @c xid_recovery_member
> >> +           into hash @c hash_arg,
> >> +           or NULL.
> >> +*/
> >> +static xid_recovery_member*
> >> +xid_member_insert(HASH *hash_arg, my_xid xid_arg, MEM_ROOT *ptr_mem_root)
> >> +{
> >> +  xid_recovery_member *member= (xid_recovery_member*)
> >> +    alloc_root(ptr_mem_root, sizeof(xid_recovery_member));
> >> +  if (!member)
> >> +    return NULL;
> >> +
> >> +  member->xid= xid_arg;
> >> +  member->in_engine_prepare= 1;
> >> +  member->decided_to_commit= false;
> >> +
> >> +  return my_hash_insert(hash_arg, (uchar*) member) ? NULL : member;
> >> +}
> >> +
> >> +/*
> >> +  Inserts a new or updates an existing hash member to increment
> >> +  the member's prepare counter.
> >> +
> >> +  returns false  on success,
> >> +           true   otherwise.
> >> +*/
> >> +static bool xid_member_replace(HASH *hash_arg, my_xid xid_arg,
> >> +                               MEM_ROOT *ptr_mem_root)
> >> +{
> >> +  xid_recovery_member* member;
> >> +  if ((member= (xid_recovery_member *)
> >> +       my_hash_search(hash_arg, (uchar *)& xid_arg, sizeof(xid_arg))))
> >> +    member->in_engine_prepare++;
> >> +  else
> >> +    member= xid_member_insert(hash_arg, xid_arg, ptr_mem_root);
> >> +
> >> +  return member == NULL;
> >> +}
> >> +
> >> +/*
> >> +  Hash iterate function to complete with commit or rollback as decided
> >> +  (typically at binlog recovery processing) in member->in_engine_prepare.
> >> +*/
> >> +static my_bool xarecover_do_commit_or_rollback(void *member_arg,
> >> +                                               void *hton_arg)
> >> +{
> >> +  xid_recovery_member *member= (xid_recovery_member*) member_arg;
> >> +  handlerton *hton= (handlerton*) hton_arg;
> >> +  xid_t x;
> >> +  my_bool rc;
> >> +
> >> +  x.set(member->xid);
> >> +  rc= member->decided_to_commit ?  hton->commit_by_xid(hton, &x) :
> >> +    hton->rollback_by_xid(hton, &x);
> >> +
> >> +  DBUG_ASSERT(rc || member->in_engine_prepare > 0);
> >> +
> >> +  if (!rc)
> >> +  {
> >
> > I don't think you can trust rc on that.
> > if it's non-zero, it's an error all right.
> > but it's a bit of a stretch to presume that
> > nonexisting xid is always an error.
> >
> > also commit_by_xid returns int, not my_bool.
> 
> [?]

I mean, if an engine returns success when you ask it to commit or
rollback an unknown xid - this is not a bug, right? Commit means "make
all changes by this xid persistent", if there are no changes, there is
nothing to do, so doing nothing and reporting success isn't exactly
wrong. Same for rollback.

> I'd rather to stick with the patch and require from any engine to
> return \cite{xa spec}
> 
>   [XAER_NOTA] The specified XID is not known by the resource manager
> 
> What'd you say?

Okay. I'd say I was wrong, if the standard requires a resource manager
to return an error for an unknown XID, then you surely can rely on that
:)

Sorry for confusion.

> >> +
> >> +  return false;
> >> +}
> >> +
> >> +static my_bool xarecover_do_count_in_prepare(void *member_arg,
> >> +                                          void *ptr_count)
> >> +{
> >> +  xid_recovery_member *member= (xid_recovery_member*) member_arg;
> >> +  if (member->in_engine_prepare)
> >> +  {
> >> +    *(uint*) ptr_count += member->in_engine_prepare;
> >
> > This is a rather weird semantics.
> > it's kind of a number of transactions times number of engines.
> > if one transaction wasn't committed in two engines and another
> > transaction wasn't rolled back in one engine, the counter will be 3.
> > how are you going to explain this to users?
> 
> So `ptr_count` actually is for transaction branches, not transactions.
> But the ultimate warning speaks erronously the latter
> 
>    +      sql_print_warning("Could not complete %u number of transactions in "
>                             "engines.", count_in_prepare);
> 
> So I'd convert the counter to count the transactions:
> 
>           (*(uint*) ptr_count) ++
> 
> [?]

yes, that'd be easier to understand, thanks

> >> diff --git a/sql/log.cc b/sql/log.cc
> >> index 8073f09ab88..a9808ed8823 100644
> >> --- a/sql/log.cc
> >> +++ b/sql/log.cc
...
> >> +    char buf[21];
> >> +
> >> +    longlong10_to_str(ptr_gtid->seq_no, buf, 10);
> >> +    sql_print_information("Successfully truncated binlog file:%s "
> >> +                          "to pos:%llu to remove transactions starting from "
> >> +                          "GTID %u-%u-%s", file_name, pos,
> >> +                          ptr_gtid->domain_id, ptr_gtid->server_id, buf);
> >> +  }
> >> +  if (!(error= init_io_cache(&cache, file, IO_SIZE, WRITE_CACHE,
> >> +                            (my_off_t) pos, 0, MYF(MY_WME|MY_NABP))))
> >> +  {
> >> +    /*
> >> +      Write Stop_log_event to ensure clean end point for the binary log being
> >> +      truncated.
> >> +    */
> >
> > I'm not sure about it. The less you touch the corrupted binlog the better.
> > simply truncating it would be enough, wouldn't it?
> 
> Yes, it would.
> But I somewhat preferred a tidy-up style.
> 
> [?]
> 
> If you feel strong to remove it will be done.

I feel it'd be safer not to write anything into the corrupted binlog.

> >> +    Stop_log_event se;
> >> +    se.checksum_alg= cs_alg;
> >> +    if ((error= write_event(&se, NULL, &cache)))
> >> +    {
> >> +      sql_print_error("Failed to write stop event to "
> >> +                      "binary log. Errno:%d",
> >> +                      (cache.error == -1) ? my_errno : error);
> >> +      goto end;
> >> +    }
> >> +    clear_inuse_flag_when_closing(cache.file);
> >> +    if ((error= flush_io_cache(&cache)) ||
> >> +        (error= mysql_file_sync(file, MYF(MY_WME|MY_SYNC_FILESIZE))))
> >> +    {
> >> +      sql_print_error("Faild to write stop event to "
> >> +                      "binary log. Errno:%d",
> >> +                      (cache.error == -1) ? my_errno : error);
> >> +    }
> >> +  }
> >> +  else
> >> +      sql_print_error("Failed to initialize binary log "
> >> +                      "cache for writing stop event. Errno:%d",
> >> +                      (cache.error == -1) ? my_errno : error);
> >
> > 1. I don't see why you need to sync after every operation.
> 
> Yep, there's one fsync before Stop event writing, redundant if Stop will
> stay in the final patch.

As far as I understand, if Stop is removed, you shouldn't need any syncs
at all, just truncate and close.

> >> +  int  next_binlog_or_round(int& round,
> >> +                            const char *last_log_name,
> >> +                            const char *binlog_checkpoint_name,
> >> +                            LOG_INFO *linfo, MYSQL_BIN_LOG *log);
> >> +  bool is_safe()
> >
> > what do you mean "safe" ?
> 
> Safe to truncate condition is explained elsewhere to mean there are no
> non-transactional group of events within the truncated tail.
> Earlier mentioned ineffective COMMIT-query ended "transactions" are safe.

may be is_safe_to_truncate() then?

...
> >> +        if (!truncate_validated)
> >> +        {
> >> +          DBUG_ASSERT(round <= 2);
> >> +          /*
> >> +            Either truncate was not set or was reset, else
> >> +            it gets incremented, otherwise it may only set to an earlier
> >> +            offset only at the turn out of the 1st round.
> >
> > sorry, I cannot parse that :(
> 
> The comments are for the assert in a branch that acts when the truncate position
> (gtid) is not yet finalized.
> Let me annotate under each assert's line.
> 
> >
> >> +          */
> >> +          DBUG_ASSERT(truncate_gtid.seq_no == 0 ||
> 
> 'truncate was not set or was reset' ^, that is truncate_gtid.
> The 'was reset' is actually not present in the assert - that'd be
> reflected with
>   truncate_reset_done == true
>  
> 
> >> +                      last_gtid_coord >= binlog_truncate_coord ||
> 
> 'it gets incremented' ^

why does that mean "it gets incremented" ?

> 
> >> +                      (round == 2 && truncate_set_in_1st));
> 
> And if it's neither of the above there must've been truncate candidate
> set in the 1st round.
 
...
> >>        switch (typ)
> >>        {
> >> +      case FORMAT_DESCRIPTION_EVENT:
> >> +        if (round > 1)
> >> +        {
> >> +          if (fdle != fdle_arg)
> >> +            delete fdle;
> >> +          fdle= (Format_description_log_event*) ev;
> >
> > why is it needed? all binlog files should have the same
> > Format_description_log_event anyway.
> 
> Actually contrary to what was stated (the patch removed that claim) in
> the parent revision of this commit, the FD:s of the
> recovery binlog files may have different checksums. I.e
> 
>    set @@global.binlog_checksum = value
> 
> can be issued to rotate binlog to a new hot file having different checksum value
> while the binlog *CheckPoint* file is behind. So this is possible to
> have at recovery:
> 
>   B_1(CP, checksum = NONE), B_2(hot, checksum = CRC32)
> 
> which also manifests a normal use case bug I have not reported any bug (though wanted to).

okay, checksum can differ. But how does it affect
Format_description_log_event ?

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