maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Andrei!

On Jun 03, Andrei Elkin wrote:
> >> -  binlog_hton->commit= binlog_commit;
> >> +  binlog_hton->commit= binlog_commit_dummy;
> >>    binlog_hton->rollback= binlog_rollback;
> >>    binlog_hton->drop_table= [](handlerton *, const char*) { return -1; };
> >
> > Oh did you actually need it? Or it's for completeness only?
> 
> I am confused a bit,
> 6c52931680a4 (Sergei Golubchik 2020-06-22 1703) binlog_hton->drop_table= [](handlerton *, const char*) { return -1; };
> 
> And I never reviewed the patch.

Grrr. Sorry!

When reviewing I diffed this patch with a previous version that I
reviewed (a useful speedup technique at the late phase, when commits
only change slightly) and this drop_table line was in the new patch and
not in the old.

Of course, it was because the new patch was in 10.6 and the old in 10.2,
there were quite a few changes in the diff because of that. I tried to
ignore them, but somehow missed this one. Note, there's no + before the
drop_table line, it's not yours, it's indeed something that already was
in 10.6, I should've noticed, my mistake :(

Why I added it in 6c52931680a4? It was for DROP TABLE FORCE, that simply
does plugin_foreach() and tries to hton->drop_table() in every hton
available. In binlog_hton too. The goal was to solve cases like a table
in InnoDB data dictionary but with no frm, or Aria table where MAD file
exists, but no MAI - cases when DROP TABLE doesn't see the table, but
CREATE TABLE still fails as some remnants of a table still exist.

> >> @@ -9975,6 +9977,154 @@ int TC_LOG::using_heuristic_recover()
> >> +  else
> >> +  {
> >> +    char buf[21];
> >> +    my_stat(file_name, &s, MYF(0));
> >> +    my_off_t new_size= s.st_size;
> >
> > do you need it? new_size == pos, isn't it? You yourself use the word
> > "size" to describe pos in sql_print_error just few lines above :)
> 
> Sure. Let turn it into an
> 
>     assert( pos == new_size )
> 
> [x]

and this is, what I think, you'd call "too much checks" :) 
I'd personally would've dropped my_stat and new_size, especially as
I also suggest to remove them from the sql_print_information below.

> >> +
> >> +    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; "
> >> +                          "former file size shrunk from %llu to %llu.",
> >
> > This isn't quite English. Better just "previous (or old) file
> > size: %llu", that is not "former" and no need to specify the new size twice.
> >
> > Also, you say that you "truncated binlog file" here, but on failure
> > you fail to "trim the binlog file". I suggest to use consistent
> > wording, either always "trim" or always "truncate" (and always with
> > or without "the") - both on success and on failure. Personally, I favor
> > "truncate the binlog file"
> 
> some two styles of two writers of these blocks mixed up :-). (Partly
> relates to the size in above).
> 
> [x]
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx