topunix Bugzilla Schema Migration Review
Bugzilla upgrade code runs unconditionally on every checksetup.pl/install
invocation, against installations at every prior schema version — so every
migration in Bugzilla::Install::DB must be safe to run against a database
that already has the change applied. Portability across MySQL, PostgreSQL,
and SQLite is enforced by an actual test (t/013db_portability.t), not just
convention. Sources are in reference/; nothing here is asserted without a
citation to the public bugzilla/harmony source tree.
Idempotency pattern
The standard pattern in Bugzilla::Install::DB is: check whether the
change already exists, and only apply it if not.
# Column
if (!$dbh->bz_column_info('fielddefs', 'obsolete')) {
$dbh->bz_add_column('fielddefs', 'obsolete',
{TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});
}
# Index
if (!$dbh->bz_index_info('email_rates', 'email_rates_message_ts_idx')) {
$dbh->bz_add_index('email_rates', 'email_rates_message_ts_idx',
['message_ts']);
}
When reviewing a new migration function:
- A bare
$dbh->bz_add_column(...)with no precedingbz_column_infocheck is a review finding — it will error (or silently duplicate, depending on driver) on a second run. bz_alter_columnis self-guarding: per its POD, "If the new type is the same as the old type, the function returns without changing anything," so it doesn't need a manual existence check the waybz_add_columndoes.bz_drop_column, per its POD, "returns without doing anything" if the column doesn't exist — also safe to call unconditionally.bz_add_columncannot add aNOT NULLcolumn with noDEFAULT, because "the database won't know what to set all the NULL values to" on existing rows — a migration adding aNOTNULL => 1column must also supplyDEFAULT.
Abstract schema over DB-specific SQL
Column types are declared via Bugzilla::DB::Schema's ABSTRACT_SCHEMA
vocabulary (INT1, INT2, INT3, INT4, MEDIUMSERIAL, BIGSERIAL,
MEDIUMTEXT, LONGBLOB, DATETIME, BOOLEAN, etc.), never a raw
MySQL/Postgres/SQLite column type string. Per its POD, this module "should
be considered package-private to the Bugzilla::DB module" — a migration
that constructs SQL type strings directly instead of going through
ABSTRACT_SCHEMA/bz_add_column is bypassing the one place that knows how
to translate a type per-database.
The same discipline applies to functions, not just types.
t/013db_portability.t scans Perl source and fails the build on any of
these raw SQL function calls outside Bugzilla/DB/:
| Forbidden (DB-specific) | Required (Bugzilla::DB method) |
|---|---|
UNIX_TIMESTAMP() |
$dbh->sql_date_to_epoch(...) |
DATE_FORMAT() |
$dbh->sql_date_format(...) |
CONCAT() |
$dbh->sql_string_concat(...) |
POSITION() / INSTR() / LOCATE() |
$dbh->sql_position(...) / sql_iposition(...) |
GROUP_CONCAT() |
$dbh->sql_group_concat(...) |
Any of these appearing literally in a patch's SQL (outside the DB driver modules themselves) is a portability bug, not a style nit — it will pass on whichever database the author tested against and break on the others.
Review checklist
- Does every
bz_add_column/bz_add_indexcall have a guardingbz_column_info/bz_index_infocheck, or is it one of the self-guarding calls (bz_alter_column,bz_drop_column)? - Does every new
NOTNULLcolumn also specifyDEFAULT? - Are all types expressed in
ABSTRACT_SCHEMAvocabulary, not a raw database-specific type string? - Does the SQL avoid the forbidden vendor-specific functions listed above,
using the
Bugzilla::DBportability methods instead? - Is the new migration function actually invoked from the upgrade driver
(added to the sequence
Bugzilla::Install::DBruns), not just defined?
See reference/db-abstraction.md and reference/portability-checklist.md
for full source quotes.