From e15cdf92e5c954bb613aa000dc82b53bde5c7a1e Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Sun, 28 Jun 2026 00:43:02 +0200 Subject: [PATCH 1/2] Fix premature COMMIT in stream-rows-to-copy under on error stop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a data error occurs inside stream-rows-to-copy, the outer handler-case handler calls ROLLBACK. In Common Lisp, handler-case handlers run *after* the unwind-protect cleanup forms have finished, so the original code sequenced: 1. COMMIT (inside unwind-protect cleanup) 2. ROLLBACK (inside handler-case handler — now a no-op) This meant any rows already streamed to PostgreSQL were committed even when the load failed, and the ROLLBACK was silently ignored with a 'there is no transaction in progress' warning. Under on error stop with concurrency > 1 the subsequent inconsistent connection state could also cause writer threads to hang (GitHub issue #1622, comment by fluca1978 on 2026-06-22). Fix: move the COMMIT after the unwind-protect form. It is now only reached when the row loop and close-db-writer both complete without signalling. When either signals, unwind-protect closes the COPY writer (cl-postgres with-syncing reads until ReadyForQuery, leaving the connection in SQL mode), and then the handler-case handler correctly finds a clean connection on which ROLLBACK works. Also guard the incf of bytes/seconds with (when row-bytes ...) so that nil returned by stream-row for a filtered row does not raise a TYPE-ERROR that was itself masking the COMMIT sequencing bug. Add regression test: test/sqlite-on-error-stop.load uses a SQLite database (test/sqlite/type-mismatch.db) that has a TEXT value in an INTEGER column. pgloader must exit cleanly without hanging. Fixes: https://github.com/dimitri/pgloader/issues/1622 --- src/pg-copy/copy-rows-in-stream.lisp | 26 +++++++--- test/Makefile | 71 ++++++++++++++++----------- test/sqlite-on-error-stop.load | 36 ++++++++++++++ test/sqlite/create-type-mismatch.py | 41 ++++++++++++++++ test/sqlite/type-mismatch.db | Bin 0 -> 8192 bytes 5 files changed, 137 insertions(+), 37 deletions(-) create mode 100644 test/sqlite-on-error-stop.load create mode 100644 test/sqlite/create-type-mismatch.py create mode 100644 test/sqlite/type-mismatch.db diff --git a/src/pg-copy/copy-rows-in-stream.lisp b/src/pg-copy/copy-rows-in-stream.lisp index 7b1b8130..523b8ba6 100644 --- a/src/pg-copy/copy-rows-in-stream.lisp +++ b/src/pg-copy/copy-rows-in-stream.lisp @@ -25,17 +25,26 @@ :table table :columns columns :condition c)))))) + ;; The unwind-protect cleanup closes the COPY writer (sending + ;; CopyDone to the server) but intentionally does NOT COMMIT. + ;; cl-postgres's with-syncing in close-db-writer reads until + ;; ReadyForQuery even on error, so the connection is always in + ;; a clean SQL state when the cleanup finishes. COMMIT only + ;; runs on the success path below; the error handler uses the + ;; now-clean connection to ROLLBACK. (unwind-protect (loop :for row := (lq:pop-queue queue) :until (eq :end-of-data row) :do (multiple-value-bind (row-bytes row-seconds) (stream-row copier copy nbcols row) - (incf rcount) - (incf bytes row-bytes) - (incf seconds row-seconds))) - (cl-postgres:close-db-writer copier) - (pomo:execute "COMMIT")))) + (when row-bytes + (incf rcount) + (incf bytes row-bytes) + (incf seconds row-seconds)))) + (cl-postgres:close-db-writer copier)) + ;; Only reached when the loop completes without signalling. + (pomo:execute "COMMIT"))) (postgresql-unavailable (condition) ;; We got disconnected, maybe because PostgreSQL is being restarted, @@ -52,8 +61,11 @@ (error condition)) (condition (c) - ;; stop at any failure here, this function doesn't implement any kind - ;; of retry behaviour. + ;; close-db-writer in the unwind-protect cleanup above has already + ;; sent CopyDone and read until ReadyForQuery (via cl-postgres's + ;; with-syncing), so the connection is now in SQL mode. ROLLBACK + ;; is safe regardless of whether the error came from data streaming + ;; or from close-db-writer itself. (log-message :error "~a" c) (update-stats :data table :errs 1) (pomo:execute "ROLLBACK") diff --git a/test/Makefile b/test/Makefile index 5f28d491..75732a6b 100644 --- a/test/Makefile +++ b/test/Makefile @@ -4,36 +4,37 @@ OUT = $(TESTS:.load=.out) REMOTE = archive.load bossa-all.load bossa.load census-places.load dbf-zip.load LOCAL = $(filter-out $(REMOTE:.load=.out),$(OUT)) -REGRESS= allcols.load \ - csv-before-after.load \ - csv-districts.load \ - csv-parse-date.load \ - csv-error.load \ - csv-escape-mode.load \ - csv-filename-pattern.load \ - csv-guess.load \ - csv-header.load \ - csv-json.load \ - csv-keep-extra-blanks.load \ - csv-missing-col.load \ - csv-non-printable.load \ - csv-nulls.load \ - csv-temp.load \ - csv-trim-extra-blanks.load \ - csv-using-sexp.load \ - csv.load \ - copy.load \ - copy-hex.load \ - dbf.load \ - errors.load \ - fk-reject.load \ - fixed.load \ - fields-with-periods.load \ - ixf.load \ - overflow.load \ - partial.load \ - serial.load \ - udc.load \ +REGRESS= allcols.load \ + csv-before-after.load \ + csv-districts.load \ + csv-parse-date.load \ + csv-error.load \ + csv-escape-mode.load \ + csv-filename-pattern.load \ + csv-guess.load \ + csv-header.load \ + csv-json.load \ + csv-keep-extra-blanks.load \ + csv-missing-col.load \ + csv-non-printable.load \ + csv-nulls.load \ + csv-temp.load \ + csv-trim-extra-blanks.load \ + csv-using-sexp.load \ + csv.load \ + copy.load \ + copy-hex.load \ + dbf.load \ + errors.load \ + fk-reject.load \ + fixed.load \ + fields-with-periods.load \ + ixf.load \ + overflow.load \ + partial.load \ + serial.load \ + sqlite-on-error-stop.load \ + udc.load \ xzero.load # Those are not included in the tests because CCL doesn't have the cp866 @@ -81,6 +82,16 @@ errors.out: errors.load -$(PGLOADER) $< @echo +# 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/type-mismatch.db: + python3 sqlite/create-type-mismatch.py + +regress/out/sqlite-on-error-stop.out: sqlite-on-error-stop.load sqlite/type-mismatch.db + -$(PGLOADER) $< + touch $@ + nofile.out: nofile.load -$(PGLOADER) $< @echo diff --git a/test/sqlite-on-error-stop.load b/test/sqlite-on-error-stop.load new file mode 100644 index 00000000..7b757a63 --- /dev/null +++ b/test/sqlite-on-error-stop.load @@ -0,0 +1,36 @@ +/* + * Regression test for GitHub issue #1622 (comment 2026-06-22). + * + * When a SQLite INTEGER column contains a TEXT value that PostgreSQL rejects + * during COPY, pgloader used to hang indefinitely under "on error stop" mode + * because the COPY writer was closed (sending CopyDone) inside the + * unwind-protect cleanup, and COMMIT ran there too — so when the outer + * handler-case handler tried to ROLLBACK the transaction had already been + * committed and the connection state was inconsistent. + * + * The database has three rows in the "products" table: + * id=1 name='apple' qty=10 (valid INTEGER) + * id=2 name='banana' qty='lots-of-it' (TEXT in INTEGER column) + * id=3 name='cherry' qty=5 (valid INTEGER) + * + * Expected outcome: + * - pgloader exits cleanly (non-zero exit code is fine). + * - pgloader does NOT hang. + * - An error is logged for the INTEGER type mismatch. + * + * To regenerate the database: + * python3 test/sqlite/create-type-mismatch.py + */ + +load database + from 'sqlite/type-mismatch.db' + into postgresql:///pgloader + + with include drop, create tables, create indexes, reset sequences, + on error stop + + set work_mem to '16MB', + search_path to 'typemismatch' + + before load do + $$ create schema if not exists typemismatch; $$; diff --git a/test/sqlite/create-type-mismatch.py b/test/sqlite/create-type-mismatch.py new file mode 100644 index 00000000..e6465aea --- /dev/null +++ b/test/sqlite/create-type-mismatch.py @@ -0,0 +1,41 @@ +#!/usr/bin/env python3 +""" +Regenerate test/sqlite/type-mismatch.db for the on-error-stop hang test +(GitHub issue #1622). + +The database has three rows in the "products" table: + id=1 name='apple' qty=10 (valid INTEGER) + id=2 name='banana' qty='lots-of-it' (TEXT stored in INTEGER column) + id=3 name='cherry' qty=5 (valid INTEGER) + +SQLite stores 'lots-of-it' as TEXT because it has dynamic typing. When +pgloader tries to COPY this into a PostgreSQL INTEGER column the server +rejects the data at CopyDone time. + +Usage: + python3 test/sqlite/create-type-mismatch.py +""" +import os +import sqlite3 + +out = os.path.join(os.path.dirname(__file__), "type-mismatch.db") +if os.path.exists(out): + os.remove(out) + +conn = sqlite3.connect(out) +conn.execute( + """CREATE TABLE products ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + qty INTEGER NOT NULL +)""" +) + +conn.execute("INSERT INTO products VALUES (1, 'apple', 10)") +# Row 2: TEXT value in an INTEGER column. SQLite accepts this because of +# dynamic typing; PostgreSQL will reject it when COPY tries to parse it. +conn.execute("INSERT INTO products VALUES (2, 'banana', 'lots-of-it')") +conn.execute("INSERT INTO products VALUES (3, 'cherry', 5)") +conn.commit() +conn.close() +print(f"Created {out}") diff --git a/test/sqlite/type-mismatch.db b/test/sqlite/type-mismatch.db new file mode 100644 index 0000000000000000000000000000000000000000..229489ef77588a94a19936ef026431a79cecdb65 GIT binary patch literal 8192 zcmeI#F;2rU6b9huGzc{ag#oEzsPx!CqAXm1fGkvz1_(x?quaV7Qrd*n3>|w24#mg? zI0Xm5DUBGJ5&xgWFZQ#SZ}a_KoLR%uqMB*TXS7E`&^a>^d7ICbb%Y!J&3|3$`ET*) z^!07m?Gm9S_aYF000bZa0SG_<0uX=z1Rwx`e+W373@iDl>aEF*;^ZNYzgy3C z`E#&o1;_Q>ycNDv_!)k Date: Sun, 28 Jun 2026 00:53:46 +0200 Subject: [PATCH 2/2] Wire WITH on error stop load-file option to copy/*on-error-stop* in v4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The WITH on error stop / on error resume next options were already parsed by the grammar and surfaced as :on-error-stop in with-options by ast.clj, but run-command in core.clj never read them. Only the --on-error-stop CLI flag set copy/*on-error-stop*. Fix: extract :on-error-stop from with-options and add it to the binding block alongside batch-rows, batch-size, etc. Use (some? v) instead of (or v ...) since the option is boolean: a false value from a future explicit on-error-resume path must not be ignored. Add parser tests verifying that: - WITH on error stop → :on-error-stop true in with-options - WITH on error resume next → :on-error-stop absent from with-options --- clojure/src/pgloader/core.clj | 4 +++- clojure/test/pgloader/load_file/parser_test.clj | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/clojure/src/pgloader/core.clj b/clojure/src/pgloader/core.clj index 4be60b5b..60ea6e24 100644 --- a/clojure/src/pgloader/core.clj +++ b/clojure/src/pgloader/core.clj @@ -454,11 +454,13 @@ opts-batch-size (some-> (get (:with-options cmd) :batch-size) (Long/parseLong)) opts-prefetch-rows (some-> (get (:with-options cmd) :prefetch-rows) (long)) opts-batch-concurr (some-> (get (:with-options cmd) :batch-concurrency) (long)) + opts-on-error-stop (get (:with-options cmd) :on-error-stop) _ (when (and opts-prefetch-rows opts-batch-concurr) (throw (ex-info "pgloader: 'batch concurrency' (deprecated) and 'prefetch rows' cannot both appear in the same WITH clause; remove 'batch concurrency'" {})))] (binding [copy/*batch-rows* (or opts-batch-rows copy/*batch-rows*) copy/*batch-size* (or opts-batch-size copy/*batch-size*) - copy/*prefetch-queue-capacity* (or opts-prefetch-rows opts-batch-concurr copy/*prefetch-queue-capacity*)] + copy/*prefetch-queue-capacity* (or opts-prefetch-rows opts-batch-concurr copy/*prefetch-queue-capacity*) + copy/*on-error-stop* (if (some? opts-on-error-stop) opts-on-error-stop copy/*on-error-stop*)] (try ;; Send MySQL-specific SET params to the MySQL source connection before catalog fetch (when (#{:mysql :mariadb} (:type source-uri)) diff --git a/clojure/test/pgloader/load_file/parser_test.clj b/clojure/test/pgloader/load_file/parser_test.clj index b9c16862..27047497 100644 --- a/clojure/test/pgloader/load_file/parser_test.clj +++ b/clojure/test/pgloader/load_file/parser_test.clj @@ -552,3 +552,20 @@ WITH downcase identifiers;")] (is (:ok result) (str "Parse failed: " (:error result))) (is (true? (get-in result [:ok :with-options :downcase-ids])))))) + +(deftest test-with-on-error-stop + (testing "WITH on error stop sets :on-error-stop true in with-options" + (let [result (parser/parse-string + "LOAD DATABASE FROM sqlite:///tmp/foo.db + INTO postgresql:///target + WITH on error stop;")] + (is (:ok result) (str "Parse failed: " (:error result))) + (is (true? (get-in result [:ok :with-options :on-error-stop]))))) + + (testing "WITH on error resume next does not set :on-error-stop" + (let [result (parser/parse-string + "LOAD DATABASE FROM sqlite:///tmp/foo.db + INTO postgresql:///target + WITH on error resume next;")] + (is (:ok result) (str "Parse failed: " (:error result))) + (is (nil? (get-in result [:ok :with-options :on-error-stop]))))))