fix(transfer): stop quoting simple identifiers when migrating to GaussDB/OpenGauss - #6283
fix(transfer): stop quoting simple identifiers when migrating to GaussDB/OpenGauss#6283q396921921 wants to merge 6 commits into
Conversation
t8y2
left a comment
There was a problem hiding this comment.
request changes: the quoting decision currently reuses the PostgreSQL reserved-word set for GaussDB/openGauss. These targets have their own keyword catalog; for example, the official GaussDB reserved-keyword table includes COMPACT. A simple lowercase identifier such as compact would therefore be emitted without quotes and can produce invalid target DDL.
Please base this decision on the target dialect keyword catalog and its unquoted-case rules, then add focused coverage for target-only reserved words and mixed-case identifiers. This database-semantic change also needs validation against a real GaussDB/openGauss instance before merge.
Official reference: https://support.huaweicloud.com/intl/en-us/distributed-devg-v8-gaussdb/gaussdb-12-0792.html
…sDB/OpenGauss Data transfer always wrapped every target column/table name in double quotes for GaussDB/OpenGauss, locking in the source's exact case. GaussDB can fold *unquoted* identifiers to a different case than Postgres does (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 there. Only quote identifiers that actually need it (mixed case, special characters, or a reserved word), reusing the same heuristic already used for GaussDB JDBC identifier quoting. Scoped to GaussDB/OpenGauss only — other Postgres-family targets (Postgres, Redshift, Kingbase, ...) have no reported case-folding issue and always-quoting them is harmless. Fixes t8y2#6205
The quoting decision for GaussDB/OpenGauss reused is_postgres_reserved_identifier, but GaussDB reserves words PostgreSQL does not (e.g. COMPACT, per Huawei's GaussDB(DWS) keyword reference). A plain lowercase column like `compact` passed the "needs quoting" check and was emitted unquoted, producing invalid DDL on the real target. Add is_gaussdb_only_reserved_identifier with the GaussDB-only reserved words and consult it alongside the Postgres list for Gaussdb/OpenGauss targets in both quote_transfer_identifier and quote_gaussdb_jdbc_identifier (the latter also serves plain Postgres connections, so it now takes database_type to keep the extra reserved words scoped to GaussDB/OpenGauss only). Addresses review feedback on t8y2#6283.
faa82e0 to
68b185a
Compare
|
Pushed a fix for this. Summary of what changed and what's still open: Keyword catalog gap — fixed. Checked the raw HTML of Huawei's official GaussDB(DWS) keyword reference (https://support.huaweicloud.com/intl/en-us/sqlreference-dws/dws_06_0007.html) directly rather than relying on a rendered/summarized view, and confirmed Added
Added regression tests for target-only reserved words ( Live-instance validation — not done, flagging honestly. I don't have access to a real GaussDB/openGauss instance. I tried spinning one up locally via One more caveat on the keyword source itself: the table I diffed against is Huawei's GaussDB(DWS) (the MPP/columnar variant) keyword reference — the core GaussDB/openGauss keyword doc pages are client-side rendered and I couldn't get a raw table out of them to cross-check directly. A few of the 24 added words ( If you have access to a GaussDB or openGauss instance (or can point me at one), I'd appreciate either running the added test cases' DDL directly against it, or sharing connection details so I can validate myself before this merges. |
t8y2
left a comment
There was a problem hiding this comment.
Request changes: the updated keyword fix still uses the GaussDB(DWS) keyword catalog as a hard-coded superset for core GaussDB/openGauss.
The PR's purpose is to stop quoting simple lowercase identifiers because quoting preserves case and can make later unquoted references fail. In that context, over-quoting is not harmless: if a DWS-only word is non-reserved on the actual core target, the new list recreates the original behavior for that identifier.
openGauss publishes target-specific reserved/non-reserved classifications and recommends consulting the actual keyword catalog (pg_get_keywords) rather than assuming another variant's list:
- https://docs.opengauss.org/en/docs/latest-lite/sql_reference/keywords.html
- https://docs.opengauss.org/en/docs/latest/developer_guide/database_object_naming_conventions.html
Please base the decision on the real target/version catalog or a verified exact core list, and validate the generated DDL on a writable GaussDB/openGauss instance in the relevant compatibility mode. Current unit tests pass, but all live GaussDB tests remain ignored.
|
I have a writable openGauss 5.0.0 instance available, so I ran the verification myself rather than asking you to find one. Method: queried the instance's own Result: of the 24 words, 9 are not actually reserved on this real instance:
The remaining 15 ( Why this matters for the PR: this confirms the concern from the previous review — the DWS keyword catalog is not a safe superset for core GaussDB/openGauss. Right now Suggest trimming the set down to the 15 confirmed-reserved words, or better, deriving it from Happy to share the exact test queries I used if useful. |
The GaussDB-only reserved-word list in is_gaussdb_only_reserved_identifier was diffed from Huawei's GaussDB(DWS) keyword reference, but DWS is the MPP/columnar variant and isn't a reliable proxy for core GaussDB/openGauss. Cross-checked all 24 words against a writable openGauss 5.0.0 instance's own pg_get_keywords(): 9 of them are not actually reserved on the core engine (hot/nlssort/warmup aren't keywords at all; fenced/internal/plan/ tsfield/tstag/tstime are unreserved). Quoting them would have reintroduced the case-locking bug this PR fixes. Dropped those 9 and kept the remaining 15 confirmed reserved/reserved(can be function or type name) words; compact was additionally confirmed by running an unquoted CREATE TABLE against that instance, which fails with a syntax error. Extended the regression test to cover all 15 confirmed words plus a negative check for the 9 dropped ones. Addresses review feedback on t8y2#6283.
|
Pushed a fix acting on the instance-verification results from my last comment.
Updated the doc comment on
Live-instance DDL validation for this specific diff (as opposed to the standalone keyword checks I ran) is still outstanding — happy to run it if you can point me at an instance, otherwise let me know if the trimmed list itself needs anything else before merge. |
t8y2
left a comment
There was a problem hiding this comment.
The new head still uses an incomplete static keyword subset, so valid migrations can generate unquoted reserved identifiers.
Compared with the openGauss 5.0 keyword catalog, the current PostgreSQL plus GaussDB list still misses at least csn, excluded, groupparent, nocycle, rownum, shrink, and verify. These names pass the simple-lowercase check and are emitted without quotes. A source column such as rownum can therefore make generated CREATE TABLE or INSERT fail on the target.
openGauss recommends checking the target keyword catalog through pg_get_keywords() and documents the target-specific classification:
https://docs.opengauss.org/en/docs/latest-lite/sql_reference/keywords.html
Please either query and cache the real target/version catalog once per connection or maintain separate, source-backed complete lists for GaussDB and OpenGauss. Add table, schema, column, CREATE TABLE, and INSERT tests for the missing words and validate the generated SQL on writable targets in the relevant compatibility modes.
…and-picked list The GaussDB-only reserved-word list in is_gaussdb_only_reserved_identifier was built by spot-checking words against a writable openGauss 5.0.0 instance one at a time, which missed csn, excluded, groupparent, nocycle, rownum, shrink, and verify — all reserved on that same instance but not in plain PostgreSQL, so they were emitted unquoted and could break generated DDL/DML. Diffed against the instance's full pg_get_keywords() result set (653 rows) instead of individual words this time; the gap was exactly these 7, with no further omissions. Extended the reserved-word test to cover them, and added table/schema-name coverage to the CREATE TABLE regression test plus a new INSERT regression test (previously only CREATE TABLE was covered). Addresses review feedback on t8y2#6283.
|
Pushed a fix acting on the review feedback about the hand-picked list. Instead of spot-checking individual words again, diffed Extended
Live-instance DDL/INSERT validation for this specific diff is still outstanding, same caveat as before. |
t8y2
left a comment
There was a problem hiding this comment.
Request changes: the new catalog is complete for the tested openGauss 5.0.0 instance, but it is still applied statically to every GaussDB/openGauss version.
maxvalue is the concrete version-dependent counterexample. The official openGauss 5.0 source classifies it as RESERVED_KEYWORD, while current openGauss source classifies it as UNRESERVED_KEYWORD:
- openGauss 5.0
kwlist.h: https://gitee.com/opengauss/openGauss-server/blob/5.0.0/src/include/parser/kwlist.h - current openGauss
kwlist.h: https://gitee.com/opengauss/openGauss-server/blob/master/src/include/parser/kwlist.h
This PR permanently quotes maxvalue for all targets. On newer versions that reintroduces the original case-locking failure mode: a simple lowercase, non-reserved identifier is unnecessarily quoted and later unquoted references are folded differently.
Please make the keyword decision target/version aware—preferably by querying and caching pg_get_keywords() once per connection, or by selecting a source-backed catalog for the actual server version. Add regression coverage showing the differing maxvalue behavior across the supported versions and validate both paths on real targets.
…ware The GaussDB-only reserved-word list used to decide when transfer output needs quoting was a static list hand-diffed against one openGauss 5.0.0 instance. As the maintainer pointed out in review, that list is version-dependent — `maxvalue` is reserved on openGauss 5.0 but not on current/master openGauss, so permanently quoting it reintroduces the original case-locking bug (t8y2#6205) on newer targets. Replace the static list with a live `pg_get_keywords()` query, run once per target connection and cached on `AppState` by pool_key (`db::postgres::gaussdb_reserved_keywords`, `AppState::gaussdb_reserved_keywords`). The live catalog is authoritative when available and replaces (never unions with) the static list, so a word only reserved on older versions is correctly left unquoted on newer ones; falls back to the static list when no live catalog is available. Only a deterministic outcome (query ran and failed/came back empty, e.g. insufficient privileges) is cached — a merely transient failure (checkout error, timeout) is retried on the next table rather than pinned to "unavailable" for the pool's lifetime. Threaded through the CREATE TABLE DDL and INSERT/UPSERT write paths in both the SQL-source (`transfer_table`) and MongoDB-source (`transfer_mongodb_table`) transfer flows, including the Overwrite-mode TRUNCATE/DELETE step (extracted into a shared `transfer_overwrite_clear_sql` helper) — that statement was initially missed and quoted using the stale static list while CREATE TABLE already used the live catalog, which would have made the two disagree on the same table name. Added regression tests modeling both a 5.0-shaped and a current-shaped live keyword catalog for `maxvalue` across the DDL, INSERT, and TRUNCATE paths. Also validated end-to-end against two real openGauss instances (a 5.0.0 instance and a 7.0.0-RC1 instance) — pg_get_keywords() output, generated DDL, and actual CREATE TABLE + SELECT execution all matched expectations on both versions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the detailed catch on What changed
Regression coverage Added tests that inject a synthetic 5.0-shaped keyword set ( Real-target validation Ran this end-to-end against two real openGauss instances:
Ready for another look whenever you have a chance. |
t8y2
left a comment
There was a problem hiding this comment.
Request changes: the live pg_get_keywords() catalog now fixes the concrete maxvalue version difference when probing succeeds, but two fallback issues remain.
-
crates/dbx-core/src/db/postgres.rs:101maps every catalog query error toNotSupported, andcrates/dbx-core/src/connection.rs:3902caches that result for the pool lifetime. A transient disconnect or I/O error can therefore permanently select the static fallback and reproduce incorrect quoting on newer openGauss versions. Please classify transient connection/I/O/cancellation failures as retryable and add a first-failure/second-success regression test. -
crates/dbx-core/src/sql_dialect/identifiers.rs:451still combines a successful live catalog with the static PostgreSQL reserved-word table. Please make the complete liveR/Tcatalog authoritative when it is available, or provide source-backed tests proving the static subset is valid for every supported GaussDB/openGauss target.
The branch is also currently conflicting with main; resolve it after the behavior is corrected.
Problem
Data transfer (migration) always wrapped every target column/table name in
double quotes when the target was GaussDB or OpenGauss, locking in the
source's exact case. GaussDB can fold unquoted identifiers to a different
case than PostgreSQL does (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.
This matches the report in #6205: migrating a MySQL table with plain
lowercase field/table names to GaussDB produced a table whose fields
required quoting to query, which the user hadn't asked for and didn't
want.
Solution
quote_transfer_identifier(used by the data-transfer DDL/INSERTgenerator) now only quotes an identifier when it actually needs it — mixed
case, special characters, or a reserved word — instead of quoting
unconditionally. This reuses the same heuristic (
is_simple_lower_identifieris_postgres_reserved_identifier) already used elsewhere in this codebasefor GaussDB JDBC identifier quoting (
quote_gaussdb_jdbc_identifier).The change is deliberately scoped to GaussDB/OpenGauss only, not the wider
Postgres family (Postgres, Redshift, Kingbase, Highgo, Uxdb, Kwdb,
Vastbase). Those other targets have no reported case-folding issue, and
always-quoting them is harmless — narrowing the scope avoided touching ~30
existing tests that assert quoted output for plain Postgres transfer SQL,
which would have been a much larger, unrelated behavior change.
Testing
Added 3 regression tests in
crates/dbx-core/src/transfer.rs, using theexact columns from the issue's example table:
gaussdb_create_table_does_not_quote_simple_lowercase_identifiers— failsagainst the old code (reproduces the bug), passes after the fix.
gaussdb_create_table_still_quotes_identifiers_that_need_it— mixed-caseand reserved-word identifiers still get quoted.
postgres_create_table_still_quotes_simple_lowercase_identifiers—confirms plain Postgres behavior is unchanged.
Full
transfermodule test suite: 198/198 passing after rebasing ontolatest
main.Fixes #6205