MDEV-16490: It's possible to make a system versioned table without an… #803
Conversation
…y versioning field * do not allow versioned table to be without versioned (non-system) fields * prohibit changing field versioning, when removing table versioning
FooBarrior
force-pushed
the
tt-10.3-MDEV-16490
branch
from
7cd78d7
to
c1bf576
Compare
There was a problem hiding this comment.
Hi @FooBarrior, sorry it took me so long to get to this. The patch looks excellent, I have some minor suggestions inline, otherwise the code is ok to push. Thanks!
| goto err; | ||
| uint versioned_fields= 0; | ||
| field_it.rewind(); | ||
| while (Create_field *f= field_it++) |
There was a problem hiding this comment.
Maybe early return the loop if we already counted more than VERSIONING_FIELDS?
There was a problem hiding this comment.
It will bring unnecessary complications. In particular, the value of versioned_fields will be inconsistent after the loop. This can confuse somebody else later.
| @@ -8473,11 +8473,21 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, | |||
| } | |||
| } | |||
|
|
|||
| if (table->versioned() && !(alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) && | |||
| new_create_list.elements == VERSIONING_FIELDS) | |||
| if (table->versioned()) | |||
There was a problem hiding this comment.
I would appreciate a lot if you could try to move this new code into a helper function, the size of mysql_prepare_alter_table is already astronomical.
There was a problem hiding this comment.
Did it with a new version
| @@ -7039,7 +7039,7 @@ bool Vers_parse_info::fix_alter_info(THD *thd, Alter_info *alter_info, | |||
|
|
|||
| if (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) | |||
| { | |||
| if (!share->versioned) | |||
| if (!share->versioned || create_info->versioned()) | |||
There was a problem hiding this comment.
Add a comment "Prohibit changing field versioning, when removing table versioning".
|
Hi, @robertbindar! But anyway, here it is: https://github.com/MariaDB/server/commits/bb-10.3-MDEV-16490 The aforementioned fix also naturally covers your request for moving the code in a separate function |
|
Hi @FooBarrior! I left some comments in the commits in bb-10.3-MDEV-16490, great work on handling the create..select case, thanks! |
|
Thank you for the new review, @robertbindar! |
|
Hi @FooBarrior! This should be ready to be closed right? Also any clarification here about the partition fix you submitted will be highly appreciated. |
|
Hi! Yes, it could be closed now. The partition test fails because of another concurrent improvement by MDEV-15408. I have just adjusted the code to be consistent for now, but there is MDEV-20507 to make diagnostics correct in case of partitioning. |
…y versioning field