[midenok]

Please, review.

[CLAassistant]

All committers have signed the CLA.

[gemini-code-assist]

Code Review

This pull request improves the ALTER TABLE FORCE syntax and implements automatic partitioning of pre-existing historical data for system-versioned tables (MDEV-25529). Key changes include adding high-precision timestamp helpers, updating parser rules to allow flexible ordering of partitioning clauses, and introducing logic to calculate the required history partitions based on the table's actual historical time range. The feedback suggests renaming the generic cmp function in my_time.h to avoid global namespace collisions, removing a temporary TODO comment, initializing best_idx to prevent compiler warnings, and simplifying the partition calculation logic in vers_calc_hist_parts.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Copilot]

Pull request overview

This PR addresses MDEV-25529 by ensuring that when ALTER TABLE ... PARTITION BY SYSTEM_TIME ... AUTO is used on a system-versioned table, pre-existing historical rows are accounted for when auto-creating history partitions (including adjusting STARTS when needed). It also expands ALTER TABLE grammar around FORCE + partitioning clauses and adds supporting timestamp/time utilities plus new regression tests.

Changes:

• Derive historical ROW END timestamp range (index-first, scan fallback) and use it during versioning partition auto-setup on ALTER to size partitions and/or adjust STARTS.
• Add my_timespec_t and session-timezone timestamp formatting helpers used in warnings/debug paths.
• Extend parser grammar for more ALTER TABLE clause orderings and add/refresh mysql-test coverage for both MDEV-25529 behavior and FORCE syntax.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
sql/table.h	Declares TABLE::vers_get_history_range() for versioned-table history range discovery.
sql/sql_yacc.yy	Extends ALTER TABLE grammar to allow additional orderings for partitioning/remove partitioning with other alter clauses.
sql/sql_type.h	Declares Temporal_hybrid(THD*, my_timespec_t) ctor for high-precision timestamps.
sql/sql_type.cc	Implements Temporal_hybrid(THD*, my_timespec_t) conversion via session time zone.
sql/sql_time.h	Adds interval2sec() / interval2usec() helpers to replace macro-based interval math.
sql/sql_time.cc	Switches interval microsecond calculations to the new helpers.
sql/sql_table.cc	Minor refactor in duplicate-key check predicate naming; adds include needed for key utilities.
sql/sql_partition.cc	Main logic: on relevant ALTER, compute history range, adjust STARTS, and compute needed history partitions (incl. month/year stepping).
sql/sql_class.h	Adds THD::timestamp_to_string() overloads and TimestampString helper for lazy formatting.
sql/sql_class.cc	Implements THD::timestamp_to_string() using Temporal_hybrid and marks TIME_ZONE_USED.
sql/share/errmsg-utf8.txt	Reworks/introduces warnings related to STARTS and slow history scanning; adds new warning messages.
sql/partition_info.h	Tracks whether STARTS was explicitly provided; adds vers_set_starts() API.
sql/partition_info.cc	Implements vers_set_starts() rounding; improves interval warning message; adds OOM error on default partition element allocation.
sql/field.h	Adds Field_vers_trx_id::get_timestamp() override declaration.
sql/field.cc	Implements transaction-registry timestamp lookup for TRX_ID-versioned fields.
sql/event_data_objects.cc	Expands documentation comment for interval scheduling behavior.
mysql-test/suite/versioning/t/partition2.test	Adds comprehensive regression test for MDEV-25529 (index vs scan paths, STARTS adjustments, month/year intervals, etc.).
mysql-test/suite/versioning/r/partition2.result	Expected output for the new versioning partition regression test.
mysql-test/suite/versioning/r/partition.result	Updates expected warnings text for STARTS-beyond-query messages.
mysql-test/main/partition.test	Adds FORCE syntax regression coverage for ALTER TABLE partitioning/remove partitioning.
mysql-test/main/partition.result	Expected output for new FORCE syntax test cases.
include/my_time.h	Introduces my_timespec_t, ordering operators, and min/max constants for high-precision timestamps.

---

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

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

[sanja-byelkin]

mreview report

Verdict: request-changes

Summary

• Tier: standard
• Blockers: 2
• Important: 35
• Nits: 33
• Praise: 9

Blocker

1. • Blocker [sql/sql_partition.cc:6164-6169] The new history-range block is guarded by vers_info->auto_hist but never checks vers_info->interval.is_set(), so PARTITION BY SYSTEM_TIME LIMIT n AUTO on a table that already has history also enters it. In that case vers_set_starts() writes an interval start for a LIMIT scheme, and vers_calc_hist_parts() reads interval.step. The Vers_part_info constructor never initialises step (only interval.type= INTERVAL_LAST), so this is an uninitialised read. If the step bytes are 0, the code hits DBUG_ASSERT(i_sec > 0) and then divides range / i_sec by zero in release builds. If they are garbage, it computes a nonsense partition count or runs the month/year loop. The test only runs limit 1 auto on an empty table. Fix: add part_info->vers_info->interval.is_set() to the condition and add a test with LIMIT AUTO plus existing history. (bug) — by code-reviewer
2. • Blocker [sql/sql_partition.cc:6288] vers_calc_hist_parts() is off by one when max_ts lands exactly on an interval boundary. It computes ceil(range / i_sec), but partition k covers [start + k*i, start + (k+1)*i), so a row with row_end == start + k*i needs k+1 history partitions. The month/year loop has the same bug (while (start < end)). Today such a row silently overflows into the last history partition, which is the defect this MDEV is meant to fix. The test even records it: in partition2.result, alter table t1 PARTITION by system_time interval 1 hour auto gives PARTITIONS 3 and puts x=3 (row_end 02:00:00.000000) into p1 = [01:00, 02:00), while the explicit 6-partition layout just above puts x=3 into p2. Fix: *hist_parts= range / i_sec + 1, and while (start <= end) in the calendar loop. The end == start special case then becomes redundant. Re-record the result. (S4 / logging-and-errors.md: "Tell the truth") — by silent-failure-hunter

Important

1. • Important [git 5478199] The commit body of "MDEV-25529 Comments" lists "* THD::used comment", but the commit only touches sql/event_data_objects.cc. No THD::used comment is added anywhere in the PR. (commit-and-process.md:153, PR/commit message must match the patch; S3) — by comment-analyzer
2. • Important [git 5f8310e] The commit "MDEV-25529 TimestampString for printing timestamps" has no body. It also silently renames the public error symbol ER_PART_STARTS_BEYOND_INTERVAL to WARN_VERS_STARTS_BEYOND_INTERVAL and changes its message format (new %s arguments). The libmariadb submodule still defines the old name (libmariadb/include/mysqld_error.h:1214). A user-visible rename like this must be stated in the commit message. (commit-and-process.md:Commit message, "body explains what ... the fix does"; S3) — by comment-analyzer
3. • Important [include/my_time.h:310] The new my_timespec_t {my_time_t sec; ulong usec;} copies the existing struct my_timeval {tv_sec; tv_usec} (include/my_time.h:133) and its C++ wrappers Timeval (sql/structs.h:1078, which already asserts usec < 1000000) and Timestamp (sql/sql_type.h:2858, which already has cmp() and to_datetime(thd)). The PR should reuse Timestamp/Timeval rather than add a third seconds+microseconds type that has fewer invariants (S2) — by type-design-analyzer
4. • Important [include/my_time.h:317] A file-scope static inline int cmp(my_timespec_t, my_timespec_t) is placed inside C_MODE_START/END in a header that C files and installed server/plugin headers (include/mysql/server/) also use. C has no overloading, so any C translation unit that defines its own cmp (strings/dtoa.c, unittest/mysys/queues-t.c and ma_dtoa.c already do) and includes my_time.h will fail to compile. Rename it to my_timespec_cmp(). Two bots flagged this and the author dismissed it, but the collision risk is real. (cited from coding-style.md:Naming, "Watch for system-macro collisions") — by code-reviewer (also flagged by type-design-analyzer)
5. • Important [mysql-test/suite/versioning/t/partition2.test:129] --echo # STARTS doesn't change without FORCE is misleading. SHOW CREATE right after it shows STARTS changed from 2000-01-01 to the user's '2005-01-01'. What does not happen is the adjustment to the history range. Reword to "STARTS is not adjusted to the history range without FORCE". This line lands in the .result file. (S3) — by comment-analyzer
6. • Important [mysql-test/suite/versioning/t/partition2.test:230] The new test file has no --echo # End of 13.0 tests footer. The block added to mysql-test/main/partition.test after --echo # End of 10.6 tests also lacks an End of 13.0 tests marker. (cited from testing.md:Headers and footers; checklist.md:Test footer) — by code-reviewer
7. • Important [mysql-test/suite/versioning/t/partition2.test:77] The test comment # Last history: 2000-01-01 02:00:00.00000021 is factually wrong. The recorded row_end is 2000-01-01 02:00:00.000000, and the value has 8 fractional digits, which TIMESTAMP(6) cannot hold. (S3) — by comment-analyzer
8. • Important [sql/partition_info.cc:2860-2861] WARN_VERS_STARTS_BEYOND_INTERVAL is suppressed whenever auto_hist && ALTER_RECREATE, on the assumption that prep_alter_part_table() will fix STARTS later. That fix only happens when the table has history and use_default_partitions is set. With no history, or with an explicit partition list, a STARTS later than query time is now accepted with no warning at all. The check also runs at parse time, so it depends on token order. In the recorded results, alter table t1 force partition by ... starts '2003-...' auto emits only 4265, but alter table t1 partition by ... starts '2004-...' auto force emits both 4164 ("STARTS ... is later than query timestamp") and 4265 ("... was set to 2000-01-01"). Those two warnings contradict each other, and that order dependence contradicts the commit claim that the clauses "can appear in any order". Drop the parse-time suppression and decide in prep_alter_part_table(), where the outcome is known. (S3 / logging-and-errors.md: "Tell the truth") — by silent-failure-hunter
9. • Important [sql/partition_info.cc:2860-2862] The !(auto_hist && (alter_info.flags & ALTER_RECREATE)) suppression of WARN_VERS_STARTS_BEYOND_INTERVAL runs in the parser and depends on token order. With the new grammar, trailing ... AUTO FORCE sets ALTER_RECREATE only after vers_set_interval() has run. The result file shows this: force partition by ... starts '2003-..' auto emits only 4265 (partition2.result line 241), while partition by ... starts '2004-..' auto force emits both 4164 and 4265 (line 260). The suppression also fires when FORCE is given and the table has no history, so the warning is lost even though STARTS stays in the future. Fix: decide in prep_alter_part_table(), where it is known whether STARTS was actually rewritten, rather than in the parser. (bug) — by code-reviewer
10. • Important [sql/partition_info.cc:2861] The FORCE-syntax commit (08acd39) says alter_list and partitioning "can appear in any order". But vers_set_interval() runs as a grammar action, so ALTER_RECREATE is only set when FORCE comes before PARTITION BY. The result file shows the effect. Prefix force partition by ... starts '2003-...' gets only WARN_VERS_WRONG_STARTS. Postfix ... starts '2004-...' auto force gets both 4164 ("STARTS ... later than query timestamp") and 4265 ("... was set to 2000-01-01"), which contradict each other. The suppression condition also has no comment explaining why it exists. Make the check independent of order (move it after parsing), or document that it depends on order. (S3; coding-style.md:Comments) — by comment-analyzer
11. • Important [sql/partition_info.cc:2891] The vers_set_starts() Doxygen says it clears components "until the value aligns with the coarsest unit present in interval.step". The code stops at the finest non-zero unit. For example, INTERVAL '1:30' HOUR_MINUTE keeps minutes and clears only seconds. Replace "coarsest" with "finest (smallest) non-zero unit", and note that day, month and year are never cleared. (S3) — by comment-analyzer
12. • Important [sql/partition_info.cc:2900] All three callers of the new vers_set_starts() (partition_info.cc:2874 via interval_starts_error, sql_partition.cc:6192 and 6207) handle failure with the same my_error(ER_PART_WRONG_VALUE, MYF(0), <table>, "STARTS"). The error belongs inside vers_set_starts(thd, ts, table_name). The two sql_partition.cc branches would then collapse into one if (!starts_clause || interval.start > min_ts.sec) block that warns only when starts_clause is set (S5) — by type-design-analyzer
13. • Important [sql/partition_info.cc:520-521] The ER_OUT_OF_RESOURCES fix in set_up_default_partitions() (line 453) leaves the identical silent else goto end; in the sibling set_up_default_subpartitions() for new partition_element(part_elem). Fix both, or neither. (S2) — by code-reviewer
14. • Important [sql/sql_class.h:8729] The new type TimestampString is CamelCase. The project convention for new types is Capital_snake_case, so it should be Timestamp_string. (cited from coding-style.md:Critical baseline, snake_case / Capital_snake_case row) — by code-reviewer (also flagged by type-design-analyzer)
15. • Important [sql/sql_class.h:8759] cstr(uint dec= 0) drops the microsecond part by default. That defeats the my_timespec_t constructor, which exists to carry it: at sql/sql_partition.cc:6198 str_min_ts.cstr() prints the earliest row_end 2000-01-01 00:10:00.220000 as 2000-01-01 00:10:00 (see the WARN_VERS_WRONG_STARTS lines in suite/versioning/r/partition2.result). Precision should be a required argument, or the my_timespec_t path should default to 6 (S3; cited from api-and-architecture.md:API design discipline, "Don't add default values for behaviour-changing ... parameters") — by type-design-analyzer
16. • Important [sql/sql_class.h:8762] On failure cstr() silently returns the literal "ERROR" and pastes it into a user-visible warning. Failure means String::alloc OOM, and real_alloc uses MY_WME, so ER_OUTOFMEMORY is already in the DA. Callers such as vers_set_interval() keep going as if nothing happened, and the statement continues with an error set (set_ok_status asserts !is_set() in debug). Let the caller see the failure (return NULL / check thd->is_error() after formatting), or at minimum document the OOM consequence. (logging-and-errors.md: "The message tells the user what happened") — by silent-failure-hunter
17. • Important [sql/sql_partition.cc:6164] The new history-range block runs for TRX_ID-versioned tables. The commit message says so: "It works in vers_get_history_range() but ... create temporary table fails". So ALTER does a TRT lookup per row on the scan path, and each TR_table::query is itself a full TRT read. That makes the work O(N*M), and it can raise and swallow errors (previous finding), all before an ALTER that is guaranteed to be rejected later with ER_VERS_FIELD_WRONG_TYPE at frm open. Add table->versioned(VERS_TIMESTAMP) to the block condition, or reject TRX_ID first, so the user gets the real error up front and no wasted scan. (S4: check at the wrong layer) — by silent-failure-hunter
18. • Important [sql/sql_partition.cc:6171-6172] if (*fast_alter_table) *fast_alter_table= false; is a check-then-assign with no effect beyond *fast_alter_table= false;. Also, the condition on line 6169 quietly overrides an explicit PARTITIONS N: use_default_partitions is still true when only a count is given, and the user's N is replaced without a note. The commit message only mentions an explicit partition list being honoured. Either honour N (for example max(N, computed)) or document and test the override. (bug/clarity; confidence ~80) — by code-reviewer
19. • Important [sql/sql_partition.cc:6177,6196,6211,6221] Every new failure exit uses DBUG_RETURN(true). All the other failures inside the if (thd->work_part_info) block use goto err, which resets *fast_alter_table and restores table->part_info= saved_part_info. The new block also changes part_info (partitions.empty(), default_partitions_setup= false, interval.start) before vers_calc_hist_parts() can fail. In the fast-ALTER branch part_info is the live tab_part_info, not a clone. Use goto err, and do the partitions.empty()/flag changes only after vers_calc_hist_parts() succeeds. (S6) — by silent-failure-hunter
20. • Important [sql/sql_partition.cc:6182-6186, 6284-6290, 6302] There is an off-by-one when the latest history row_end falls exactly on a partition boundary. vers_get_partition_id() and the vers_set_hist_part() sibling treat the left boundary as closed and the right as open, so ts == start + k*interval belongs to history partition k and needs k+1 partitions. The new code takes the ceiling of (end-start)/interval in the seconds path and loops while (start < end) in the month/year path, so it returns k. The sibling uses while (h <= q). The end == start → 1 special case shows the inconsistency: end == start + interval also returns 1. Fix: compute floor((max_ts.sec - start)/i_sec) + 1 on the unrounded seconds value, use <= in the loop, and drop the usec → sec++ rounding, since partitioning compares whole seconds only. (bug; S6) — by code-reviewer
21. • Important [sql/sql_partition.cc:6187-6211] The vers_set_starts(thd, min_ts.sec) call followed by my_error(ER_PART_WRONG_VALUE, ..., "STARTS"); DBUG_RETURN(true); is copy-pasted in both branches. Restructure as if (!starts_clause || interval.start > min_ts.sec) { save old start; if (vers_set_starts()) error; if (starts_clause) warn; }. (S5) — by code-reviewer
22. • Important [sql/sql_partition.cc:6192-6212] The vers_set_starts() + my_error(ER_PART_WRONG_VALUE, MYF(0), table->s->table_name.str, "STARTS"); DBUG_RETURN(true); block is copied twice, differing only in the condition that leads to it. Restructure so vers_set_starts() is called and checked once, with the warning emitted only in the starts_clause case. (S5 / S1) — by silent-failure-hunter
23. • Important [sql/sql_partition.cc:6293,6306,6313,6320] ER_TOO_MANY_PARTITIONS_ERROR is raised bare. The user is not told that the history range divided by INTERVAL is the cause, nor which table. The sibling vers_set_hist_part() (partition_info.cc:877-881) raises it as a warning followed by ER_VERS_HIST_PART_FAILED with db and table names. ER_DATA_OUT_OF_RANGE, "TIMESTAMP", "INTERVAL" likewise names no table. Follow the sibling pattern and name the table. (S6 / logging-and-errors.md: "Include the table/constraint/object name") — by silent-failure-hunter
24. • Important [sql/sql_partition.cc:6300-6310] vers_calc_hist_parts() steps month/year boundaries in thd->variables.time_zone. The code that actually builds the boundaries (range_int_array, around sql_partition.cc:1581) and the auto-create sibling vers_set_hist_part() (partition_info.cc:866) both use my_tz_OFFSET0. Under a session time zone with DST, the computed count can differ from the boundaries that get built. Fix: use my_tz_OFFSET0 like the siblings, and consider the sibling's MAX_PARTITIONS - 2 cap. (S6) — by code-reviewer
25. • Important [sql/sql_partition.cc:6300-6315] vers_calc_hist_parts() steps month/year boundaries in the session time zone (thd->variables.time_zone). The partitions are actually built by check_vers_constants() (sql_partition.cc:1581) and vers_set_hist_part() (partition_info.cc:866), and both step in my_tz_OFFSET0. In DST zones the computed count can differ from the real boundaries by one. Also, TIME_to_gmt_sec() returns ER_WARN_INVALID_TIMESTAMP (tztime.cc:968) for a time that falls in a DST gap. The new code treats any non-zero error as fatal, so a valid ALTER fails with the misleading "TIMESTAMP value is out of range in 'INTERVAL'". The sibling only fails on ER_WARN_DATA_OUT_OF_RANGE, and even then it clamps the last partition. Use my_tz_OFFSET0 as the siblings do. The tests only run with time_zone='+00:00', so this is untested. (S2 / S6) — by silent-failure-hunter
26. • Important [sql/sql_table.cc:13947] The vers_end_best_idx() Doxygen says the index's "first and last entries yield the range", and vers_get_history_range() at :13998 repeats "reads its first and last entries". The code actually reads ha_index_first() and then ha_index_read_map(..., HA_READ_BEFORE_KEY) from a max-valued key, i.e. the last historical entry before the current rows. The real last entry is a current row with the MY_TIMESPEC_MAX row_end. Say "the first entry and the last entry before the current-row (max) key". (S3) — by comment-analyzer
27. • Important [sql/sql_table.cc:13971-13987] vers_end_best_idx() iterates over all s->keys and ignores s->keys_in_use. After ALTER TABLE ... DISABLE KEYS on MyISAM/Aria, it can pick a disabled non-unique (row_end, ...) index, and ha_index_first() then fails (HA_ERR_WRONG_INDEX) or returns no rows, so the ALTER either errors out or treats the history as empty. Fix: also require s->keys_in_use.is_set(idx). (bug) — by code-reviewer
28. • Important [sql/sql_table.cc:13994] The vers_get_history_range() Doxygen does not document the no-history result (min_ts == MY_TIMESPEC_MAX, max_ts == MY_TIMESPEC_MIN). The caller in sql_partition.cc:6178 and its DBUG_ASSERT(max_ts == MY_TIMESPEC_MIN) depend on that contract. Add it to the @param text for min_ts/max_ts. (coding-style.md:Comments, "add a comment when surprising-but-correct code can mislead") — by comment-analyzer
29. • Important [sql/sql_table.cc:14011] @retval true Handler or conversion error is wrong. Only handler errors set error. A get_timestamp() failure (the TRX_ID path returns -1 with a warning) is not detected and flows silently into min_ts/max_ts. Drop "or conversion", or check the -1 result. — by comment-analyzer
30. • Important [sql/sql_table.cc:14038, 14125] vers_get_history_range() calls file->ha_external_lock(thd, F_RDLCK) and later ha_external_unlock() without checking thd->locked_tables_mode. Under LOCK TABLES t1 WRITE; ALTER TABLE t1 PARTITION BY SYSTEM_TIME ... AUTO, the TABLE is already externally locked. That trips the "consecutive calls for lock without unlocking" assertion in handler::ha_external_lock() (handler.cc:7996), and in release builds the final unlock drops a lock that LOCK TABLES still believes it holds. Fix: skip the lock/unlock when file->get_lock_type() != F_UNLCK (or under LTM), and add a LOCK TABLES case to partition2.test. (bug) — by code-reviewer
31. • Important [sql/sql_table.cc:14056,14061,14097 / sql/field.cc:2453] The -1 that get_timestamp() returns on a TRT miss is never checked. TR_table::query() has already raised my_error(ER_VERS_NO_TRX_ID) (table.cc:10772), or the TRT open error, as an ERROR. get_timestamp() then pushes the same code again as a warning, and vers_get_history_range() returns success (error == 0) while thd->is_error() is set. The caller then goes on with min_ts.sec/max_ts.sec == -1. On 64-bit, MY_TIME_T_MIN == 0, so max_ts.sec == -1 takes the "no history" branch, and DBUG_ASSERT(max_ts == MY_TIMESPEC_MIN) fails in debug builds. A bad min_ts gives a misleading ER_PART_WRONG_VALUE 'STARTS'. The doc comment ("reported as a warning ... caller can still continue") is false. Fix: check thd->is_error() (or return a bool status) after each get_timestamp() call in vers_get_history_range() and fail. (logging-and-errors.md: "Don't lose context") — by silent-failure-hunter
32. • Important [sql/sql_table.cc:14071-14104] The debug cross-check (test_mdev-25529) does not reset min_ts/max_ts before the goto jump_scan scan. The scan therefore starts from the index results and can only lower min or raise max. An index path that reports a min that is too small or a max that is too large is never caught, which defeats the "compare the results" purpose stated in the commit and the docstring. Fix: reset both to MY_TIMESPEC_MAX/MIN after saving min_ts2/max_ts2. (bug in test harness) — by code-reviewer
33. • Important [sql/sql_table.cc:14073] The comment "Test both index and scan, compare the results between them" (and the matching sentence in the vers_get_history_range() Doxygen at :14001-14003) claims an independent cross-check. But min_ts/max_ts are not reset to MY_TIMESPEC_MAX/MIN before goto jump_scan, so the scan at :14097-14104 can only widen the index result. If the index path returns a range that is too wide (for example a min below the true min), DBUG_ASSERT(min_ts == min_ts2) still passes. Either reset the two values before the scan, or reword the comment to say the check is one-sided. (S3) — by comment-analyzer
34. • Important [sql/sql_type.cc:408] Temporal_hybrid(THD*, my_timespec_t) has the same body as the existing Datetime::Datetime(THD*, const my_timeval&) (sql/sql_type.cc:1116), but leaves out thd->used|= THD::TIME_ZONE_USED. That flag is set one layer up in THD::timestamp_to_string, so any future direct user of the constructor will silently skip it. Datetime(thd, Timeval(sec, usec)).to_string(str, dec) already does the whole job (S2) — by type-design-analyzer
35. • Important [sql/sql_type.h:1424] Temporal_hybrid::to_string now returns NULL when alloc() fails, but its siblings Interval_DDhhmmssff::to_string (sql/sql_type.h:1626), Time::to_string (2064) and Datetime::to_string (2752) keep the old "return str with stale length" shape. Either fix all four together (preferably as a separate commit) or keep this PR's change local to the new caller (S6) — by type-design-analyzer

Nit

1. • Nit [git 3615202] The commit "set_up_default_partitions() ER_OUT_OF_RESOURCES error" also adds the unrelated TODO (newbie) comment. (commit-and-process.md:Commit structure, "split unrelated changes") — by comment-analyzer
2. • Nit [git 8d25e36] The subject "cleanup for vers_set_starts() and starts_clause" is not accurate. Besides the vers_set_starts() extraction, the commit adds a new Vers_part_info::starts_clause member that is unused until 2fee395. Either describe it as preparation, or squash it. (commit-and-process.md:Commit structure) — by comment-analyzer
3. • Nit [include/my_time.h:313] ulong usec is used in a new struct. It is defensible only because it has to match Field::get_timestamp(ulong*); a short comment saying so would preempt the comment from reviewers. (cited from coding-style.md:Critical baseline, no long/ulong) — by code-reviewer (also flagged by type-design-analyzer)
4. • Nit [mysql-test/main/partition.test:3151] The test header --echo # MDEV-25529 ALTER TABLE FORCE syntax improved uses the commit subject, not the MDEV title. (testing.md:37) — by comment-analyzer
5. • Nit [mysql-test/main/partition.test:3162] "Strange syntax, but ..." is an opinion. State the fact, e.g. "alter_list and partitioning may be combined in either order (supported since cd483c5, 2005)". The commit hash checks out. (coding-style.md:Comments, "state facts, no opinions") — by comment-analyzer
6. • Nit [sql/event_data_objects.cc:692] The "MDEV-25529 Comments" commit documents get_next_time() in the event scheduler, which has nothing to do with this MDEV. Split it out or drop it. (cited from anti-patterns.md:Commit / process, "Don't bundle unrelated changes") — by code-reviewer (also flagged by comment-analyzer)
7. • Nit [sql/field.cc:2420] The @note rationale for the TRT lookup ("registry row may have been purged ...; the caller can still continue") is not supported by the code. No caller checks the -1 sentinel. vers_get_history_range() stores it as a real timestamp, and the sibling Field_vers_trx_id::get_date() treats the same case as an error (return true). Remove the speculative rationale, or document that callers must check for -1. (S3) — by comment-analyzer
8. • Nit [sql/field.cc:2424] @param[in] and @param[out] (also at sql/sql_table.cc:13961 @param[out] best_idx) go against the project Doxygen sub-style "no [in]/[out] markers". That rulebook entry is listed under InnoDB, so apply it only if the server-wide style is meant to match. (checklist.md:InnoDB-specific, Doxygen) — by comment-analyzer
9. • Nit [sql/field.cc:2458] Field_vers_trx_id::get_timestamp reports "TRX not found" by returning -1 as a my_time_t, and that value can flow into a my_timespec_t with no flag. The author disputes reachability (TRX_ID versioning is rejected later), but the sentinel is part of a value type and should at least be noted in the vers_get_history_range contract (cited from anti-patterns.md:Specific subtle bugs, "-1 smuggled into ... slot") — by type-design-analyzer
10. • Nit [sql/partition_info.cc:432] The // TODO (newbie): how is it freed? ... comment is an open question, not a statement of fact. partition_element is Sql_alloc'ed on the THD mem_root, so either state that or drop the comment. (cited from coding-style.md:Comments, "State facts") — by code-reviewer (also flagged by comment-analyzer, silent-failure-hunter)
11. • Nit [sql/partition_info.h:93] starts_clause has no comment and it doesn't strictly mean "the user wrote STARTS in this statement". vers_set_interval also sets it on the mysql_unpack_partition() (frm) path, and the copy-ctor and operator= propagate it. A one-line comment on what the flag means and when it is valid would help (own lens: invariant expression) — by type-design-analyzer
12. • Nit [sql/share/errmsg-utf8.txt:12147] ER_PART_STARTS_BEYOND_INTERVAL is renamed to WARN_VERS_STARTS_BEYOND_INTERVAL and its argument count goes from 1 to 3. libmariadb/include/mysqld_error.h still exports the old name for 4164, so the server and Connector/C now disagree on the symbol. Not ABI-breaking (same number), but worth a note in the commit. (cited from api-and-architecture.md:Connector / ABI) — by code-reviewer
13. • Nit [sql/sql_class.h:4783] THD::timestamp_to_string(String*, uint dec, ts) orders its parameters differently from its neighbour THD::timestamp_to_TIME(MYSQL_TIME*, my_time_t ts, ulong sec_part, ...). It also adds another formatting entry point to THD when Datetime(thd, Timeval) already provides this (cited from api-and-architecture.md:API design discipline, "De-THD direction") — by type-design-analyzer
14. • Nit [sql/sql_class.h:8729] The new class TimestampString has no class-level comment and uses CamelCase. Project style is Capital_snake_case for new types (e.g. Timestamp_string). (coding-style.md:18) — by comment-analyzer
15. • Nit [sql/sql_class.h:8741] TimestampString is copyable through the defaulted String copy-ctor, which shares Ptr without owning it, and it keeps a THD* only to read the time zone later. If the class stays, delete the copy operations and format in the constructor (own lens: ownership/lifetime) — by type-design-analyzer
16. • Nit [sql/sql_partition.cc:103] The vers_calc_hist_parts() Doxygen sits on the static forward declaration among undocumented prototypes. The definition at :6264 has none. Put the doc block on the definition, as the surrounding code does. Also, "number of partitions would exceed MAX_PARTITIONS" is ambiguous: the check is hist_parts >= MAX_PARTITIONS, so the limit applies to history + current. (S6) — by comment-analyzer
17. • Nit [sql/sql_partition.cc:123] vers_calc_hist_parts has a forward declaration whose Doxygen block sits about 6000 lines from the definition at sql_partition.cc:6264. Define it before prep_alter_part_table and keep the doc next to the body (own lens) — by type-design-analyzer
18. • Nit [sql/sql_partition.cc:6164-6170] When an explicit partition list is given with AUTO FORCE, or AUTO is used on an already-partitioned table without FORCE ("STARTS doesn't change without FORCE"), the history re-layout is silently skipped. Existing history then stays crammed into p0/the last partition, with no note telling the user to add FORCE or drop the explicit list. A NOTE would make this fallback visible. — by silent-failure-hunter
19. • Nit [sql/sql_partition.cc:6169] The (!*fast_alter_table || (alter_info->flags & ALTER_RECREATE)) gate, together with alt_part_info->use_default_partitions, encodes the rules "FORCE is required on fast ALTER" and "an explicit partition list disables this". Those rules are only in the commit message. A short comment above the if would keep them next to the code. (coding-style.md:Comments) — by comment-analyzer
20. • Nit [sql/sql_partition.cc:6177] The new error exits use DBUG_RETURN(true) (:6177, :6196, :6211, :6221). The sibling error paths in the same if (thd->work_part_info) block (:6145, :6246) use goto err, which resets *fast_alter_table and restores saved_part_info. Match the sibling factoring, or comment on why the restore is not needed here. (S6) — by comment-analyzer
21. • Nit [sql/sql_partition.cc:6178] "No history" is encoded in my_timespec_t itself: the caller tests max_ts.sec > MY_TIME_T_MIN but the else-branch asserts max_ts == MY_TIMESPEC_MIN (sec and usec both). On 64-bit, MY_TIME_T_MIN is 0, so the test and the assert disagree for {0, usec>0}. Returning an explicit emptiness flag, or a small {min, max} struct with an is_empty() method, from vers_get_history_range would make the sentinel unnecessary (own lens: invariant expression) — by type-design-analyzer
22. • Nit [sql/sql_partition.cc:6182] Rounding max_ts up to the next whole second when usec != 0 is a deliberate ceiling, but it has no comment. Add one saying why (vers_calc_hist_parts() works in whole seconds, and the history end must be covered). (coding-style.md:Comments, "add a comment when surprising-but-correct code can mislead") — by comment-analyzer
23. • Nit [sql/sql_partition.cc:6195] The my_error(ER_PART_WRONG_VALUE, MYF(0), table->s->table_name.str, "STARTS"); DBUG_RETURN(true); block is duplicated at :6210. Compute "need to reset STARTS" once (!starts_clause || interval.start > min_ts.sec) and call vers_set_starts() in one place. (S5) — by comment-analyzer
24. • Nit [sql/sql_partition.cc:6199-6202] Lines exceed 80 columns (push_warning_printf(... WARN_VERS_WRONG_STARTS, ER_THD(...) and the cstr() argument line), and there is a stray blank line before the closing brace. (cited from coding-style.md:Critical baseline, 80-col) — by code-reviewer
25. • Nit [sql/sql_table.cc:13958] The @note keys_in_use_for_query is intentionally not consulted... repeats the inline comment inside the loop at :13974 word for word. Keep only one. (code-comments-trim-obvious) — by comment-analyzer
26. • Nit [sql/sql_table.cc:13966] vers_end_best_idx(uint &best_idx) returns both the KEY* and its index through an out-param that stays uninitialized when nothing is found. The caller then rebuilds the same pointer (key_info + best_idx, sql_table.cc:14044). One return value is enough: the index with MAX_KEY as "none", or the KEY* with best_key - key_info (cited from api-and-architecture.md:Errors and return values; checklist.md:Code, "Initialise output variables on all paths") — by type-design-analyzer
27. • Nit [sql/sql_table.cc:14018] // Find best key above vers_end_best_idx(best_idx) restates the call. Remove it. (code-comments-trim-obvious) — by comment-analyzer
28. • Nit [sql/sql_table.cc:14044] KEY *best_key_info= key_info + best_idx; duplicates best_key, which vers_end_best_idx() already returns. Use best_key, or have the helper return only the index (MAX_KEY when none fits). — by code-reviewer
29. • Nit [sql/sql_table.cc:14083] WARN_VERS_SLOW_ROW_END says what happened but not what to do about it. Suggest wording that tells the user to add an index whose first column is ROW END (and not DESC). (logging-and-errors.md: "The message tells the user what happened and what they can do about it") — by silent-failure-hunter
30. • Nit [sql/sql_table.cc:14129] /* Other handler errors are fatal */ was copied from sql_update.cc, where it follows an "If (ignore && error is ignorable)..." comment. Here there is no earlier case, so "Other" refers to nothing. Drop the comment or say "fatal handler errors get ME_FATAL". (S3) — by comment-analyzer
31. • Nit [sql/sql_time.h:169] interval2sec() and interval2usec() are documented as converting "an interval value". The only interval2usec() caller (sql/sql_time.cc:647) also applies it to ltime, a MYSQL_TIME point in time. The docs also do not say that year, month and neg are ignored, a precondition vers_calc_hist_parts() relies on. Mention both. — by comment-analyzer
32. • Nit [sql/sql_time.h:179] The interval2sec doc says it converts "an interval value to a total number of seconds", but it ignores year, month and neg, and interval2usec is also applied to a plain MYSQL_TIME (sql_time.cc:647). The doc should state those limits. Only vers_calc_hist_parts guards the year/month case (S3) — by type-design-analyzer
33. • Nit [sql/sql_yacc.yy / commit 08acd39] The commit body claims improved "handling of AUTO with FORCE", but the change only lets alter_list trail the partitioning clause. Reword it. (S3) — by code-reviewer

Praise

1. • Praise [sql/field.cc:2453] Field_vers_trx_id::get_timestamp now sets *sec_part on every path, including the TRT-miss path (cited from checklist.md:Code, "Initialise output variables on all paths") — by type-design-analyzer
2. • Praise [sql/partition_info.cc:452] set_up_default_partitions() no longer fails silently on a new partition_element()/push_back failure. It now sets an error before goto end, which matches the other exits in the function. — by silent-failure-hunter
3. • Praise [sql/sql_class.cc:9129] The THD::timestamp_to_string() Doxygen is accurate, including the TIME_ZONE_USED side effect, which is easy to overlook. — by comment-analyzer
4. • Praise [sql/sql_class.h:8736] The TimestampString constructor and cstr() docs are accurate. The value is copied at construction and "ERROR" is returned on failure, which is what makes the str_interval / str_interval2 before/after use in sql_partition.cc:6191-6200 correct. — by comment-analyzer
5. • Praise [sql/sql_table.cc:14120-14136] vers_get_history_range() sends every handler error (external_lock, index_init, rnd_init, read) through print_error with ME_FATAL classification, and restores the column bitmaps on all paths. The only gap is the TRT-miss case above. — by silent-failure-hunter
6. • Praise [sql/sql_time.cc:27] The /* Daynumber from year 0 to 9999-12-31 */ comment was misplaced above the old COMBINE macro. It now sits on MAX_DAY_NUMBER (3652424), which it actually describes. Good comment-rot fix. — by comment-analyzer
7. • Praise [sql/sql_time.cc:647] Replacing the untyped COMBINE macro with inline helpers, and moving the misplaced "Daynumber" comment onto MAX_DAY_NUMBER, are good cleanups — by type-design-analyzer
8. • Praise [sql/sql_time.h:178-199] Replacing the COMBINE macro with typed interval2sec()/interval2usec() helpers, and reusing them in vers_calc_hist_parts(), is a good cleanup-before-fix commit. — by code-reviewer
9. • Praise [sql/sql_type.h:1424] The fix to Temporal_hybrid::to_string(), so it returns NULL on alloc failure instead of a string object with garbage length, is a real latent-bug fix and is correctly split into its own commit. — by code-reviewer (also flagged by silent-failure-hunter)

[sanja-byelkin]

Is embedded crash related ?

[midenok]

Triage of mreview report

Rejected findings, grouped by the original importance tier, most important first,
sorted by id within each tier. IDs reference the numbered index of the original
mreview report (i = important, n = nit). Each entry gives the reason and names
the surviving finding it should defer to, where applicable.

Total rejected: 16 (5 important, 11 nits).

Important rejected (5)

• i9 [sql/partition_info.cc:2860-2862] — Duplicate.
  Token-order-dependent suppression of WARN_VERS_STARTS_BEYOND_INTERVAL.
  Restates i8 [partition_info.cc:2860-2861] almost verbatim (same lines,
  same result-file evidence, same fix: decide in prep_alter_part_table()).
  Keep i8.
• i20 [sql/sql_partition.cc:6182-6186,6284-6290,6302] — Duplicate.
  Off-by-one on a partition boundary. Same defect as blocker b2
  [sql_partition.cc:6288], which states the fix (range / i_sec + 1,
  while (start <= end)). Fold into b2.
• i22 [sql/sql_partition.cc:6192-6212] — Duplicate.
  Copy-pasted vers_set_starts() + my_error(ER_PART_WRONG_VALUE,"STARTS")
  block. Same remedy as i21 [sql_partition.cc:6187-6211]. Keep i21.
• i24 [sql/sql_partition.cc:6300-6310] — Duplicate.
  Month/year boundaries stepped in the session time zone instead of
  my_tz_OFFSET0. Strict subset of i25 [sql_partition.cc:6300-6315], which
  adds the DST-gap ER_WARN_INVALID_TIMESTAMP fatal-error case. Keep i25.
• i33 [sql/sql_table.cc:14073] — Duplicate.
  Debug cross-check does not reset min_ts/max_ts before the scan, so the check
  is one-sided. Same defect as i32 [sql_table.cc:14071-14104], which also
  states the fix. Keep i32.

Nits rejected (11)

• n1 [git 3615202] — Non-legit.
  Claims the set_up_default_partitions() ER_OUT_OF_RESOURCES error commit
  bundles an unrelated comment. The flagged comment is the same hunk as n10
  [partition_info.cc:432], which asked to turn the TODO (newbie) question into
  a statement of fact — already done. The replacement note documents how
  partition_element is allocated in set_up_default_partitions(), the exact
  function the commit repairs (it adds the ER_OUT_OF_RESOURCES on allocation
  failure). The doc and the error handling are the same concern in the same
  function; splitting a six-line note into its own commit is churn. Reject.
• n4 [mysql-test/main/partition.test:3151] — Non-legit.
  Claims the --echo # MDEV-25529 ALTER TABLE FORCE syntax improved header
  should use the MDEV title instead of the commit subject. But that block was
  added by the FORCE-syntax commit and tests FORCE syntax; the descriptive
  header names the sub-feature correctly, and the Auto-create MDEV title would
  mislabel a FORCE-syntax test. Reject.
• n6 [sql/event_data_objects.cc:692] — Author's call.
  Asks to split out or drop the get_next_time() comment as unrelated to the
  MDEV. The author keeps it: the note documents the interval-stepping semantics
  (multiples of the interval, time-zone and DST handling) that the versioning
  interval math mirrors, so it is relevant background and worth retaining in the
  branch's "MDEV-25529 Comments" commit. Reject.
• n7 [sql/field.cc:2420] — Moot (outdated).
  The @note on Field_vers_trx_id::get_timestamp() no longer exists: the
  method was dropped from the branch during rebase. Nothing to fix.
• n8 [sql/field.cc:2424, sql/sql_table.cc:13961] — Non-legit.
  Claims @param[in] / @param[out] markers violate project Doxygen style.
  The cited rule is listed under InnoDB. Server code uses these markers
  pervasively: @param[in] appears 455 times and @param[in,out] 60 times
  across sql/*.cc. The marker is the server-wide norm, not a violation. The
  finding even hedges this itself ("apply only if the server-wide style is
  meant to match"). Reject.
• n14 [sql/sql_class.h:8729] — Duplicate.
  TimestampString is CamelCase. Naming point already made as important finding
  i14 [sql_class.h:8729]; the only extra bit ("no class-level comment") is
  marginal. Fold into i14.
• n17 [sql/sql_partition.cc:123] — Duplicate.
  vers_calc_hist_parts() forward-decl Doxygen sits far from the definition.
  Same point as n16 [sql_partition.cc:103], which also flags the
  >= MAX_PARTITIONS wording. Keep n16.
• n18 [sql/sql_partition.cc:6164-6170] — Not worth a new error code.
  Asks for a NOTE when the AUTO re-layout is skipped (explicit partition list,
  or fast ALTER without FORCE). The table is still repartitioned (AUTO is
  honored); only the STARTS alignment to the existing history is skipped. No
  existing message describes this accurately (WARN_OPTION_IGNORED/"AUTO" is
  wrong), and it is too specific to justify a new error number. Already
  documented by the test comment "STARTS is not adjusted to the history range
  without FORCE". Reject.
• n23 [sql/sql_partition.cc:6195] — Duplicate.
  The duplicated STARTS error block again. Covered by i21/i22 and the "move
  error into vers_set_starts()" finding i12.
• n26 [sql/sql_table.cc:13966] — Non-legit.
  Asks to collapse vers_end_best_idx() to a single return value (the index,
  with MAX_KEY as "none") and drop the best_idx out-param. But both outputs
  are consumed downstream: best_idx feeds ha_index_init(best_idx) and the
  KEY *best_key feeds the branch guard, key_part, and key_copy(). Returning
  only the index forces the caller to rebuild best_key= key_info + best_idx
  behind a second == MAX_KEY check — an extra line and condition for no gain.
  The out-param is not gratuitous. Reject;
• n32 [sql/sql_time.h:179] — Duplicate.
  interval2sec() doc omits that year/month/neg are ignored. Same as n31
  [sql_time.h:169]. Keep n31.

Notes on findings kept but overlapping

• i8 and i10 [partition_info.cc:2860-2861] — i10 adds a distinct angle
  (the commit's "any order" claim is false because vers_set_interval() is a
  grammar action). Kept separate from i8.
• i12 [partition_info.cc:2900], i21 [sql_partition.cc:6187-6211] — Both
  target the duplicated STARTS-error handling, describing the same fix from two
  angles. Keep as a single action item; not double-counted.
• n9 [field.cc:2458] — concerns the -1 TRT-miss sentinel entering a value
  type. (n7, the related @note finding, is now rejected as moot above — the
  method was dropped.)

[sanja-byelkin]

Regression: WARN_VERS_STARTS_BEYOND_INTERVAL (4164) is lost with AUTO

The i8 fix changed the condition in vers_set_interval() (sql/partition_info.cc) to

if (thd->query_start() < vers_info->interval.start && !auto_hist)

The base code raised the warning unconditionally. Now any STARTS later than the query time combined with AUTO gives no warning. That includes:

• CREATE TABLE, which never reaches the STARTS re-alignment in prep_alter_part_table().
• ALTER TABLE (with or without FORCE) on a table that has no history, where STARTS is not re-aligned either.

The warning should only be dropped when prep_alter_part_table() actually re-aligns STARTS (in that case WARN_VERS_WRONG_STARTS is raised instead).

The test below fails on 58e20ec: all three Warning 4164 lines expected after the AUTO statements are missing. Please add it, or an equivalent, and fix the condition.

mysql-test/suite/versioning/t/partition_auto_starts.test:

--source include/have_partition.inc

set @@session.time_zone='+00:00';

--echo #
--echo # MDEV-25529 STARTS later than query time must warn with AUTO too
--echo #
set timestamp= unix_timestamp('2000-01-01 00:00:00');

# Without AUTO: warns
create or replace table t1 (x int) with system versioning
partition by system_time interval 1 hour starts '2000-01-01 01:00:00';

# With AUTO: must warn the same way (CREATE never re-aligns STARTS)
create or replace table t1 (x int) with system versioning
partition by system_time interval 1 hour starts '2000-01-01 01:00:00' auto;

# ALTER adding AUTO partitioning to a table without history: STARTS is
# not re-aligned, so it must warn as well
create or replace table t1 (x int) with system versioning;
alter table t1 partition by system_time interval 1 hour
  starts '2000-01-01 01:00:00' auto;
alter table t1 force partition by system_time interval 1 hour
  starts '2000-01-01 01:00:00' auto;

drop table t1;
set timestamp= default;

--echo # End of 13.2 tests

mysql-test/suite/versioning/r/partition_auto_starts.result (expected):

set @@session.time_zone='+00:00';
#
# MDEV-25529 STARTS later than query time must warn with AUTO too
#
set timestamp= unix_timestamp('2000-01-01 00:00:00');
create or replace table t1 (x int) with system versioning
partition by system_time interval 1 hour starts '2000-01-01 01:00:00';
Warnings:
Warning	4164	`t1`: STARTS timestamp 2000-01-01 01:00:00 is later than query timestamp 2000-01-01 00:00:00, first history partition may exceed INTERVAL value
create or replace table t1 (x int) with system versioning
partition by system_time interval 1 hour starts '2000-01-01 01:00:00' auto;
Warnings:
Warning	4164	`t1`: STARTS timestamp 2000-01-01 01:00:00 is later than query timestamp 2000-01-01 00:00:00, first history partition may exceed INTERVAL value
create or replace table t1 (x int) with system versioning;
alter table t1 partition by system_time interval 1 hour
starts '2000-01-01 01:00:00' auto;
Warnings:
Warning	4164	`t1`: STARTS timestamp 2000-01-01 01:00:00 is later than query timestamp 2000-01-01 00:00:00, first history partition may exceed INTERVAL value
Warning	4275	`t1`: ROW END index not found, using slow scan; add an index starting with the ROW END column (ascending, not DESC) to avoid it
alter table t1 force partition by system_time interval 1 hour
starts '2000-01-01 01:00:00' auto;
Warnings:
Warning	4164	`t1`: STARTS timestamp 2000-01-01 01:00:00 is later than query timestamp 2000-01-01 00:00:00, first history partition may exceed INTERVAL value
Warning	4275	`t1`: ROW END index not found, using slow scan; add an index starting with the ROW END column (ascending, not DESC) to avoid it
drop table t1;
set timestamp= default;
# End of 13.2 tests

[sanja-byelkin]

Review round 2 (head 58e20ec)

This round covers only new issues. Everything from round 1, your triage reply, and the STARTS-warning regression posted earlier is left out. Line numbers refer to 58e20ec.

Blockers

1. Server crash on Aria: the history range is read before the table is locked. (sql/sql_table.cc vers_get_history_range(), called from prep_alter_part_table())

   • prep_alter_part_table() runs from mysql_alter_table() before lock_tables(). At that point only ha_external_lock(F_RDLCK) is taken, under MDL_SHARED_UPGRADABLE, with no THR_LOCK and no Aria transaction setup.
   • On a transactional Aria table with a (row_end, ...) index, ha_index_first() crashes with SIGSEGV:

     trnman_can_read_from <- maria_rfirst <- handler::ha_index_first
       <- vers_get_history_range <- prep_alter_part_table <- mysql_alter_table
     

   • The same statement under LOCK TABLES works.
   • Even without the crash, MDL_SHARED_UPGRADABLE lets concurrent DML in. History rows written after the range was sampled land in the last computed history partition, silently. That is the very problem this MDEV fixes.
   • Read the range after the MDL upgrade and lock_tables(), or take a proper read lock for it.
   • Test case below.

2. After ALTER ... AUTO without STARTS, every UPDATE/DELETE fails. (sql/sql_partition.cc ~6259, vers_calc_hist_parts())

   • STARTS is now aligned to the oldest history row, but the layout is sized only up to max_ts, never up to query_start().
   • With history from 2000, alter table t1 partition by system_time interval 1 hour auto succeeds with PARTITIONS 2. Then:

     update t1 set x= 2;
     Warning 1499 Too many partitions (including subpartitions) were defined
     Error   4189 Versioned table `test`.`t1`: adding HISTORY partition(s) failed
     

   • Before this PR, STARTS defaulted to query_start() and DML kept working. An explicit old STARTS with AUTO already failed like this; the PR makes the default ALTER hit it.
   • partition2.result:619-621 already records this 1499/4189 pair under LOCK TABLES with no explanation.
   • Size against max(max_ts, query_start()), and reject (or don't align) when that needs too many partitions. Add a test that runs DML after such an ALTER.

Important

3. ALTER fails in DST zones where midnight doesn't exist. (sql/partition_info.cc:2944, vers_set_starts())
   • It is now applied to the oldest history row_end. It rounds down in the session time zone and treats any TIME_to_timestamp() error as fatal.
   • In America/Santiago, Asuncion, Havana or Asia/Beirut, with INTERVAL n DAY and an oldest row_end on a DST-start day, local 00:00 doesn't exist. The ALTER then fails with ER_PART_WRONG_VALUE ... 'STARTS' for a STARTS the user never wrote, and whether it fails depends on the stored data.
   • Accept ER_WARN_INVALID_TIMESTAMP (TIME_to_gmt_sec() already adjusts out of the gap), or round in UTC as vers_calc_hist_parts() now does.
4. Timestamp_string duplicates the ErrConv/ErrBuff formatters. (sql/sql_class.h:8832)
   • ErrConvTime / ErrBuff::set_mysql_time() in sql/sql_error.h already do this.
   • Compared with them, it heap-allocates a String per instance (three per WARN_VERS_WRONG_STARTS), has an OOM path, needs a non-const cstr(), and picks the precision implicitly from the constructor overload.
   • An ErrConvTimestamp : public ErrConv with a fixed stack buffer can't fail.
5. A test comment is false. (mysql-test/main/partition.test:3162)
   • # alter_list and partitioning may be combined in either order (since cd483c55, 2005): only alter_list partitioning dates from 2005.
   • The reverse order (remove partitioning add column y, the next statement) is added by this PR's own sql_yacc.yy change in 05b36ad.

Nits

• Timestamp_string::cstr() (sql_class.h:8864-8872) is documented as "empty on failure", but on OOM to_string() leaves Ptr == NULL and c_ptr_safe() fails too, so it returns NULL and the warning prints (null). A !is_valid_temporal() failure also puts nothing in the diagnostics area, although the doc says the error was already reported. Return "" explicitly, or fix the doc.
• THD::timestamp_to_string(String*, my_time_t, uint) (sql_class.h:4866) has no callers. Drop it.
• WARN_VERS_SLOW_ROW_END (sql_table.cc:14438,14506-14510) is raised before any row is read: on nearly every AUTO ALTER (the default layout has no row_end-leading index) and on empty tables. When the index exists but indexes_are_disabled(), "ROW END index not found ... add an index" is wrong advice; the user needs ENABLE KEYS.
• vers_calc_hist_parts() (sql_partition.cc:6355-6394) pastes my_error(..., ME_WARNING); my_error(ER_VERS_HIST_PART_FAILED, ...); return true; four times, and two copies are identical. Use one fail: exit, as the sibling vers_set_hist_part() does.
• if (end < start) return false; (sql_partition.cc:6335) reports success with *hist_parts == 0. The caller then builds num_parts= 1 and fails later with a confusing ER_VERS_WRONG_PARTS. Make it a DBUG_ASSERT, or a real error.
• vers_set_starts() summary (partition_info.cc:2912): "Round down ... to the partition interval boundary" overstates it. It truncates to the finest non-zero unit (INTERVAL 2 HOUR keeps the odd hour, a month interval keeps the day).
• The block comment "Align STARTS to the oldest historical row_end" (sql_partition.cc:6205-6208) omits that a user STARTS <= min_ts is kept unchanged.
• WARN_VERS_WRONG_STARTS (errmsg-utf8.txt:12428) says "is above earliest history date". Its sibling 4164 says "is later than", and the value is a microsecond timestamp, not a date.
• interval2sec() / interval2usec() (sql_time.h:183,200) are unconstrained template <typename T>. Two overloads, (const INTERVAL*) and (const MYSQL_TIME*), would state the accepted types.
• event_data_objects.cc:690-694: the old first line "Computes the sum of a timestamp plus interval." now contradicts the new paragraph under it. Replace it rather than keep both.
• The LOCK TABLES block in partition2.test:278 says it only checks "must not lock again", but its result records unexplained 1499/4189 (see Blocker 2). Pin the timestamp, or --echo why they are expected.
• The new footer in main/partition.test:3167 is single-line, while the rest of the file uses the framed three-line --echo # style.
• Commits:
  • 653c28b get_next_time() comment lost its MDEV-25529 prefix and has no body.
  • 8411940 and d1f66a3 have no body, and their subjects no longer cover what they do: subpartitions plus the arena comment, and interval2sec() plus the moved comment.

Test case for Blocker 1

This fails (server crash) on 58e20ec. The expected result was recorded with engine=myisam, which takes the same code path without crashing, so it shows the correct layout.

mysql-test/suite/versioning/t/partition_auto_aria.test:

--source include/have_partition.inc

set @@session.time_zone='+00:00';

--echo #
--echo # MDEV-25529 ALTER ... AUTO crashes on Aria: history range is read
--echo # before the table is locked
--echo #
create table t1 (x int,
  row_start timestamp(6) as row start invisible,
  row_end timestamp(6) as row end invisible,
  period for system_time (row_start, row_end),
  index (row_end, row_start)) with system versioning engine=aria;
set timestamp= unix_timestamp('2000-01-01 00:00:00');
insert t1 values (0);
set timestamp= unix_timestamp('2000-01-01 01:00:00');
update t1 set x= 1;
set timestamp= unix_timestamp('2000-01-01 02:00:00');
update t1 set x= 2;
set timestamp= unix_timestamp('2000-01-01 03:00:00');
alter table t1 partition by system_time interval 1 hour auto;
select x, row_end from t1 partition (p0);
select x, row_end from t1 partition (p1);
select x from t1 partition (pn);
drop table t1;
set timestamp= default;

--echo # End of 13.2 tests

mysql-test/suite/versioning/r/partition_auto_aria.result:

set @@session.time_zone='+00:00';
#
# MDEV-25529 ALTER ... AUTO crashes on Aria: history range is read
# before the table is locked
#
create table t1 (x int,
row_start timestamp(6) as row start invisible,
row_end timestamp(6) as row end invisible,
period for system_time (row_start, row_end),
index (row_end, row_start)) with system versioning engine=aria;
set timestamp= unix_timestamp('2000-01-01 00:00:00');
insert t1 values (0);
set timestamp= unix_timestamp('2000-01-01 01:00:00');
update t1 set x= 1;
set timestamp= unix_timestamp('2000-01-01 02:00:00');
update t1 set x= 2;
set timestamp= unix_timestamp('2000-01-01 03:00:00');
alter table t1 partition by system_time interval 1 hour auto;
select x, row_end from t1 partition (p0);
x	row_end
0	2000-01-01 01:00:00.000000
select x, row_end from t1 partition (p1);
x	row_end
1	2000-01-01 02:00:00.000000
select x from t1 partition (pn);
x
2
drop table t1;
set timestamp= default;
# End of 13.2 tests

[midenok]

Review round 2: rejected items

Everything else from round 2 and the WARN_VERS_STARTS_BEYOND_INTERVAL regression report is fixed. The items below are rejected in full or in part.

Nits

• WARN_VERS_SLOW_ROW_END is raised on nearly every AUTO ALTER: partly rejected.
  • Fixed: the warning is now raised after the scan, and only if the scan read at least one row. Empty tables no longer get it.
  • Fixed: when a suitable row_end index exists but is disabled (ALTER ... DISABLE KEYS), there is no warning, so it never advises adding an index that already exists.
  • Kept: a table that has rows and no row_end-leading index still gets the warning on AUTO ALTER. The full scan is really slow there, and the advice to add the index is correct, so this is intended.

Commits

• 653c28b get_next_time() comment lost its MDEV-25529 prefix and has no body: rejected, author's call.
  • It is a comment-only change to the events code (get_next_time()), independent of the MDEV-25529 logic, and its subject already describes it fully.
  • The comment itself is fixed: the contradicting first line was removed (Nits, event_data_objects.cc).
• 8411940 set_up_default_partitions() ER_OUT_OF_RESOURCES error has no body, and its subject no longer covers subpartitions and the arena comment: rejected, author's call.
  • The set_up_default_subpartitions() change is the same fix (report ER_OUT_OF_RESOURCES on allocation failure) applied to the sibling function.
  • The arena comment documents how partition_element is allocated in the function this commit repairs. Both belong to one concern, and the subject names the user-visible change.

[midenok]

Out of scope: issues in vanilla code, to be fixed under a separate MDEV

These review comments concern code that predates MDEV-25529. They are left for a separate MDEV.

• v1: The old comment above the interval check says the opposite of what the code does.

  [sql/sql_partition.cc:6167] The pre-existing comment /* If interval is changed we can not do fast alter */ sits in the branch taken when the interval is unchanged, unset in the table, or (new in this diff) when AUTO is combined with FORCE. The changed-interval case takes the else branch, which sets *fast_alter_table= true. The diff rewrote this condition and branch body, so a reviewer will read the inverted comment as part of the change. Rewrite it to describe the branch, e.g. "Same or no previous INTERVAL, or AUTO with FORCE: alter a clone of the table's partitioning by copying instead of fast_alter_partition_table()" (cited from coding-style.md:Comments) — by comment-analyzer

• v2: The ALGORITHM error call is copied in three places, two of them old.

  [sql/sql_partition.cc:6256] The new AUTO re-layout block sends the LOCK=NONE rejection through the newly extracted Alter_info::report_lock_none_unsupported(), but inlines the sibling ALGORITHM rejection my_error(ER_ALTER_OPERATION_NOT_SUPPORTED, MYF(0), alter_info->algorithm_clause(thd), "ALGORITHM=COPY"). That makes it the third verbatim copy (also sql/sql_table.cc:11896 and 12050), so the factoring is asymmetric within one block. Either add the ALGORITHM counterpart next to the LOCK helper, or explain the split. (S6) — by type-design-analyzer

• v3: The new partition counting repeats an old loop that uses a different limit.

  [sql/sql_partition.cc:6391] vers_calc_hist_parts() repeats partition_info::vers_set_hist_part()'s auto-create loop (sql/partition_info.cc:866-888): both step boundaries in UTC with date_add_interval() until they pass an end time, and emit the same ER_TOO_MANY_PARTITIONS_ERROR(ME_WARNING) + ER_VERS_HIST_PART_FAILED pair, but with different partition limits (*create_count == MAX_PARTITIONS - 2, ignoring subpartitions, against (n + 1) * num_subparts > MAX_PARTITIONS), so sizing at ALTER and auto-creation at DML can disagree; extract one shared counting helper used by both, or explain the divergence in the commit message (S2/S6) — by code-reviewer

• v4: The to_string() OOM bug is old and also exists in 10.11.

  [sql/sql_type.h:1421] Commit 1df95bbe4cc fixes a temporal to_string() OOM bug that also exists in 10.11 and that nothing else in MDEV-25529 depends on, yet it is filed under MDEV-25529 on main; give it its own MDEV and target the lowest affected branch (cited from commit-and-process.md:Commit structure "Split unrelated changes" and :Branch targeting) — by code-reviewer

• v5: The five to_string() copies were already duplicated before this branch.

  [sql/sql_type.h:1424] The five to_string() bodies (Temporal_hybrid :1424, Interval_DDhhmmssff :1626, Time :2066, Date :2429, Datetime :2756) are the same 4-line block: set_charset / if (str->alloc(MAX_DATE_STRING_REP_LENGTH)) return NULL; / str->length(my_X_to_str(...)) / return str;. Only the formatter differs. The diff patches the same OOM bug five times instead of factoring it once, for example into a protected Temporal helper that takes the formatter as a lambda, so the next fix cannot miss a copy. (S5) — by silent-failure-hunter

• v6: get_clone() never checked its vers_info allocation.

  [sql/sql_partition.cc:6169] The new if (!clone) goto err; covers only part of OOM handling. get_clone() does not check clone->vers_info= new (mem_root) Vers_part_info(...) (partition_info.cc:123) and dereferences it right away, and the new clone->vers_info->copy_settings() adds one more unchecked dereference of it. The gap is pre-existing, but this change now relies on the clone being complete. (silent-failure lens) — by silent-failure-hunter

• v7: The old MAX_DAY_NUMBER comment is unclear.

  [sql/sql_time.h:171] The comment kept for MAX_DAY_NUMBER ("Daynumber from year 0 to 9999-12-31") reads like a count of days over a range. The dropped item_timefunc.cc doxygen comment ("Day number for Dec 31st, 9999.") was clearer. Suggest /** Day number of 9999-12-31, calc_daynr(9999, 12, 31) */. (cited from coding-style.md:Comments) — by comment-analyzer

• v8: The old MAX_DAY_NUMBER is an untyped #define.

  [sql/sql_time.h:172] MAX_DAY_NUMBER moves from two .cc files into a widely included header, still as an untyped #define. Since the diff relocates it anyway, a typed static constexpr constant is the preferred form. (coding-style.md: Constants and magic numbers) — by type-design-analyzer (also flagged by code-reviewer)

• v9: The old range check uses C-style casts.

  [sql/sql_time.h:179] The new header function interval_days_out_of_range() keeps the moved code's C-style (ulonglong) casts; use ulonglong(interval.day) (cited from coding-style.md:Casts) — by code-reviewer (also flagged by type-design-analyzer)

• v10: date_add_interval() can overflow on huge year or month values.

  [sql/sql_partition.cc:6438] [blocking] interval.year/interval.month are stored as unsigned values, but date_add_interval() casts them to long and performs signed arithmetic. An input such as INTERVAL 9223372036854775808 MONTH AUTO therefore reaches this call with an out-of-range value and can overflow before the date-range check, rather than returning ER_VERS_HIST_PART_FAILED. Validate year/month magnitudes before stepping (or use checked arithmetic) so oversized AUTO intervals fail deterministically. — by Copilot

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

[midenok]

Is embedded crash related ?

Maybe, but that was fixed after mreview iterations.