MDEV-40417 Fix default value for compressed columns - #5484
Conversation
…LT '' Compression is remembered by storing Field::TMYSQL_COMPRESSED in Column_definition::unireg_check, but has_default_function() treated every unireg_check other than Field::NONE as "this column has a default function". A COMPRESSED NOT NULL column without an explicit DEFAULT therefore never got NO_DEFAULT_VALUE_FLAG, neither in mysql_prepare_create_table() nor in Column_definition::check(). As FIELDFLAG_NO_DEFAULT was not set either, the missing flag was written into the FRM and the column silently became optional: CREATE TABLE t (c LONGTEXT COMPRESSED NOT NULL) ENGINE=InnoDB; INSERT INTO t () VALUES (); -- succeeded SHOW CREATE TABLE also displayed a phantom DEFAULT '' for such a column. Exclude TMYSQL_COMPRESSED in has_default_function(), and use that method in Column_definition::check() instead of the open-coded unireg_check comparison, so that both places which set NO_DEFAULT_VALUE_FLAG cannot drift apart again. Note that tables created before this fix keep the wrong pack_flag in their FRM until they are rebuilt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…LT '' Repair pre-existing tables when their FRM is read. The previous commit fixed the write path only, so a table created before it keeps a pack_flag without FIELDFLAG_NO_DEFAULT, and a COMPRESSED NOT NULL column of such a table still behaves as if it had DEFAULT '': 10.11.19 > CREATE TABLE t (c LONGTEXT COMPRESSED NOT NULL); 10.11.19+ > INSERT INTO t (other_column) VALUES (1); -- still succeeded Restore the flag in TABLE_SHARE::init_from_binary_frm_image() for blob columns. An explicit DEFAULT of a blob column is always stored in the FRM as an expression, see Column_definition::has_default_expression(), so a COMPRESSED NOT NULL blob that has no default_value provably had no DEFAULT clause. The check is a no-op for FRMs written after the fix, where the flag is present already, so no version condition is needed. VARCHAR and VARBINARY are deliberately not repaired. A constant DEFAULT of a non-blob column is stored in the default record, exactly like the wrong implicit default, which makes the FRMs of c VARCHAR(100) COMPRESSED NOT NULL c VARCHAR(100) COMPRESSED NOT NULL DEFAULT '' byte for byte identical. Repairing them would be a guess, and a wrong guess would start rejecting INSERTs against tables whose DEFAULT '' was intentional. Such columns can still be corrected explicitly with ALTER TABLE ... MODIFY. std_data/MDEV-40417.* is a MyISAM table created by 10.11.19 before the fix, used by the new test to cover reading an old FRM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Please merge the two commits into one.
Also, please add a design description of the fix:
- approach rationale and summary
- detailed design notes
- what functionality is changed
- what is supposed to work
- what is supposed to fail etc.
I also do not quite subscribe to the premise of the fix: I believe that whether a column has a default or not should not depend on other column attributes. Thus, to me it's weird to always require having a default for compressed columns.
I would consider fixing the bug differently: I'd make the DEFAULT clause independent from the COMPRESSED attribute. And then offer upgrade advice for tables that are binary and have a default value, but do not have the flag on in FRM.
Please at least explain why you've taken the approach you did.
|
I think there is a misunderstanding. autoincrement, and default timestamp functions are stored in MDEV-40417 describes the issue which this PR resolves. |
3 participants
Column compression is stored in unireg_check and has to be ignored when checking for a default function.
For VARCHAR / VARBINARY there is no way to know whether an empty string default was explicitly set or is the result of this bug.
For TEXT / BINARY this can be automatically repaired when loading the table (second commit here),
but an
ALTER TABLE ... FORCEwould also fix it.MDEV-40417