MDEV-40417 Fix default value for compressed columns

[M393]

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 ... FORCE would also fix it.

MDEV-40417

[CLAassistant]

All committers have signed the CLA.

[gkodinov]

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.

[M393]

I think there is a misunderstanding.

autoincrement, and default timestamp functions are stored in unireg_check. column compression is also stored there, but it is not a default, that's why it has to be ignored in has_default_function.
Currently has_default_function returns true when column compression is enabled, even without a default, which means all compressed columns have effectively a default value.

MDEV-40417 describes the issue which this PR resolves.
The gist of it is when the column is created without a default, inserts without a value for this column should fail.

[Copilot]

Pull request overview

This PR fixes MDEV-40417 where COMPRESSED NOT NULL columns could incorrectly appear to have an implicit DEFAULT '' due to TMYSQL_COMPRESSED being stored in unireg_check and mistakenly treated as a “default function”. It also adds an FRM-load repair path for affected legacy tables (limited to BLOB/TEXT where the FRM can distinguish explicit defaults).

Changes:

• Treat Field::TMYSQL_COMPRESSED as not being a default function when determining whether a NOT NULL column lacks a default.
• Repair legacy FRMs on load by setting NO_DEFAULT_VALUE_FLAG for COMPRESSED NOT NULL BLOB/TEXT columns that provably had no explicit DEFAULT.
• Add/extend mysql-test coverage, including an upgrade scenario using a pre-fix .frm.

Reviewed changes

Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
sql/table.cc	Adds FRM-load repair for legacy compressed BLOB/TEXT columns missing the no-default flag.
sql/field.h	Updates has_default_function() to ignore TMYSQL_COMPRESSED.
sql/field.cc	Uses has_default_function() in default/NOT NULL validation to avoid treating compression as a default.
mysql-test/main/column_compression.test	Adds regression + upgrade tests for MDEV-40417 behavior.
mysql-test/main/column_compression.result	Updates expected outputs for the new/changed test cases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gkodinov]

Indeed. I was a bit too hasty to speak before throughly understanding the change. Sorry for that. And thank you for your patience.

I've taken the time to study this now to the best of my abilities.

I believe that indeed the changes to field.[cc|h] are OK.

But I have an issue with the change in table.cc: I do not think it's a good idea to keep "garbled" .frms on disk that we can repair and silently just ignore the broken parts at load. I'd do it as follows: if there's a controversy in how the FRM is (as in: it's garbled), I'd report it in the error log and optionally mark the table as crashed.
IMHO the actual repair should happen in e.g. ALTER TABLE ... FORCE and be done once and not on every load. This said, I do not mind the workaround you did completely, but it needs a more permanent ways to fix this situation in the FRM itself IMHO.

However this is all stuff for the final reviewer to decide on it seems.

Hence please just focus on making up your mind on how exactly do you think this should be fixed and merging the two commits to match that.

[M393]

Thank, you. I added a warning to the log when a column with buggy implicit default is encountered.

Currently the table is silently patched on every load. Which means some inserts that worked with a previous version may now produce an error when they are not explicitly set in the insert statement.

Also it may be advised to mention in the upgrade notes that you need to manually update any compressed varchar / varbinary columns that should not have a default value.

This query shows all columns that may have an unwanted default, not automatically detectable by MariaDB.

SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, COLUMN_TYPE
FROM information_schema.COLUMNS
WHERE COLUMN_TYPE LIKE 'var%COMPRESSED%'
  AND IS_NULLABLE = 'NO'
  AND COLUMN_DEFAULT = ''''''
  AND TABLE_SCHEMA NOT IN ('mysql','information_schema','performance_schema','sys')
ORDER BY TABLE_SCHEMA, TABLE_NAME, ORDINAL_POSITION;

As a side effect this query should cause MariaDB to log all affected tables which can be fixed with ALTER TABLE ... FORCE, if that's how it is merged.

grep 'implicit DEFAULT' error.log | sort -u

[M393]

Alternatives and recommendation as per Claude:

Route 1:

patch the bytes in place — possible, and there's precedent

Everything needed is already at hand. TABLE_SHARE::write_frm_image() (table.cc:3630) exists, and init_from_binary_frm_image() itself already writes the image back when its write parameter is true (table.cc:1829) — that's how CREATE TABLE persists an FRM. The normal open path passes write=false (table.cc:737). Even the surgical variant has precedent: update_frm_version() (handler.cc:4975) does a 4-byte pwrite at offset 51 to stamp the version. Ours would be one bit — FIELDFLAG_NO_DEFAULT in the field's 2-byte pack_flag.

But doing it from inside init_from_binary_frm_image() would be wrong, because that function runs on a plain SELECT under a shared MDL. It would mean writing to the schema during a read, and it breaks in every context where that isn't allowed: --read-only, replicas, read-only filesystems, a mariadb-backup snapshot. There's also no replication of the change (Galera nodes would silently diverge), and a torn write leaves an unopenable table. Note that update_frm_version() avoids all of this by running only from mysql_admin_table after a successful CHECK TABLE — under an admin statement's locking, not a reader's — and it deliberately refuses when keep_original_mysql_version is set (handler.cc:4993-4995).

Route 2:

mark it as needing upgrade — the idiomatic one

CHECK TABLE ... FOR UPGRADE calls handler::ha_check_for_upgrade() ([handler.cc:4911](vscode-webview://0hi93marjqtcegb1kl26eoi6khc7dqabi28vb3afi1j0b3k5528d/sql/handler.cc#L4911)), which is already a composition of exactly this kind of check, each returning HA_ADMIN_NEEDS_ALTER: old types, pre-5.0.3 varchar, collation compatibility, and — the direct analogue — check_long_hash_compatibility() ([handler.cc:4880](vscode-webview://0hi93marjqtcegb1kl26eoi6khc7dqabi28vb3afi1j0b3k5528d/sql/handler.cc#L4880)). That last one is MDEV-27653: a wrong value written by an older server that only a rebuild can fix, and its comment even explains choosing HA_ADMIN_NEEDS_ALTER over NEEDS_UPGRADE so the user gets ER_TABLE_NEEDS_REBUILD. Same situation, same shape.

Adding a check_compressed_default_compatibility() alongside it means mariadb-check --check-upgrade --all-databases — i.e. what mariadb-upgrade runs, and what operators already run after upgrading — issues the ALTER TABLE ... FORCE itself. The FRM is then rewritten through the normal path: correct locking, crash-safe, replicated, version bookkeeping handled. That is "automatic" in the way that matters, and it also solves the completeness problem from your last question, since --all-databases opens everything rather than waiting for someone to touch each table.

One wrinkle: after the in-memory repair the field is indistinguishable from a healthy one, so the check needs a breadcrumb — a bool on TABLE_SHARE set where the repair happens. I'd add a dedicated one rather than reuse share->incompatible_version, because that field is a mask of HA_CREATE_USED_* bits with a heavy side effect: it makes open_table_from_share() fail outright with OPEN_FRM_NEEDS_REBUILD (table.cc:4221-4227) for anything but ALTER/REPAIR/FLUSH. That would turn a working table into an unusable one until upgraded — far too aggressive for a phantom default, and there's no sensible HA_CREATE_USED_* bit for a column default anyway.

Route 3:

current state — warn and let the DBA decide

Where we are now. But possibly no automatic patching of loaded tables if that seems too risky?

Recommendation:

route 2. It's ~15 lines plus a share flag, needs no new tooling, and it's the mechanism MariaDB already uses for "an old server wrote something wrong; only a rebuild fixes it". Route 1 I'd argue against regardless of effort. Worth noting it only covers the blob half — the varchar half stays a review list either way, since nothing on disk marks those as affected.

[gkodinov]

These are all questions for the final review. Please squash the commits so I can get this over to the final review stage.

[gkodinov]

LGTM. Please stand by for the final review.