fix(postgres): keep index operator classes in read and generation

Closes #7166
This commit is contained in:
Tara authored and GitHub committed 2026-09-04 14:48:41 +08:00
1 parent 09f8d095d2
commit 01623ab38d
31 files changed
+854 -51

No files matched your search

@@ -114,6 +114,45 @@ pub(super) fn has_existing_index_change(index: &EditableStructureIndex) -> bool
|| index_list_changed(&index.included_columns, original.included_columns.as_ref())
|| clean(&index.filter) != clean(original.filter.as_deref().unwrap_or(""))
|| clean(&index.comment) != clean(original.comment.as_deref().unwrap_or(""))
|| index_opclasses_changed(
&index.column_opclasses,
&original.column_opclasses,
&index.columns,
&original.columns,
)
}
/// Compare opclass arrays positionally, skipping expression columns.
/// When the column list has changed we already detect that above; here we only
/// compare opclasses for columns that exist in both the edited and original lists.
fn index_opclasses_changed(
next: &[Option<String>],
previous: &[Option<String>],
next_cols: &[String],
prev_cols: &[String],
) -> bool {
// If the edited side has explicit opclasses, compare positionally.
if !next.is_empty() {
// Length mismatch (shouldn't happen when columns are in lockstep, but
// defend against it): any difference in length is a change.
if next.len() != previous.len() {
return true;
}
return next.iter().zip(previous.iter()).any(|(n, p)| {
let n_flat = n.as_deref().map(clean).filter(|v| !v.is_empty());
let p_flat = p.as_deref().map(clean).filter(|v| !v.is_empty());
n_flat != p_flat
});
}
// If the edited side has no opclasses, check whether the original side
// had any non-default opclasses for columns still present.
if previous.iter().any(|o| o.as_deref().map(|v| !clean(v).is_empty()).unwrap_or(false)) {
let prev_set: std::collections::HashSet<_> = prev_cols.iter().map(|c| clean(c)).collect();
return next_cols.iter().zip(previous.iter()).any(|(nc, po)| {
po.as_deref().map(|v| !clean(v).is_empty()).unwrap_or(false) && prev_set.contains(&clean(nc))
});
}
false
}
pub(super) fn index_list_changed(next: &[String], previous: Option<&Vec<String>>) -> bool {
@@ -170,14 +209,19 @@ fn mysql_index_column_sql(column: &str) -> String {
}
}
fn postgres_index_column_sql(column: &str, is_expression: bool) -> String {
// Expression/functional index key parts arrive as raw expression text, not a plain
// column name; quoting the whole expression as an identifier turns it into a literal
// column reference that doesn't exist (#6295).
if is_expression {
column.trim().to_string()
} else {
quote_ident(StructureDialect::Postgres, column)
fn postgres_index_column_sql(column: &str, is_expression: bool, opclass: Option<&str>) -> String {
// The base key text: a real column is quoted as an identifier; an expression/functional
// key part arrives as raw expression text (from the per-column `pg_get_indexdef`, which
// omits the opclass — see `list_indexes_with_sql`), so quoting the whole thing as an
// identifier would turn it into a nonexistent column reference (#6295).
let base =
if is_expression { column.trim().to_string() } else { quote_ident(StructureDialect::Postgres, column.trim()) };
// The opclass is read separately from `pg_index.indclass` for every key position
// (including expression keys) and appended uniformly — it never lives inside the
// expression text, so there is no duplication risk.
match opclass.filter(|o| !o.is_empty()) {
Some(opc) => format!("{} {}", base, opc),
None => base,
}
}
@@ -219,6 +263,49 @@ fn key_expression_flags(index: &EditableStructureIndex, columns: &[String]) -> V
.collect()
}
/// Per-key operator class provenance for `columns` (the edited/current key list).
///
/// Honors explicit per-key opclasses (`index.column_opclasses`) positionally when
/// they are aligned to `columns` (non-empty, same length) and represent a genuine
/// edit: either there is no `original` (a newly created index, where the UI
/// authors `column_opclasses` in lockstep with `columns`), or the edited
/// opclasses differ from the round-tripped `original.column_opclasses`. A stale
/// copy left behind by a column toggle/reorder — still equal to the original —
/// falls through to the order-independent text-matching below, so opclasses are
/// never misassigned positionally before the editor maintains `column_opclasses`
/// in lockstep with `columns`. Once that lockstep is in place the guard always
/// passes and the honor branch is authoritative.
fn key_opclasses(index: &EditableStructureIndex, columns: &[String]) -> Vec<Option<String>> {
if !index.column_opclasses.is_empty()
&& index.column_opclasses.len() == columns.len()
&& index.original.as_ref().is_none_or(|original| original.column_opclasses != index.column_opclasses)
{
return index.column_opclasses.to_vec();
}
let original = match &index.original {
Some(original) if !original.column_opclasses.is_empty() => original,
_ => return vec![None; columns.len()],
};
let mut consumed = vec![false; original.columns.len()];
columns
.iter()
.map(|column| {
let claimed = original
.columns
.iter()
.enumerate()
.find(|(i, original_column)| !consumed[*i] && *original_column == column);
match claimed {
Some((i, _)) => {
consumed[i] = true;
original.column_opclasses.get(i).cloned().flatten()
}
None => None,
}
})
.collect()
}
pub(super) fn build_drop_index_sql(
database_type: Option<DatabaseType>,
dialect: StructureDialect,
@@ -278,6 +365,7 @@ pub(super) fn build_create_index_statements(
let unique = if index.is_unique { "UNIQUE " } else { "" };
let replace = if or_replace { "OR REPLACE " } else { "" };
let key_is_expression = key_expression_flags(index, &columns);
let key_opclasses = key_opclasses(index, &columns);
let cols = columns
.iter()
.enumerate()
@@ -287,7 +375,7 @@ pub(super) fn build_create_index_statements(
} else if dialect == StructureDialect::GaussdbM {
gaussdbm_index_column_sql(column, key_is_expression[i])
} else if dialect == StructureDialect::Postgres {
postgres_index_column_sql(column, key_is_expression[i])
postgres_index_column_sql(column, key_is_expression[i], key_opclasses[i].as_deref())
} else if for_new_table {
quote_new_ident(database_type, dialect, column)
} else {
@@ -1694,6 +1694,7 @@ mod tests {
filter: String::new(),
index_type: String::new(),
included_columns: Vec::new(),
column_opclasses: Vec::new(),
comment: String::new(),
concurrently: false,
original: None,
@@ -2013,6 +2014,7 @@ mod tests {
filter: String::new(),
index_type: String::new(),
included_columns: Vec::new(),
column_opclasses: Vec::new(),
comment: String::new(),
concurrently: false,
original: Some(IndexInfo {
@@ -2025,6 +2027,7 @@ mod tests {
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
}),
marked_for_drop: true,
});
@@ -120,6 +120,7 @@ fn index(name: &str, columns: &[&str]) -> EditableStructureIndex {
filter: String::new(),
index_type: String::new(),
included_columns: Vec::new(),
column_opclasses: Vec::new(),
comment: String::new(),
concurrently: false,
original: None,
@@ -140,6 +141,7 @@ fn existing_index(name: &str, columns: &[&str], is_unique: bool) -> EditableStru
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
});
index
}
@@ -231,6 +233,7 @@ fn builds_mysql_column_and_index_changes() {
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
});
let mut email_index = index("uniq_users_email", &["email"]);
email_index.is_unique = true;
@@ -870,6 +873,7 @@ fn builds_informix_column_and_index_changes() {
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
});
let mut email_index = index("uniq_users_email", &["email"]);
email_index.is_unique = true;
@@ -1305,6 +1309,7 @@ fn iris_drop_index_includes_table_name() {
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
});
let result = build_table_structure_change_sql(TableStructureSqlOptions {
@@ -1609,6 +1614,7 @@ fn gbase8a_uses_limited_mysql_ddl() {
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
});
let result = build_table_structure_change_sql(TableStructureSqlOptions {
@@ -2226,6 +2232,7 @@ fn qualifies_attached_sqlite_table_and_index_changes() {
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
});
let email_index = index("idx_users_email", &["email"]);
@@ -6728,6 +6735,7 @@ fn oscar_drop_index_with_schema_qualifier() {
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
});
let result = build_table_structure_change_sql(TableStructureSqlOptions {
@@ -7184,6 +7192,7 @@ fn gaussdb_m_index(name: &str, columns: &[&str]) -> EditableStructureIndex {
filter: String::new(),
index_type: String::new(),
included_columns: Vec::new(),
column_opclasses: Vec::new(),
comment: String::new(),
concurrently: false,
original: None,
@@ -7210,6 +7219,7 @@ fn gaussdb_m_existing_index(
included_columns: None,
comment: None,
key_is_expression: Vec::new(),
column_opclasses: vec![],
});
idx
}
@@ -7542,3 +7552,244 @@ fn mysql_add_column_nullable_timestamp_without_default_gets_explicit_null() {
assert_eq!(result.warnings, Vec::<String>::new());
assert_eq!(result.statements, vec!["ALTER TABLE `u7_game_order_step` ADD COLUMN `updated_at` timestamp NULL;"]);
}
// ── PostgreSQL operator class tests ──
#[test]
fn postgres_gin_index_with_explicit_opclass_generates_opclass_in_ddl() {
let mut idx = index("idx_name_trgm", &["name"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![Some("gin_trgm_ops".to_string())];
let result = build_table_structure_change_sql(index_change_options(DatabaseType::Postgres, Some("public"), idx));
assert!(result.warnings.is_empty());
let sql = result.statements.join("\n");
assert!(sql.contains(r#""name" gin_trgm_ops"#), "Expected opclass in DDL, got: {sql}");
assert!(sql.contains("USING GIN"), "Expected USING GIN, got: {sql}");
}
#[test]
fn postgres_index_opclass_from_original_when_editable_empty() {
// When the editable side has no opclass, fall back to original matching.
let mut idx = index("idx_name_trgm", &["name"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![]; // empty: fall back to original
idx.original = Some(IndexInfo {
name: "idx_name_trgm".to_string(),
columns: vec!["name".to_string()],
is_unique: false,
is_primary: false,
filter: None,
index_type: Some("gin".to_string()),
included_columns: None,
comment: None,
key_is_expression: vec![false],
column_opclasses: vec![Some("gin_trgm_ops".to_string())],
});
let result = build_table_structure_change_sql(index_change_options(DatabaseType::Postgres, Some("public"), idx));
assert!(result.warnings.is_empty());
let sql = result.statements.join("\n");
assert!(sql.contains(r#""name" gin_trgm_ops"#), "Expected opclass from original, got: {sql}");
}
#[test]
fn postgres_index_no_rebuild_when_original_has_opclass_and_edit_is_identical() {
// When the edited index matches the original (including opclass), no rebuild.
let mut idx = index("idx_name_trgm", &["name"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![Some("gin_trgm_ops".to_string())];
idx.original = Some(IndexInfo {
name: "idx_name_trgm".to_string(),
columns: vec!["name".to_string()],
is_unique: false,
is_primary: false,
filter: None,
index_type: Some("gin".to_string()),
included_columns: None,
comment: None,
key_is_expression: vec![false],
column_opclasses: vec![Some("gin_trgm_ops".to_string())],
});
let result = build_table_structure_change_sql(index_change_options(DatabaseType::Postgres, Some("public"), idx));
assert!(result.warnings.is_empty());
assert!(result.statements.is_empty(), "Expected no rebuild for unchanged index, got: {:?}", result.statements);
}
#[test]
fn postgres_index_rebuild_when_opclass_changed() {
// When opclass changes, the index should be rebuilt.
let mut idx = index("idx_name_trgm", &["name"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![Some("gin_trgm_ops".to_string())];
idx.original = Some(IndexInfo {
name: "idx_name_trgm".to_string(),
columns: vec!["name".to_string()],
is_unique: false,
is_primary: false,
filter: None,
index_type: Some("gin".to_string()),
included_columns: None,
comment: None,
key_is_expression: vec![false],
column_opclasses: vec![None], // was default, now gin_trgm_ops
});
let result = build_table_structure_change_sql(index_change_options(DatabaseType::Postgres, Some("public"), idx));
assert!(result.warnings.is_empty());
assert!(!result.statements.is_empty(), "Expected rebuild when opclass changes");
let sql = result.statements.join("\n");
assert!(sql.contains("DROP INDEX"), "Expected DROP INDEX, got: {sql}");
assert!(sql.contains("gin_trgm_ops"), "Expected new opclass in DDL, got: {sql}");
}
#[test]
fn postgres_gin_varchar_no_opclass_warns() {
let mut idx = index("idx_name_search", &["name"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![];
let mut options = index_change_options(DatabaseType::Postgres, Some("public"), idx);
// Add a varchar column for the warning to match against.
options.columns.push(EditableStructureColumn {
id: "col_name".to_string(),
name: "name".to_string(),
data_type: "character varying(255)".to_string(),
is_nullable: true,
default_value: String::new(),
comment: String::new(),
is_primary_key: false,
extra: None,
original: None,
original_position: None,
marked_for_drop: false,
character_set: String::new(),
collation: String::new(),
});
let result = build_table_structure_change_sql(options);
assert!(
result.warnings.iter().any(|w| w.contains("operator class") && w.contains("idx_name_search")),
"Expected opclass warning for GIN+varchar, got: {:?}",
result.warnings
);
}
#[test]
fn postgres_gin_text_column_with_explicit_opclass_no_warning() {
let mut idx = index("idx_body_search", &["body"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![Some("gin_trgm_ops".to_string())];
let mut options = index_change_options(DatabaseType::Postgres, Some("public"), idx);
options.columns.push(EditableStructureColumn {
id: "col_body".to_string(),
name: "body".to_string(),
data_type: "text".to_string(),
is_nullable: true,
default_value: String::new(),
comment: String::new(),
is_primary_key: false,
extra: None,
original: None,
original_position: None,
marked_for_drop: false,
character_set: String::new(),
collation: String::new(),
});
let result = build_table_structure_change_sql(options);
assert!(
!result.warnings.iter().any(|w| w.contains("operator class")),
"Expected NO warning when opclass is set, got: {:?}",
result.warnings
);
}
#[test]
fn postgres_expression_index_appends_opclass() {
// The per-column `pg_get_indexdef(indexrelid, colno, pretty)` returns only the
// bare expression (PostgreSQL sets `attrsOnly = (colno != 0)`, so the
// opclass/COLLATE/DESC block is skipped — see `ruleutils.c` and the note on
// `list_indexes_with_sql`). The opclass is read separately from `indclass`
// into `column_opclasses`, so the generator must append it to an expression
// key just like a real column.
let mut idx = index("idx_lower_email", &["lower(email)"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![Some("gin_trgm_ops".to_string())];
idx.original = Some(IndexInfo {
name: "idx_lower_email".to_string(),
columns: vec!["lower(email)".to_string()],
is_unique: false,
is_primary: false,
filter: None,
index_type: Some("gin".to_string()),
included_columns: None,
comment: None,
key_is_expression: vec![true],
column_opclasses: vec![None],
});
let result = build_table_structure_change_sql(index_change_options(DatabaseType::Postgres, Some("public"), idx));
assert!(result.warnings.is_empty());
let sql = result.statements.join("\n");
assert!(sql.contains("lower(email) gin_trgm_ops"), "Expression should have opclass appended, got: {sql}");
}
#[test]
fn postgres_jsonb_gin_with_jsonb_path_ops() {
let mut idx = index("idx_metadata", &["metadata"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![Some("jsonb_path_ops".to_string())];
let result = build_table_structure_change_sql(index_change_options(DatabaseType::Postgres, Some("public"), idx));
assert!(result.warnings.is_empty());
let sql = result.statements.join("\n");
assert!(sql.contains(r#""metadata" jsonb_path_ops"#), "Expected jsonb_path_ops in DDL, got: {sql}");
assert!(sql.contains("USING GIN"), "Expected USING GIN, got: {sql}");
}
#[test]
fn postgres_expression_index_opclass_round_trips_from_indclass() {
// Real PostgreSQL introspection: the expression text from
// `pg_get_indexdef(indexrelid, colno, false)` is the BARE expression
// (`attrsOnly = (colno != 0)` drops the opclass block — see `ruleutils.c`).
// The opclass is read separately from `pg_index.indclass` (schema-qualified)
// into `column_opclasses`, so the generator appends it to the bare expression
// rather than expecting it to be embedded in the text.
let mut idx = index("idx_lower_email_trgm", &["lower(email)"]);
idx.index_type = "GIN".to_string();
idx.column_opclasses = vec![Some("public.gin_trgm_ops".to_string())];
idx.original = Some(IndexInfo {
name: "idx_lower_email_trgm_old".to_string(), // different name → forces rebuild
columns: vec!["lower(email)".to_string()],
is_unique: false,
is_primary: false,
filter: None,
index_type: Some("gin".to_string()),
included_columns: None,
comment: None,
key_is_expression: vec![true],
column_opclasses: vec![Some("public.gin_trgm_ops".to_string())],
});
let result = build_table_structure_change_sql(index_change_options(DatabaseType::Postgres, Some("public"), idx));
assert!(result.warnings.is_empty());
let sql = result.statements.join("\n");
assert!(
sql.contains("lower(email) public.gin_trgm_ops"),
"Expected bare expression + schema-qualified opclass, got: {sql}"
);
// The bare expression text no longer carries the opclass, so it cannot be duplicated.
assert!(!sql.contains("gin_trgm_ops gin_trgm_ops"), "Opclass must not be duplicated, got: {sql}");
}
@@ -94,6 +94,11 @@ pub struct EditableStructureIndex {
pub index_type: String,
#[serde(default)]
pub included_columns: Vec<String>,
/// Parallel to `columns`: operator class for each key column (PostgreSQL).
/// `None` means default operator class. The UI keeps this array in lockstep
/// with `columns`; when empty, opclasses fall back to `original` matching.
#[serde(default)]
pub column_opclasses: Vec<Option<String>>,
#[serde(default)]
pub comment: String,
#[serde(default)]
@@ -125,6 +130,9 @@ pub struct IndexInfo {
/// (e.g. sourced from `pg_get_indexdef`), not a plain column name.
#[serde(default)]
pub key_is_expression: Vec<bool>,
/// Parallel to `columns`: operator class name for each key column, if non-default.
#[serde(default)]
pub column_opclasses: Vec<Option<String>>,
}
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
@@ -43,6 +43,7 @@ pub(super) fn validate_draft(options: &TableStructureSqlOptions) -> Vec<String>
let active_columns: Vec<_> = options.columns.iter().filter(|column| !column.marked_for_drop).collect();
validate_columns(&active_columns, &mut warnings);
validate_dameng_identity(options, &active_columns, &mut warnings);
validate_gin_opclass_warnings(options, &mut warnings);
for index in options
.indexes
.iter()
@@ -101,3 +102,67 @@ pub(super) fn validate_columns(columns: &[&EditableStructureColumn], warnings: &
}
}
}
/// PostgreSQL GIN indexes on varchar/text/character columns without an explicit
/// operator class are a common trap: the default `array_ops` class is rarely what
/// the user wants, and `gin_trgm_ops` (from `pg_trgm` extension) or similar
/// classes are almost always needed. This is a warning, not an error, because
/// the default class may be deliberate in some cases.
fn validate_gin_opclass_warnings(options: &TableStructureSqlOptions, warnings: &mut Vec<String>) {
let capabilities = capabilities_for(options.database_type);
if capabilities.dialect != StructureDialect::Postgres {
return;
}
// Build a lookup: column name (lowercased) → data type for active columns.
let col_types: std::collections::HashMap<String, &str> = options
.columns
.iter()
.filter(|col| !col.marked_for_drop)
.map(|col| (clean(&col.name).to_lowercase(), col.data_type.as_str()))
.collect();
let text_like = |dt: &str| -> bool {
let lower = dt.to_lowercase();
lower == "text"
|| lower.starts_with("character varying")
|| lower.starts_with("varchar")
|| lower.starts_with("character")
|| lower == "char"
};
for index in options.indexes.iter().filter(|idx| {
!idx.marked_for_drop
&& clean(&idx.index_type).eq_ignore_ascii_case("GIN")
&& (idx.original.is_none() || has_existing_index_change(idx))
}) {
for (i, col) in index.columns.iter().enumerate() {
let col_name = clean(col);
if col_name.is_empty() {
continue;
}
// Expression columns are not matched against table column types.
let is_expr =
index.original.as_ref().and_then(|orig| orig.key_is_expression.get(i).copied()).unwrap_or(false);
if is_expr {
continue;
}
// Check if this column has a text-like type.
let Some(dt) = col_types.get(&col_name.to_lowercase()) else {
continue;
};
if !text_like(dt) {
continue;
}
// Check if opclass is explicitly provided.
let has_opclass =
index.column_opclasses.get(i).and_then(|o| o.as_deref()).map(|v| !clean(v).is_empty()).unwrap_or(false);
if !has_opclass {
warnings.push(format!(
"GIN index \"{}\" on column \"{}\" (type {}) has no operator class specified. Consider adding an operator class, e.g. `gin_trgm_ops` (requires pg_trgm extension) for text search.",
index.name, col_name, dt
));
}
}
}
}