diff --git a/clojure/src/pgloader/core.clj b/clojure/src/pgloader/core.clj index 60ea6e24..45518433 100644 --- a/clojure/src/pgloader/core.clj +++ b/clojure/src/pgloader/core.clj @@ -589,6 +589,28 @@ (map :name) (type-names-in-schema pg-conn {:schema schema :names names})))))] + ;; After apply-identifier-case, check whether any table has two columns + ;; whose names become identical after PostgreSQL's 63-char truncation. + ;; Accumulate every instance and report them all before aborting. + (let [collisions (ddl/check-identifier-collisions cat)] + (when (seq collisions) + (doseq [c collisions] + (log/error (str (:schema c) "." (:table c) + ": column name collision — " + (str/join ", " (map pr-str (:columns c))) + " all truncate to " (pr-str (:effective-name c))))) + (throw (ex-info + (str (count collisions) + " column name collision(s) found in source catalog.\n\n" + "PostgreSQL limits identifier names to " + ddl/pg-max-identifier-length + " bytes (NAMEDATALEN-1). The tables listed above contain " + "multiple columns whose names become identical after " + "truncation. This would cause CREATE TABLE to fail or COPY " + "to load data into the wrong column.\n\n" + "Please rename the affected columns in the source database " + "before migrating.") + {:collisions collisions})))) ;; Truncate tables if requested — each table its own transaction (when (get with-options :truncate) (log/debug "Truncating tables") diff --git a/clojure/src/pgloader/ddl/common.clj b/clojure/src/pgloader/ddl/common.clj index 5ba75333..ce3a6623 100644 --- a/clojure/src/pgloader/ddl/common.clj +++ b/clojure/src/pgloader/ddl/common.clj @@ -333,7 +333,9 @@ on-upd on-del ";\n"))) fkeys)) -(def ^:private pg-max-identifier-length 63) +(def pg-max-identifier-length + "PostgreSQL's NAMEDATALEN-1: the maximum number of bytes in an identifier." + 63) (defn- snake-case-transform "Convert an identifier to snake_case: @@ -403,6 +405,25 @@ fks)))))) catalog)))) +(defn check-identifier-collisions + "Return a seq of collision maps for every (table, truncated-name) pair in + CATALOG where two or more columns map to the same PostgreSQL identifier + after 63-character truncation. Each map has keys :schema :table + :effective-name :columns. Returns an empty seq when no collisions exist." + [catalog] + (for [t catalog + :let [names (mapv :column-name (:columns t)) + eff-names (mapv #(subs % 0 (min pg-max-identifier-length (count %))) names) + freqs (frequencies eff-names) + coll-effs (into #{} (keep (fn [[k v]] (when (> v 1) k)) freqs))] + :when (seq coll-effs) + eff coll-effs] + {:schema (:schema t) + :table (:table-name t) + :effective-name eff + :columns (filterv #(= eff (subs % 0 (min pg-max-identifier-length (count %)))) + names)})) + (defn apply-alter-schema "Apply ALTER SCHEMA ... RENAME TO ... rules from the load command. Each rule: {:source-name s :target-name t} diff --git a/clojure/test/pgloader/ddl_test.clj b/clojure/test/pgloader/ddl_test.clj index 875aa0e5..d1356447 100644 --- a/clojure/test/pgloader/ddl_test.clj +++ b/clojure/test/pgloader/ddl_test.clj @@ -232,6 +232,63 @@ {:snake-case-ids true})] (is (= "xml_parser" (:table-name t)))))) +;; ── Identifier collision detection (#353) ──────────────────────────────────── + +(def ^:private col-prefix-63 + "col_very_long_name_that_exceeds_postgresql_identifier_limit_aaa") + +(deftest test-check-identifier-collisions + (testing "no collisions: empty catalog" + (is (empty? (ddl/check-identifier-collisions [])))) + + (testing "no collisions: short column names" + (let [cat [(make-table "t" ["id" "name" "value"] [])]] + (is (empty? (ddl/check-identifier-collisions cat))))) + + (testing "no collisions: names exactly 63 chars (no truncation)" + (let [cat [(make-table "t" [col-prefix-63 "other_col"] [])]] + (is (empty? (ddl/check-identifier-collisions cat))))) + + (testing "no collisions: 64-char names that differ within first 63 chars" + (let [col-a (str "aaaa_" col-prefix-63) ;; first 63 chars differ from col-b + col-b (str "bbbb_" col-prefix-63) + cat [(make-table "t" [col-a col-b] [])]] + (is (empty? (ddl/check-identifier-collisions cat))))) + + (testing "collision: two columns sharing first 63 chars" + (let [col-a (str col-prefix-63 "x") + col-b (str col-prefix-63 "y") + cat [(make-table "t" [col-a col-b "short_col"] [])]] + (let [cs (ddl/check-identifier-collisions cat)] + (is (= 1 (count cs))) + (is (= col-prefix-63 (:effective-name (first cs)))) + (is (= #{col-a col-b} (set (:columns (first cs))))) + (is (= "t" (:table (first cs))))))) + + (testing "collision: three columns sharing first 63 chars" + (let [col-a (str col-prefix-63 "x") + col-b (str col-prefix-63 "y") + col-c (str col-prefix-63 "z") + cat [(make-table "t" [col-a col-b col-c] [])]] + (let [cs (ddl/check-identifier-collisions cat)] + (is (= 1 (count cs))) + (is (= #{col-a col-b col-c} (set (:columns (first cs)))))))) + + (testing "collision: distinct collisions in two different tables — both reported" + ;; All four names share the same 63-char prefix so both tables get a + ;; collision entry — total must be 2, one per table. + (let [a1 (str col-prefix-63 "x") a2 (str col-prefix-63 "y") + b1 (str col-prefix-63 "p") b2 (str col-prefix-63 "q") + cat [(make-table "table_a" [a1 a2] []) + (make-table "table_b" [b1 b2] [])]] + (is (= 2 (count (ddl/check-identifier-collisions cat)))))) + + (testing "no cross-table collision: same long name in two different tables is fine" + (let [col-a (str col-prefix-63 "x") + cat [(make-table "t1" [col-a "short"] []) + (make-table "t2" [col-a "other"] [])]] + (is (empty? (ddl/check-identifier-collisions cat)))))) + ;; ── FK filter (#1216) ───────────────────────────────────────────────────────── (deftest test-create-fkeys-sql-filters-missing-tables diff --git a/src/load/migrate-database.lisp b/src/load/migrate-database.lisp index 5613491a..ed82009f 100644 --- a/src/load/migrate-database.lisp +++ b/src/load/migrate-database.lisp @@ -258,6 +258,31 @@ ;; cast the catalog into something PostgreSQL can work on (cast catalog) + ;; After casting, apply-identifier-case has been applied to every column + ;; name. Check now whether any table has two columns whose names become + ;; identical after PostgreSQL's 63-character identifier truncation. All + ;; collisions are accumulated and reported together so users can fix + ;; everything in one pass before re-running the migration. + (let ((collisions (check-catalog-identifier-collisions catalog))) + (when collisions + (dolist (c collisions) + (log-message :error + "~a.~a: column name collision — ~{~s~^, ~} all truncate to ~s" + (getf c :schema) + (getf c :table) + (getf c :columns) + (getf c :effective-name))) + (error "~d column name collision~:p found in source catalog. + +PostgreSQL limits identifier names to ~d bytes (NAMEDATALEN-1). The tables +listed above contain multiple columns whose names become identical after +truncation. This would cause CREATE TABLE to fail or COPY to load data into +the wrong column. + +Please rename the affected columns in the source database before migrating." + (length collisions) + +pg-max-identifier-length+))) + ;; support code for index filters (where clauses) (process-index-definitions catalog :sql-dialect (class-name (class-of copy))) diff --git a/src/package.lisp b/src/package.lisp index af390ef2..dfc4e3ff 100644 --- a/src/package.lisp +++ b/src/package.lisp @@ -233,7 +233,11 @@ #:citus-format-sql-select #:citus-backfill-table-p - #:format-table-name)) + #:format-table-name + + #:+pg-max-identifier-length+ + #:pg-effective-name + #:check-catalog-identifier-collisions)) (defpackage #:pgloader.state (:use #:cl #:pgloader.params #:pgloader.catalog) diff --git a/src/utils/catalog.lisp b/src/utils/catalog.lisp index ab9044ec..2598de09 100644 --- a/src/utils/catalog.lisp +++ b/src/utils/catalog.lisp @@ -390,6 +390,45 @@ (defmethod field-name ((column column) &key) (column-name column)) +;;; +;;; PostgreSQL limits identifier names to NAMEDATALEN-1 = 63 bytes. Two +;;; source columns in the same table whose names map to the same truncated +;;; identifier cause CREATE TABLE to fail with a duplicate-column error, or +;;; COPY to silently load data into the wrong column. Detect these +;;; collisions after the catalog has been cast (apply-identifier-case +;;; already applied) so the names here are exactly what pgloader will send +;;; to PostgreSQL. +;;; +(defconstant +pg-max-identifier-length+ 63) + +(defun pg-effective-name (identifier) + "Return the name PostgreSQL stores in pg_attribute for IDENTIFIER. + Strips outer double-quotes (from :quote identifier-case mode) and + truncates to +pg-max-identifier-length+ characters." + (let ((inner (ensure-unquoted identifier))) + (subseq inner 0 (min +pg-max-identifier-length+ (length inner))))) + +(defun check-catalog-identifier-collisions (catalog) + "Return a plist for every (table, truncated-name) pair in CATALOG where + two or more columns map to the same PostgreSQL identifier after 63-char + truncation. Each entry has keys :schema :table :effective-name :columns. + Returns NIL when no collisions are found." + (loop :for schema :in (catalog-schema-list catalog) + :append + (loop :for table :in (schema-table-list schema) + :append + (let ((name-map (make-hash-table :test #'equal))) + (dolist (col (table-column-list table)) + (push (column-name col) + (gethash (pg-effective-name (column-name col)) name-map))) + (loop :for eff :being :the :hash-keys :of name-map + :using (hash-value cols) + :when (> (length cols) 1) + :collect (list :schema (schema-name schema) + :table (table-name table) + :effective-name eff + :columns (nreverse cols))))))) + ;;; ;;; There's no simple equivalent to array_agg() in MS SQL, so the index and ;;; fkey queries return a row per index|fkey column rather than per diff --git a/test/Makefile b/test/Makefile index 75732a6b..d81d8fce 100644 --- a/test/Makefile +++ b/test/Makefile @@ -33,6 +33,7 @@ REGRESS= allcols.load \ overflow.load \ partial.load \ serial.load \ + sqlite-collision.load \ sqlite-on-error-stop.load \ udc.load \ xzero.load @@ -85,6 +86,15 @@ errors.out: errors.load # sqlite LOAD DATABASE is incompatible with --regress (parse-target-pg-db-uri # returns nil for LOAD DATABASE commands). Run without --regress; any hang # would be caught by the CI job timeout. +sqlite/collision.db: + python3 sqlite/create-collision.py + +# pgloader must detect the 63-char identifier collision and exit non-zero. +# LOAD DATABASE is incompatible with --regress; run without it. +regress/out/sqlite-collision.out: sqlite-collision.load sqlite/collision.db + -$(PGLOADER) $< + touch $@ + sqlite/type-mismatch.db: python3 sqlite/create-type-mismatch.py diff --git a/test/mysql-collision.load b/test/mysql-collision.load new file mode 100644 index 00000000..3b0117a7 --- /dev/null +++ b/test/mysql-collision.load @@ -0,0 +1,31 @@ +/* + * Manual test for GitHub issue #353 (column name identifier collision). + * + * Requires a running MySQL server with the mysql_collision database set up: + * + * CREATE DATABASE mysql_collision; + * USE mysql_collision; + * CREATE TABLE products ( + * id INT PRIMARY KEY, + * name VARCHAR(100) NOT NULL, + * col_very_long_name_that_exceeds_postgresql_identifier_limit_aaax INT, + * col_very_long_name_that_exceeds_postgresql_identifier_limit_aaay INT + * ); + * + * Expected outcome: pgloader exits non-zero, logs the collision, touches no + * PostgreSQL tables. + * + * Run manually (not in REGRESS — requires a MySQL server): + * pgloader test/mysql-collision.load + */ + +load database + from mysql://root@localhost/mysql_collision + into postgresql:///pgloader + + with include drop, create tables, create indexes, reset sequences + + set search_path to 'mysql_collision_test' + + before load do + $$ create schema if not exists mysql_collision_test; $$; diff --git a/test/sqlite-collision.load b/test/sqlite-collision.load new file mode 100644 index 00000000..b7d03d2f --- /dev/null +++ b/test/sqlite-collision.load @@ -0,0 +1,31 @@ +/* + * Regression test for GitHub issue #353 (column name identifier collision). + * + * The source SQLite database has a table "products" with two columns whose + * names share the same first 63 characters: + * + * col_very_long_name_that_exceeds_postgresql_identifier_limit_aaax + * col_very_long_name_that_exceeds_postgresql_identifier_limit_aaay + * + * Both truncate to the same PostgreSQL identifier (NAMEDATALEN-1 = 63). + * + * Expected outcome: + * - pgloader exits with a non-zero exit code. + * - pgloader does NOT hang. + * - pgloader logs an error naming the colliding columns. + * - pgloader does NOT attempt to create any table or copy any data. + * + * To regenerate the database: + * python3 test/sqlite/create-collision.py + */ + +load database + from 'sqlite/collision.db' + into postgresql:///pgloader + + with include drop, create tables, create indexes, reset sequences + + set search_path to 'collision_test' + + before load do + $$ create schema if not exists collision_test; $$; diff --git a/test/sqlite/collision.db b/test/sqlite/collision.db new file mode 100644 index 00000000..d37339d0 Binary files /dev/null and b/test/sqlite/collision.db differ diff --git a/test/sqlite/create-collision.py b/test/sqlite/create-collision.py new file mode 100644 index 00000000..2e9e34d0 --- /dev/null +++ b/test/sqlite/create-collision.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python3 +""" +Regenerate test/sqlite/collision.db for the identifier collision test +(GitHub issue #353). + +Two columns in the same table share the same first 63 characters so they +would both truncate to the same PostgreSQL identifier name. pgloader must +detect this early and exit with a clear error rather than letting PostgreSQL +silently lose data. + +Usage: + python3 test/sqlite/create-collision.py +""" +import os +import sqlite3 + +PREFIX = "col_very_long_name_that_exceeds_postgresql_identifier_limit_aaa" +assert len(PREFIX) == 63, f"prefix should be 63 chars, got {len(PREFIX)}" +COL_A = PREFIX + "x" # 64 chars, truncates to PREFIX +COL_B = PREFIX + "y" # 64 chars, truncates to PREFIX — collision! +assert len(COL_A) == 64 +assert COL_A[:63] == COL_B[:63] + +out = os.path.join(os.path.dirname(__file__), "collision.db") +if os.path.exists(out): + os.remove(out) + +conn = sqlite3.connect(out) +conn.execute( + f"""CREATE TABLE products ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + {COL_A} INTEGER, + {COL_B} INTEGER +)""" +) +conn.execute("INSERT INTO products VALUES (1, 'apple', 10, 20)") +conn.commit() +conn.close() +print(f"Created {out}") +print(f" colliding columns: {COL_A!r}") +print(f" {COL_B!r}") +print(f" both truncate to: {PREFIX!r}")