-
Notifications
You must be signed in to change notification settings - Fork 481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PS-5035 : rocksdb.show_table_status: 1051: Unknown table 'db_new' #2712
Conversation
jenkins https://ps.cd.percona.com/view/5.7/job/percona-server-5.7-param/29/ |
sql/partitioning/partition_base.cc
Outdated
// this 'name' and turn it into a proper fully qualified path to the .frm file | ||
// we are doing a little copy/cut-n-paste from build_table_filename that | ||
// composes the full path but without the additional encoding. | ||
char path[FN_REFLEN + 1]; //< Path to .FRM file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at build_table_filename, it looks like it should be reasonable to extract this to a separate function to be called both here and from build_table_filename. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually @laurynas-biveinis, my first idea to fix this was to add another flag to build_table_filename (maybe something like FN_NO_ENCODE) that would force it to use my_stpnmov instead of tablename_to_filename for both the database and table name, but, I did not want to go hacking about the server code without consulting you. Now it seems that we are both thinking along the same line.
0ac0256
to
4b2ac7d
Compare
- This is not a simple test failure, it is a regression introduced as a part of the native partitioning implementation. The handler::delete_table method takes in a 'database.table' name in the expanded/converted format of the file system character set or fscs. The character set has file system special characters expanded into '@xxxx' format so a '.' in a database or table name is represented as a '@002e' in the expanded name. The delete_table method needs to determine if the table is partitioned or not. It does this through the helper function native_part::get_part_str_for_table in sql/partitioning/partition_base.cc by parsing, then building the filename of the .frm file so that it can open and read partition info. This parsing makes use of the function build_table_filename in sql/sql_table.cc. The build_table_filename function expects that the given database and table names are in system charset and again, encodes these into the fscs, therefore double encoding the database and table name into something that does not exist and can not be opened or read from. This results in delete_table returning ERR_TABLE_CORRUPT to the server which then complains that it can't DROP the database because it can't properly remove all of the tables within. - Since there seems to be no other obvious helper function to take the database.table name in fscs encoding and turn it into a proper fully qualified path to the .frm file, we need to extend the flag set and functionality of build_table_filename that allows us to compose the full path but without the additional encoding. - This is already covered by the test which revealed the issue within rocksdb.show_table_status so no new test is implemented.
4b2ac7d
to
53371b0
Compare
OK, introduced new flag for build_table_filename and reverted copy-n-paste. New jenkins https://ps.cd.percona.com/view/5.7/job/percona-server-5.7-param/33/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Is it possible to package this in some way the upstream would accept a bug report?
I certainly could, but I know, 100%, based on past experiences with things like this, it will go ignored :-\ |
This is not a simple test failure, it is a regression introduced as a part of
the native partitioning implementation.
The handler::delete_table method takes in a 'database.table' name in the
expanded/converted format of the file system character set or fscs. The
character set has file system special characters expanded into '@xxxx' format
so a '.' in a database or table name is represented as a '@002e' in the
expanded name.
The delete_table method needs to determine if the table is partitioned or not.
It does this through the helper function native_part::get_part_str_for_table
in sql/partitioning/partition_base.cc by parsing, then building the filename
of the .frm file so that it can open and read partition info.
This parsing makes use of the function build_table_filename in
sql/sql_table.cc. The build_table_filename function expects that the given
database and table names are in system charset and again, encodes these into
the fscs, therefore double encoding the database and table name into something
that does not exist and can not be opened or read from.
This results in delete_table returning ERR_TABLE_CORRUPT to the server which
then complains that it can't DROP the database because it can't properly
remove all of the tables within.
Since there seems to be no other obvious helper function to take the
database.table name in fscs encoding and turn it into a proper fully qualified
path to the .frm file, we need to extend the flag set and functionality of
build_table_filename that allows us to compose the full path but without the
additional encoding.
This is already covered by the test which revealed the issue within
rocksdb.show_table_status so no new test is implemented.