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

Ensure generate_schema EXACTLY the same as SHOW CREATE TABLE in MySQL #237

Open
wallneradam opened this issue Nov 13, 2019 · 14 comments
Open
Labels
enhancement New feature or request

Comments

@wallneradam
Copy link
Contributor

Describe the bug
As you may know, I'm working on a schema sync to MySQL which should work with Tortoise ORM. For this to work good the schema that Tortoise generate should be as close to the result of SHOW CREATE TABLE as possible. Now it works quite well, though I still have a problem with unique_together.

To Reproduce
See the example in #215 .
The generated SQL is semantically good, though MySQL will give a name to the unique key, which is the name of the 1st field it uses. But if you have another key with that name, it will add a number to it. So it is not deterministic enough.

Expected behavior
It should look like this:

UNIQUE KEY `key_user_id_62ed175d` (`key`, `user_id`)

Additional context
Also the full command has the KEY keyword, tortoise generates code without it, though I could resolve it with regexp in my schema sync.

I still need a similar custom schema generator described in #215 to solve this.

@grigi
Copy link
Member

grigi commented Nov 13, 2019

That's easy enough to do. Thank you for the very clear requirement

@grigi grigi added the enhancement New feature or request label Nov 13, 2019
@grigi
Copy link
Member

grigi commented Nov 25, 2019

Wow, how did I miss this for the 0.15.0 release?

@grigi
Copy link
Member

grigi commented Nov 25, 2019

Essentially this?

UNIQUE KEY `idx_event_name_c6f89f` (`name`, `prize`)

I'm just busy trying to get it to work right for the other DB's as that seems entierly MySQL specific.

@grigi
Copy link
Member

grigi commented Nov 25, 2019

Ok, I confirmed that is right for MySQL. I now also noticed when I do a SHOW CREATE TABLE unique columns are changed, they are represented like this:

`token` varchar(100) NOT NULL COMMENT 'Unique token',
UNIQUE KEY `token` (`token`),

instead of this:

`token` VARCHAR(100) NOT NULL UNIQUE COMMENT 'Unique token'

Is this a problem for you? Should I make it look exactly like the SHOW CREATE TABLE instead?

@wallneradam
Copy link
Contributor Author

No, it is not problem, because my script tries to convert it to the default MySQL dialect. Though in my opinion a long (or not so long) term goal should be to make SchemaGenerators to create exactly the same (or very close) as the engines' DDL.

Thanks for your work!

grigi added a commit that referenced this issue Nov 27, 2019
…auto-assigning a potentially non-unique constraint name. (#237)
@grigi
Copy link
Member

grigi commented Nov 27, 2019

Ok, the next version should have the fix. I didn't get around to doing the "make generate_schema EXACTLY the same as SHOW CREATE TABLE in MySQL" yet.
I feel it is a laudable target, but not immeditely. I'll add it as a requirement for v1.0

@grigi grigi mentioned this issue Nov 27, 2019
72 tasks
grigi added a commit that referenced this issue Nov 27, 2019
…auto-assigning a potentially non-unique constraint name. (#237)
@grigi
Copy link
Member

grigi commented Nov 27, 2019

Release v0.15.2 should have the fix for this. Please test and confirm.

@grigi grigi changed the title unique_together field unique name Ensure generate_schema EXACTLY the same as SHOW CREATE TABLE in MySQL Nov 28, 2019
@wallneradam
Copy link
Contributor Author

Thanks, it is solved the original issue.

@grigi
Copy link
Member

grigi commented Dec 12, 2019

Hi, I noticed that on MySQL TEXT and BLOB is limited to 65534 Bytes (2B less than 64k).
Which, honestly, is kinda small.
Considering that they get stored in their own extent, and that upgrading them to MEDIUMTEXT/MEDIUMBLOB would allow just under 16MB (less 3B). Or should we do HUGETEXT/HUGEBLOB (4GN-4B)
The storage limit for PG/SQLite is much higher at least as big as the HUGE variants.
Somehow I feel MEDIUM is better than HUGE, as we don't want to encourage people storing too large objects in the db.

What do you think?

@grigi
Copy link
Member

grigi commented Dec 12, 2019

Oh, no. The aiomysql driver chokes the moment we go 1 byte over 128k... 😭
It literally has a buffer overflow...
So a MEDIUM only buys us a doubling of the field size, another 128x to go...
Maybe we should just leave it at the 64k objects?

@grigi
Copy link
Member

grigi commented Dec 12, 2019

Right, its 128k for all the parameters added up. So adding more parameters eats into that 128k :-(
The aiomysql driver resets some counters on the connection, but the server is confused as it thinks the connection has not been reset. So nothing works on that connection afterwards, with messages like

Packet sequence number wrong - got 199 expected 1

😞

@wallneradam
Copy link
Contributor Author

Somehow I feel MEDIUM is better than HUGE, as we don't want to encourage people storing too large objects in the db. What do you think?

What about allowing the user to specify it? Most of the time users use simple TEXT fields (64K), it is for storing product descriptions or things like that, for them 64k is more than enough. So I would make this the default. It could be configurable by fields or by connection parameter. May be the latter would better preserve the general fields of Tortoise.

If you want to generalize the behavior between engines, LONGTEXT is a good choice. It has 4 bytes overhead per record, which is not a big deal nowdays. Only 4MB per 1M records. It is a way less than storing data on filesystems (e.g. NTFS).

I like about Tortoise that it is not overcomplicated and beeing something general across database engines may be better than having a lot of not so important unique database features. And if I need something like that I could easily override tortoise classes (because it is not overcomplicated).

Oh, no. The aiomysql driver chokes the moment we go 1 byte over 128k

I don't really understand this. Is it a bug in aiomysql? Ir is it an implementation problem of it?
Is it this issue? aio-libs/aiomysql#450

@grigi
Copy link
Member

grigi commented Dec 12, 2019

Yes, The same bug. It only happens with aiobysql that I see so far. The other DB backends happily do a lot more.

Well, I was wondering just changing the default so that it works with a good default. I mean if the standard size limit of SQLite is 2GB, and PostgreSQL is 16EB, then MySQL at 64KB seems a bit out of place. I'd rather up it to 4GB and just use 2 extra bytes in the header. Seems very cheap to me, and I don't see the need to ever use plain ol TEXT for this.

So options are:

  1. Stay at 64KB (for MySQL)
  2. Change to 4GB (for MySQL)
  3. Make it configurable via a hint (once again, only for MySQL)

@wallneradam
Copy link
Contributor Author

I agree with you, 64KB is small for a lot of tasks. MySQL is the oldest engine of them, and it can work on very low end hw, this is why they leave it configurable this way.

Though I can't imagine a situation where it is useful to store a whole DVD in a DB column. It would be very slow. These are only theoretical values. It is useful to store pictures, PDF-s and similar things in DB columns which is very common. So I think option 2 is the way to go, which makes tortoise-mysql similar to other engines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants