maybe daily dev notes

私の開発日誌

トップ > MariaDB > MariaDBコントリビューション録その2 - builtin_expect による分岐予測

MariaDBコントリビューション録その2 - builtin_expect による分岐予測

MariaDB 開発

前回のあらすじ

初のMariaDBコントリビューションを達成し気をよくした俺。順調に2つ目のIssueもマージされ、浮かれ気分で次なる敵に挑むのであった。

tmokmss.hatenablog.com

今回のIssue

[MDEV-28599] EXCHANGE PARTITION on view causes ER_CHECK_NO_SUCH_TABLE instead of ER_WRONG_OBJECT - Jira

ALTER TABLE v EXCHANGE PARTITION p0 WITH TABLE t2 というクエリが何らかの原因で失敗した際のエラーメッセージを、よりわかりやすいものに変更するという目的のようだ。

Fix Version/s の中で最も古いのは 10.3 なので、そのブランチから作業を始めれば良いらしい。

また、事前情報として perror というコマンドも教えてもらっている。

修正箇所の特定

とりあえず、現行のエラーコードで perror してみる。

$ extra/perror 1177
MariaDB error code 1177 (ER_CHECK_NO_SUCH_TABLE): Can't open table

ER_CHECK_NO_SUCH_TABLE というのがエラー名の模様。おそらくこれが呼び出されている部分が対象の修正箇所なのだろうと予想し、grepする。

数カ所発見。大量に出てくるとかではなくてホッとした。この中から対象箇所を特定するため、Issueを読み返す。

まず EXCHANGE PARTITION というクエリ では、あるテーブルから他のテーブルに指定したPARTITIONを移動する機能らしい。で、この移動先のテーブルがTABLEでなくVIEWだとエラーになるが、その時に表示されるエラーがユーザーからは原因が分かりづらいと。 おそらくは 移動先がVIEWかどうかチェックしている箇所がありそうなので、それを探す。

それらしいのはここしかないのだけど、Viewというワードが出てこないので自信が持てない。当初の予想とは異なり、まだVIEWか否かのチェックは実装されていないのかも？ とりあえず、ここのエラーコードを変えてからテストを実行してみよう。

上のgrep 結果から、対応するテストコードが mysql-test/main/partition_error.test であることは特定済みなので、次のコマンドでテストを実行できる。

$ cd bld/mysql-test
$ ./mysql-test-run.pl main.partition_error

main.partition_error                     [ fail ]
        Test ended at 2022-05-29 03:23:46

CURRENT_TEST: main.partition_error
mysqltest: At line 20: query 'ALTER TABLE t1 EXCHANGE PARTITION p0 WITH TABLE v1' failed with wrong errno 1178: 'The storage engine for the table doesn't support (null)', instead of 1177...

The result from queries just before the failure was:
drop table if exists t1, t2;
#
# Bug#60039: crash when exchanging a partition on
#            nonpartitioned table with a view
#
CREATE TABLE t1 (a int);
CREATE OR REPLACE VIEW v1 AS SELECT * FROM t1;
ALTER TABLE t1 EXCHANGE PARTITION p0 WITH TABLE v1;

エラーコードを 1177から1178 に変えてみたのだが、期待通りテストに失敗してくれた。どうやらここが該当のエラーコードを出力しているところということで間違いないらしい。これで修正箇所を特定できた。

修正方針の検討

最終目標はIssueに書いてある通りで、移動先にVIEWが指定された場合は 1347: 'test.v' is not of type 'BASE TABLE' というエラーを表示すること。 *1

そのためには、次のようなことをやれば良さそう。

  1. 既存のエラーを吐く条件 if (unlikely(!part_table || !table)) の意図を考える
  2. コード上のどこでViewを判定するか考える
  3. 入力がViewかどうか判定する方法を調べる

前のIssueと比べると全然自明じゃない…！とりあえず1から。

if (unlikely(!part_table || !table)) だが、これはただのnullチェックをしているだけ。 現行のエラーはこの条件から生じているので、ナイーブに考えれば、ここの ER_CHECK_NO_SUCH_TABLE を1347に書き換えれば良さそう。しかしそれでは他のケースでも1347エラーになる場合が生じそうで、都合の悪いことがわかる。つまり、移動先がViewであることとtableがnullであることは必要十分条件じゃないだろうという予想。ここは正直コードだけから判断するの至難なので、予想だけしておいて判断はPRレビューに任せることにする。

ということで、この部分はそのまま放置して、ここよりも前の処理で移動先がViewであるかどうかのチェックをする方針に。最悪レビュー受けてからまた調整しましょう。ちなみに今回コードはこの辺りを見ている。

上記の考察を踏まえて、以下のコードを追加した:

...
  // これを追加してみた　ifの条件はTBD
  if (swap_tableがViewなら)
  {
    my_error(ER_WRONG_OBJECT, MYF(0), table_list->db.str,
              swap_table_list->table_name.str, "BASE TABLE");
    DBUG_RETURN(TRUE);
  }

  // 既存のValidation
  if (unlikely(check_exchange_partition(swap_table, part_table)))
    DBUG_RETURN(TRUE);
...

変数 swap_table_list は、移動先のテーブルの情報が入った構造体 (TABLE_LIST) のようなので、これを見てViewかどうか判定すれば良さそう。また、 ER_WRONG_OBJECT は既存のコードを参考にそれっぽく穴埋めして書いている。これも帰納的プログラミング。

tmokmss.hatenablog.com

これで2も完了。あとは3だけで、if文の条件を考えれば良い。 しかしこれもコード読むだけだと正しい判断をするのは難しいと予想*2。一旦少なくともテストケースはパスするコードにしておき、PRレビューで担保してもらうことにする。ちなみにここの条件を間違えていたら、今まで問題なかったクエリでもエラーを吐くことになる。なかなか重要な変更になるぞ…

とりあえずで提出したif条件はこれ。swap_table_list->viewがnullでなければ、移動先がViewであるとみなす。他の箇所でも似たような判定をしていたので、それを流用しただけ。ちなみに詳しい人は｢なぜ likely ？｣と思うかもしれないが、この時点では likely(x)で xが0でないときに真を返すものだと勘違いしていた🤦 。

if (likely(swap_table_list->view))

これで PRを作成。レビューを待つことにしよう。

レビューを受ける

安定安心の nayuta-yanagisawa によるレビュー。以下のようなレビューを受けた。

  1. ifの条件は問題ない
  2. テストケースがいまいちだから新たに追加しておいて
  3. likelyじゃなくunlikelyじゃない？

1はこれで安堵できる。理屈としては、変更の影響範囲はごく狭いため、自動テストによる検証で担保する方針でOKとのこと。

2は順当に追加。たしかによく見ると既存のテストは2つのエラー要因があり、今回のテストケースとしてはふさわしくないものとなっていた。

3が問題のlikely。そもそもの定義は以下のコードで、自分は当初 __builtin_expect(((x) != 0),1) を ((x) != 0) == 1 という意味だと早とちりしていた！ __builtin_expect のようなよくわからない関数はスルーせずしっかりと調べるべきである (教訓)。

#define likely(x)    __builtin_expect(((x) != 0),1)
#define unlikely(x) __builtin_expect(((x) != 0),0)

__builtin_expect の正体はこれ。 評価結果が真と偽のどちらになる可能性が高いかをコンパイラに教えることで、分岐予測による性能を高めることを狙うものであった。今回は swap_table_list->view は偽である可能性が高いので、 unlikely が正しい。英語を考えると likely は起こりやすい、 unlikely は起こりにくいなので、たしかに明らかでした。

ということで全て修正して、無事マージされたのであった 🎉

まとめ

今回のIssueも無事にクローズできた。しかし難易度は徐々に上がっていることを感じる。次回もVSCodeと共にあらんことを。

*1:ちなみにエラーコードを変更する程度であれば、破壊的変更とはみなさないとのこと。

*2:この辺りはMariaDBのドキュメントや資料、あるいはExpert MySQLを読むと分かるかも、とのこと。