Skip to content
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

importccl: IMPORT MYSQLDUMP failure outputs broken character #29348

Closed
rolandcrosby opened this issue Aug 30, 2018 · 3 comments · Fixed by #30386
Closed

importccl: IMPORT MYSQLDUMP failure outputs broken character #29348

rolandcrosby opened this issue Aug 30, 2018 · 3 comments · Fixed by #30386
Assignees
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@rolandcrosby
Copy link

Testing with this MySQL dump file I found on GitHub, CockroachDB gave me this error:

root@:26257/defaultdb> import mysqldump ('https://raw.githubusercontent.com/artmarkov/smart.loc/50b43d5640dcfcc38f2c7db22f04982e43b93a09/backup/db/db__2017-11-19_16-38-09.sql');
pq: expected a table key, had �: insufficient bytes to decode uvarint value: ""
@rolandcrosby rolandcrosby added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-disaster-recovery labels Aug 30, 2018
@maddyblue
Copy link
Contributor

This is a fun one. If I open that file in my editor and save it, the error goes away. There's a weird byte in here somewhere.

@maddyblue
Copy link
Contributor

maddyblue commented Sep 18, 2018

Ok I did something wrong above and that comment is completely untrue.

The problem is that when importing a large number of tables (>=56), regardless of pgdump or mysqldump, we have a table key decoding problem.

When determining split points, we merge two lists: 1) all of the sampled keys, 2) all of the start and and keys of spans of all table and sequence descriptors being added. Assume that 100 empty tables (i.e., a PGDUMP file with 100 CREATE TABLE statements) are being imported. We start our numbering at 55, so tables with ID 55 to 154 will be created. We start with table ID 55 and get its descriptor span: /Table/5{5-6}, or in bytes: start: "\xbf", end: "\xc0". The end span is computed by making the start key for a table and calling PrefixEnd on it. PrefixEnd doesn't know anything about table IDs, it just works on bytes and finds the next byte, hence bf -> c0. We can do this for all the tables. Great.

Now move on to the SST writer. When writing SSTs we do a check when done writing them to see if we are at the end of a table's SSTs. If we are, we mark the entire table range as done so its progress contribution is complete (and it isn't imported again if we have to restart). This works by taking the split point key and, assuming it's a valid table key prefix, extracts out the table ID. However table ID 109 has an end span that's not a valid table key prefix (because of the blind call to PrefixEnd described above). For example, the following code:

	for i := sqlbase.ID(107); i <= 111; i++ {
		desc := sqlbase.TableDescriptor{ID: i}
		fmt.Println(desc.TableSpan())
	}

prints

/Table/10{7-8}
/Table/10{8-9}
/Table/109{-/PrefixEnd}
/Table/11{0-1}
/Table/11{1-2}

So when the SST writer tries to figure out the table ID on table 109 it encounters an error because it's not a valid table prefix. This happens because it wasn't properly encoded with encoding.EncodUvarintAscending, which does (I guess?) other stuff above that.

We may need to change sqlbase/TableDescriptor.TableSpan to not use PrefixEnd in EndKey, but instead something like:

func (desc *TableDescriptor) TableSpan() roachpb.Span {
	return roachpb.Span{
		Key:    roachpb.Key(keys.MakeTablePrefix(uint32(desc.ID))),
		EndKey: roachpb.Key(keys.MakeTablePrefix(uint32(desc.ID + 1))),
	}
}

The start key for the table span of table 109 is \xf5. Using the PrefixEnd method, this currently produces \xf6 as the end key, when it should actually be \x6n.

@maddyblue
Copy link
Contributor

Luckily this function is only used in IMPORT, so the impact is pretty small I think?

craig bot pushed a commit that referenced this issue Sep 19, 2018
30386: importccl: correctly determine start span for resume r=mjibson a=mjibson

When writing the "done spans" list, use the min key instead of trying
to determine the table start key. Same for at the end. This removes the
possibility of misusing the table span end key as described below. We
may end up changing TableDescriptor.TableSpan, but the change here is
guaranteed to be correct regardless of that.

--

The problem is that when importing a large number of tables (>=56),
regardless of pgdump or mysqldump, we have a table key decoding problem.

When determining split points, we merge two lists: 1) all of the sampled
keys, 2) all of the start and and keys of spans of all table and sequence
descriptors being added. Assume that 100 empty tables (i.e., a PGDUMP
file with 100 `CREATE TABLE` statements) are being imported. We start our
numbering at 55, so tables with ID 55 to 154 will be created. We start
with table ID 55 and get its descriptor span: `/Table/5{5-6}`, or in
bytes: `start: "\xbf", end: "\xc0"`. The end span is computed by making
the start key for a table and calling PrefixEnd on it. PrefixEnd doesn't
know anything about table IDs, it just works on bytes and finds the next
byte, hence `bf` -> `c0`. We can do this for all the tables. Great.

Now move on to the SST writer. When writing SSTs we do a check when done
writing them to see if we are at the end of a table's SSTs. If we are,
we mark the entire table range as done so its progress contribution
is complete (and it isn't imported again if we have to restart). This
works by taking the split point key and, assuming it's a valid table
key prefix, extracts out the table ID. However table ID 109 has an end
span that's not a valid table key prefix (because of the blind call to
PrefixEnd described above). For example, the following code:

```
	for i := sqlbase.ID(107); i <= 111; i++ {
		desc := sqlbase.TableDescriptor{ID: i}
		fmt.Println(desc.TableSpan())
	}
```

prints

```
/Table/10{7-8}
/Table/10{8-9}
/Table/109{-/PrefixEnd}
/Table/11{0-1}
/Table/11{1-2}
```

So when the SST writer tries to figure out the table ID on table 109 it
encounters an error because it's not a valid table prefix. This happens
because it wasn't properly encoded with encoding.EncodUvarintAscending,
which does (I guess?) other stuff above that.

Fixes #29348

Release note (bug fix): fix IMPORT of empty or small tables under rare
conditions.

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
@craig craig bot closed this as completed in #30386 Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants