Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 88 additions & 1 deletion crates/dbx-core/src/sql_dialect/identifiers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,11 @@ pub fn quote_table_identifier(database_type: Option<DatabaseType>, name: &str) -
}
}

pub(crate) fn quote_gaussdb_jdbc_identifier(name: &str, identifier_quote: &str) -> String {
pub(crate) fn quote_gaussdb_jdbc_identifier(
name: &str,
identifier_quote: &str,
database_type: Option<DatabaseType>,
) -> String {
if is_explicitly_quoted_identifier(name) {
return name.to_string();
}
Expand All @@ -157,6 +161,8 @@ pub(crate) fn quote_gaussdb_jdbc_identifier(name: &str, identifier_quote: &str)
}
let requires_quote = !is_simple_lower_identifier(name)
|| is_postgres_reserved_identifier(name)
|| (matches!(database_type, Some(DatabaseType::Gaussdb | DatabaseType::OpenGauss))
&& is_gaussdb_only_reserved_identifier(name))
|| (quote == "`" && is_mysql_only_reserved_identifier(name));
if !requires_quote {
return name.to_string();
Expand Down Expand Up @@ -287,6 +293,60 @@ fn is_postgres_reserved_identifier(name: &str) -> bool {
)
}

/// Words GaussDB/openGauss reserve that plain PostgreSQL does not, so they
/// are missed by [`is_postgres_reserved_identifier`] alone (t8y2/dbx#6283).
/// For example `compact` is a bare lowercase identifier that passes
/// [`is_simple_lower_identifier`] and isn't a Postgres keyword, but GaussDB
/// reserves it and rejects it unquoted in DDL.
///
/// An initial cut of this list was diffed from Huawei's GaussDB(DWS)
/// keyword reference, but DWS is the MPP/columnar variant and isn't a
/// reliable proxy for the core engine's catalog. It was cross-checked
/// against a writable openGauss 5.0.0 instance's own `pg_get_keywords()`,
/// which is the authoritative source per openGauss's docs. That check
/// found 9 of the original words are not actually reserved on core
/// GaussDB/openGauss — `hot`, `nlssort`, and `warmup` aren't keywords at
/// all, and `fenced`, `internal`, `plan`, `tsfield`, `tstag`, and `tstime`
/// are `unreserved` — so quoting them would have reintroduced the
/// case-locking bug this PR fixes. They've been dropped. `compact` was
/// additionally confirmed by running `CREATE TABLE ... (compact int)`
/// unquoted, which fails with `ERROR: syntax error at or near "compact"`.
///
/// A follow-up review on #6283 diffed this list against the *full*
/// `pg_get_keywords()` result set from that same instance (653 rows) rather
/// than spot-checking individual words, and found 7 more reserved words
/// this hand-picked list had missed: `csn`, `excluded`, `groupparent`,
/// `nocycle`, `rownum`, `shrink`, `verify`. All 22 words below have
/// `catcode` `R` (`reserved`) or `T` (`reserved, can be function or type
/// name`) on that instance.
fn is_gaussdb_only_reserved_identifier(name: &str) -> bool {
matches!(
name,
"authid"
| "buckets"
| "compact"
| "csn"
| "deltamerge"
| "excluded"
| "groupparent"
| "hdfsdirectory"
| "less"
| "maxvalue"
| "minus"
| "modify"
| "nocycle"
| "performance"
| "procedure"
| "recyclebin"
| "reject"
| "rownum"
| "shrink"
| "sysdate"
| "timecapsule"
| "verify"
)
}

fn is_mysql_only_reserved_identifier(name: &str) -> bool {
matches!(
name,
Expand Down Expand Up @@ -385,10 +445,37 @@ pub(crate) fn quote_transfer_identifier(name: &str, database_type: &DatabaseType
| DatabaseType::Spark
| DatabaseType::Questdb => format!("`{}`", name.replace('`', "``")),
DatabaseType::SqlServer => format!("[{}]", name.replace(']', "]]")),
_ if needs_conditional_quoting_for_gaussdb_family(database_type) => {
if is_simple_lower_identifier(name)
&& !is_postgres_reserved_identifier(name)
&& !is_gaussdb_only_reserved_identifier(name)
{
name.to_string()
} else {
format!("\"{}\"", name.replace('\"', "\"\""))
}
}
_ => format!("\"{}\"", name.replace('\"', "\"\"")),
}
}

/// GaussDB/OpenGauss can run in an Oracle-compatible mode where *unquoted*
/// identifiers fold to uppercase instead of Postgres' lowercase. Transfer
/// always quoting every identifier therefore locks in the source's exact
/// (often lowercase) case; a later unquoted reference from the user's own
/// tooling then folds to a different case and the target reports "column
/// does not exist" even though the migrated column is right there
/// (t8y2/dbx#6205). Only quote when the name actually needs it (mixed case,
/// special characters, or a reserved word) — matching the same heuristic
/// already used for GaussDB JDBC identifier quoting in
/// `quote_gaussdb_jdbc_identifier` above. Deliberately scoped to just
/// GaussDB/OpenGauss rather than the wider Postgres family (see
/// `transfer::is_postgres_family_target`): those other engines have no
/// reported case-folding quirk, and always-quoting them is harmless.
fn needs_conditional_quoting_for_gaussdb_family(database_type: &DatabaseType) -> bool {
matches!(database_type, DatabaseType::Gaussdb | DatabaseType::OpenGauss)
}

/// Qualified table name for transfer SQL.
///
/// * Without catalog: produces `schema.table` (or just `table` for MySQL family).
Expand Down
2 changes: 1 addition & 1 deletion crates/dbx-core/src/sql_dialect/table_select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ pub(crate) fn quote_table_data_identifier(
return quote_table_identifier(database_type, name);
};
if matches!(database_type, Some(DatabaseType::Gaussdb | DatabaseType::OpenGauss | DatabaseType::Postgres)) {
return quote_gaussdb_jdbc_identifier(name, quote);
return quote_gaussdb_jdbc_identifier(name, quote, database_type);
}
if quote.is_empty() {
return name.to_string();
Expand Down
70 changes: 70 additions & 0 deletions crates/dbx-core/src/sql_dialect/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,76 @@ fn quotes_gaussdb_jdbc_identifiers_selectively() {
}
}

#[test]
fn quotes_gaussdb_only_reserved_words_not_shared_with_postgres() {
// t8y2/dbx#6283: `compact` and friends are reserved in GaussDB/openGauss
// but are not PostgreSQL keywords, so a check that only consults the
// Postgres reserved-word set misses them and would emit invalid unquoted
// DDL on the real target. This word list is cross-checked against a
// writable openGauss 5.0.0 instance's own `pg_get_keywords()` (not just
// Huawei's GaussDB(DWS) doc) — see the comment on
// `is_gaussdb_only_reserved_identifier` for how the DWS-derived list was
// trimmed down to these instance-confirmed words. `csn` through `verify`
// below were caught by diffing against the instance's full 653-row
// keyword catalog rather than spot-checking individual words.
for name in [
"authid",
"buckets",
"compact",
"csn",
"deltamerge",
"excluded",
"groupparent",
"hdfsdirectory",
"less",
"maxvalue",
"minus",
"modify",
"nocycle",
"performance",
"procedure",
"recyclebin",
"reject",
"rownum",
"shrink",
"sysdate",
"timecapsule",
"verify",
] {
assert_eq!(
quote_table_data_identifier(Some(DatabaseType::Gaussdb), name, Some("\"")),
format!("\"{name}\""),
"GaussDB must quote target-only reserved word `{name}`"
);
assert_eq!(
quote_table_data_identifier(Some(DatabaseType::OpenGauss), name, Some("\"")),
format!("\"{name}\""),
"OpenGauss must quote target-only reserved word `{name}`"
);
}

// The same words are not reserved in real PostgreSQL, so the dialect-aware
// check must not over-quote a genuine Postgres target.
for name in ["compact", "buckets", "sysdate", "minus", "modify", "excluded", "rownum", "verify"] {
assert_eq!(
quote_table_data_identifier(Some(DatabaseType::Postgres), name, Some("\"")),
name,
"plain Postgres must not quote `{name}` — it is not a Postgres keyword"
);
}

// Words that were in the original DWS-derived list but turned out not to
// be reserved on a real GaussDB/openGauss instance must not be quoted —
// over-quoting them would reintroduce the case-locking bug this PR fixes.
for name in ["fenced", "hot", "internal", "nlssort", "plan", "tsfield", "tstag", "tstime", "warmup"] {
assert_eq!(
quote_table_data_identifier(Some(DatabaseType::Gaussdb), name, Some("\"")),
name,
"GaussDB must not quote `{name}` — confirmed not reserved on a real instance"
);
}
}

#[test]
fn qualifies_schema_only_for_schema_aware_databases() {
assert_eq!(qualified_table_name(Some(DatabaseType::Postgres), Some("public"), "users"), "\"public\".\"users\"");
Expand Down
175 changes: 175 additions & 0 deletions crates/dbx-core/src/transfer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8827,6 +8827,181 @@ mod tests {
);
}

#[test]
fn gaussdb_create_table_does_not_quote_simple_lowercase_identifiers() {
// Regression test for t8y2/dbx#6205: migrating a MySQL table with plain
// lowercase/snake_case identifiers to GaussDB used to quote every column
// and the table/schema name, which locks in exact case. GaussDB can fold
// *unquoted* references to a different case (e.g. its Oracle-compatible
// mode), so a later unquoted query against the migrated table fails with
// "column does not exist" even though the column is right there.
let cols = vec![
db::ColumnInfo { is_primary_key: true, ..test_column("id", "bigint") },
test_column("created_at", "datetime"),
test_column("updated_at", "datetime"),
test_column("deleted_at", "datetime"),
test_column("categories_id", "bigint"),
test_column("background_image_id", "text"),
test_column("level", "int"),
test_column("parent_id", "bigint"),
];

let ddl = generate_create_table_ddl(
&cols,
"background_categories_img",
"",
"public",
&DatabaseType::Gaussdb,
&DatabaseType::Mysql,
None,
None,
);

assert!(
ddl.starts_with("CREATE TABLE IF NOT EXISTS public.background_categories_img ("),
"table/schema must be unquoted, ddl: {ddl}"
);
for column in [
"id",
"created_at",
"updated_at",
"deleted_at",
"categories_id",
"background_image_id",
"level",
"parent_id",
] {
assert!(ddl.contains(&format!(" {column} ")), "column `{column}` must be unquoted, ddl: {ddl}");
}
assert!(ddl.contains("PRIMARY KEY (id)"), "primary key must be unquoted, ddl: {ddl}");
assert!(!ddl.contains('"'), "no identifier should need quoting, ddl: {ddl}");
}

#[test]
fn gaussdb_create_table_still_quotes_identifiers_that_need_it() {
let cols = vec![
test_column("Order", "int"), // mixed case
test_column("select", "text"), // reserved word
];

let ddl = generate_create_table_ddl(
&cols,
"t",
"",
"public",
&DatabaseType::Gaussdb,
&DatabaseType::Mysql,
None,
None,
);

assert!(ddl.contains("\"Order\""), "mixed-case column must stay quoted, ddl: {ddl}");
assert!(ddl.contains("\"select\""), "reserved-word column must stay quoted, ddl: {ddl}");
}

#[test]
fn gaussdb_create_table_quotes_target_only_reserved_words() {
// t8y2/dbx#6283: `compact` is a plain lowercase identifier that isn't a
// Postgres reserved word, so relying solely on the Postgres reserved-word
// set would leave it unquoted — but GaussDB reserves it (Huawei's
// GaussDB(DWS) keyword reference lists it as "Reserved (functions and
// types allowed)"), and unquoted `compact` is invalid in GaussDB DDL.
// `rownum`/`verify` (used below as the table/schema names) were caught
// by a follow-up full-catalog diff — see `is_gaussdb_only_reserved_identifier`.
let cols = vec![test_column("compact", "int"), test_column("sysdate", "text"), test_column("excluded", "text")];

let ddl = generate_create_table_ddl(
&cols,
"rownum",
"",
"verify",
&DatabaseType::Gaussdb,
&DatabaseType::Mysql,
None,
None,
);

assert!(ddl.contains("\"verify\".\"rownum\""), "target-only reserved schema/table must be quoted, ddl: {ddl}");
assert!(ddl.contains("\"compact\""), "target-only reserved word must be quoted, ddl: {ddl}");
assert!(ddl.contains("\"sysdate\""), "target-only reserved word must be quoted, ddl: {ddl}");
assert!(ddl.contains("\"excluded\""), "target-only reserved word must be quoted, ddl: {ddl}");

let opengauss_ddl = generate_create_table_ddl(
&cols,
"rownum",
"",
"verify",
&DatabaseType::OpenGauss,
&DatabaseType::Mysql,
None,
None,
);
assert!(
opengauss_ddl.contains("\"compact\""),
"OpenGauss must also quote target-only reserved word, ddl: {opengauss_ddl}"
);
assert!(
opengauss_ddl.contains("\"verify\".\"rownum\""),
"OpenGauss must also quote target-only reserved schema/table, ddl: {opengauss_ddl}"
);
}

#[test]
fn gaussdb_insert_quotes_target_only_reserved_words() {
// t8y2/dbx#6283 follow-up: the INSERT path shares `quote_transfer_identifier`
// with CREATE TABLE, but had no dedicated regression test — cover table,
// schema, and column names here with words missed by the original
// Postgres-only reserved-word check (`csn`, `groupparent`, `nocycle`,
// `shrink` weren't covered by the CREATE TABLE test above either).
let columns = vec!["id".to_string(), "csn".to_string(), "groupparent".to_string(), "nocycle".to_string()];
let rows = vec![vec![
serde_json::Value::from(1),
serde_json::Value::from(2),
serde_json::Value::from(3),
serde_json::Value::from(4),
]];

let sql = generate_insert_typed(
&columns,
&vec![None; columns.len()],
&rows,
"shrink",
"verify",
&DatabaseType::Gaussdb,
None,
);

assert!(
sql.starts_with("INSERT INTO \"verify\".\"shrink\""),
"reserved schema/table must be quoted, sql: {sql}"
);
assert!(
sql.contains("(id, \"csn\", \"groupparent\", \"nocycle\")"),
"plain `id` must stay unquoted and the reserved words must be quoted, sql: {sql}"
);
}

#[test]
fn postgres_create_table_still_quotes_simple_lowercase_identifiers() {
// Unlike GaussDB, plain Postgres has no reported case-folding quirk, so
// the fix is deliberately scoped to Gaussdb/OpenGauss only — Postgres
// itself keeps quoting everything, unchanged.
let cols = vec![test_column("id", "int")];

let ddl = generate_create_table_ddl(
&cols,
"t",
"",
"public",
&DatabaseType::Postgres,
&DatabaseType::Mysql,
None,
None,
);

assert!(ddl.contains("\"id\""), "ddl: {ddl}");
}

#[test]
fn postgres_create_table_preserves_defaults_identity_and_exact_types() {
let cols = vec![
Expand Down
Loading