Only let an explicit ON_ERROR_STOP excuse a later turn-off in test/install

CLAUDE.md

@@ -203,7 +203,7 @@ When tests fail, examine the diff output carefully. The actual test output in `t
 **Exceptions to the above** -- `test-build` and `test/install` (both optional, see `README.asc`) don't follow the `test/results` vs `test/expected` model:
 
 - **test-build** runs first, in its own separate `pg_regress` pass over `test/build/*.sql`, and gates the main suite: if it fails, `test/install`/`test/sql` never run at all. It does compare actual vs expected normally (`test/build/results/` vs `test/build/expected/`) -- use `make results-build` to refresh its expected output, not `make results`.
-- **test/install** does NOT get a real diff at all: its actual output is written to the exact same file as its expected output, so a content difference can never fail the build, no matter what changed. The only thing that still fails the build is a hard SQL error, and only if the file has `ON_ERROR_STOP` set (directly or via `\i test/pgxntool/psql.sql`) -- pgxntool checks for this by default. If a `test/install/*.sql` file is misbehaving, don't go looking for a diff; check whether it errored, and don't assume a stale-looking `.out` for it means anything.
+- **test/install** does NOT get a real diff at all: its actual output is written to the exact same file as its expected output, so a content difference can never fail the build, no matter what changed. The only thing that still fails the build is a hard SQL error, and only if the file has `ON_ERROR_STOP` set (directly, or via `\i test/pgxntool/psql.sql` with no later turn-off) -- pgxntool checks for this by default. If a `test/install/*.sql` file is misbehaving, don't go looking for a diff; check whether it errored, and don't assume a stale-looking `.out` for it means anything.
 
 ## Key Implementation Details
 

HISTORY.asc

@@ -56,7 +56,9 @@ written to the same file as their expected output, so a content difference
 can never fail the build. Without `ON_ERROR_STOP`, a hard SQL error was
 silently swallowed too, making the file "pass" regardless of what happened.
 `make test` now fails if a `test/install/*.sql` file doesn't set
-`ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`); see
+`ON_ERROR_STOP`: either with an explicit `\set ON_ERROR_STOP on` (after
+which turning it off again is allowed), or via `\i test/pgxntool/psql.sql`
+with no later turn-off; see
 `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable. Since
 `test/install/*.out` was never really compared against anything, and is
 rewritten by every run, it's now gitignored -- stop committing it.

README.asc

@@ -195,7 +195,7 @@ Without `test/install`, each test file typically needs to run `CREATE EXTENSION`
 
 **Key detail:** Install files and regular tests run in a single `pg_regress` invocation. This means the database is NOT dropped between install and test phases — state created by install files persists into the main test suite.
 
-WARNING: **`+test/install/*.out+` is never actually compared against anything.** Unlike every other test type pgxntool supports, the `test/install` directory's actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The *only* thing that still fails the build is a hard SQL error, and only if the file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (`check-test-install-error-stop`, see `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in `test/sql` instead (or use pgTap assertions from within the install file itself).
+WARNING: **`+test/install/*.out+` is never actually compared against anything.** Unlike every other test type pgxntool supports, the `test/install` directory's actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The *only* thing that still fails the build is a hard SQL error, and only if the file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`; a file relying on `psql.sql` must never turn `ON_ERROR_STOP` back off) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (`check-test-install-error-stop`, see `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in `test/sql` instead (or use pgTap assertions from within the install file itself).
 
 ==== Update & Upgrade (U&U) Testing
 
@@ -771,7 +771,7 @@ Default: auto-detected -- `yes` if `test/install/*.sql` files exist, `no` otherw
 
 === PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK *
 
-Default: `yes`. Enables or disables a build-time check that every `test/install/*.sql` file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`) -- see <<_testinstall,test/install>> for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to `no` to disable the check.
+Default: `yes`. Enables or disables a build-time check that every `test/install/*.sql` file sets `ON_ERROR_STOP`: either with an explicit `\set ON_ERROR_STOP on` (after which turning it off again is allowed), or via `\i test/pgxntool/psql.sql` with no later turn-off -- see <<_testinstall,test/install>> for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to `no` to disable the check.
 
 === PGXNTOOL_ENABLE_FS_INSTALL *
 

README.html

@@ -898,7 +898,7 @@ <h3 id="_testinstall"><a class="anchor" href="#_testinstall"></a><a class="link"
 <div class="title">Warning</div>
 </td>
 <td class="content">
-<strong><code>test/install/*.out</code> is never actually compared against anything.</strong> Unlike every other test type pgxntool supports, the <code>test/install</code> directory&#8217;s actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The <strong>only</strong> thing that still fails the build is a hard SQL error, and only if the file sets <code>ON_ERROR_STOP</code> (directly, or via <code>\i test/pgxntool/psql.sql</code>) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (<code>check-test-install-error-stop</code>, see <code>PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK</code> to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in <code>test/sql</code> instead (or use pgTap assertions from within the install file itself).
+<strong><code>test/install/*.out</code> is never actually compared against anything.</strong> Unlike every other test type pgxntool supports, the <code>test/install</code> directory&#8217;s actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The <strong>only</strong> thing that still fails the build is a hard SQL error, and only if the file sets <code>ON_ERROR_STOP</code> (directly, or via <code>\i test/pgxntool/psql.sql</code>; a file relying on <code>psql.sql</code> must never turn <code>ON_ERROR_STOP</code> back off) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (<code>check-test-install-error-stop</code>, see <code>PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK</code> to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in <code>test/sql</code> instead (or use pgTap assertions from within the install file itself).
 </td>
 </tr>
 </table>
@@ -2065,7 +2065,7 @@ <h3 id="_pgxntool_enable_test_install"><a class="anchor" href="#_pgxntool_enable
 <div class="sect2">
 <h3 id="_pgxntool_enable_test_install_error_stop_check"><a class="anchor" href="#_pgxntool_enable_test_install_error_stop_check"></a><a class="link" href="#_pgxntool_enable_test_install_error_stop_check">8.11. PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK *</a></h3>
 <div class="paragraph">
-<p>Default: <code>yes</code>. Enables or disables a build-time check that every <code>test/install/*.sql</code> file sets <code>ON_ERROR_STOP</code> (directly, or via <code>\i test/pgxntool/psql.sql</code>)&#8201;&#8212;&#8201;see <a href="#_testinstall">test/install</a> for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to <code>no</code> to disable the check.</p>
+<p>Default: <code>yes</code>. Enables or disables a build-time check that every <code>test/install/*.sql</code> file sets <code>ON_ERROR_STOP</code>: either with an explicit <code>\set ON_ERROR_STOP on</code> (after which turning it off again is allowed), or via <code>\i test/pgxntool/psql.sql</code> with no later turn-off&#8201;&#8212;&#8201;see <a href="#_testinstall">test/install</a> for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to <code>no</code> to disable the check.</p>
 </div>
 </div>
 <div class="sect2">
@@ -2134,7 +2134,7 @@ <h2 id="_copyright"><a class="anchor" href="#_copyright"></a><a class="link" hre
 </div>
 <div id="footer">
 <div id="footer-text">
-Last updated 2026-09-16 16:23:56 -0500
+Last updated 2026-10-04 14:56:23 -0500
 </div>
 </div>
 </body>

base.mk

@@ -201,7 +201,8 @@ endif
 # statement that raises a hard error only aborts psql (non-zero exit, which
 # pg_regress does report as a failure) if ON_ERROR_STOP is set. So it is
 # entirely up to each test/install/*.sql file to `\set ON_ERROR_STOP on` (or
-# `\i test/pgxntool/psql.sql`, which already does) if it wants failures
+# `\i test/pgxntool/psql.sql`, which already does, as long as the file never
+# turns it back off) if it wants failures
 # caught at all. check-test-install-error-stop below enforces this by
 # default; see PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK to disable it.
 #

test/bin/check-test-install-error-stop.sh

@@ -12,8 +12,14 @@
 # net so its absence is caught at build time instead of discovered the hard
 # way (issue #97).
 #
-# A file passes if it either sets ON_ERROR_STOP itself, or sources
-# test/pgxntool/psql.sql (which already sets it, among other things).
+# A file passes if it either:
+#   - explicitly enables ON_ERROR_STOP itself (`\set ON_ERROR_STOP` on/true/1/
+#     yes) anywhere; it may then also disable it anywhere, or
+#   - includes test/pgxntool/psql.sql and never disables ON_ERROR_STOP
+#     (`\set ON_ERROR_STOP` off/false/0/no, or `\unset ON_ERROR_STOP`).
+# Every other file fails. A user who explicitly enables ON_ERROR_STOP is
+# assumed to know what they're doing if they also disable it; there's no
+# reason to think a user who includes psql.sql knows it enables ON_ERROR_STOP.
 #
 # Usage: check-test-install-error-stop.sh <testdir>
 
@@ -30,27 +36,48 @@ testdir="$1"
 install_dir="$testdir/install"
 missing=()
 
-for f in "$install_dir"/*.sql; do
-  [ -f "$f" ] || continue
+# Prints why $1 fails the rule in the header, or nothing if it passes.
+error_stop_problem() {
+  local f="$1" line direct_on=no psql_sql=no disabled=no
+  while IFS= read -r line; do
+    if [[ "$line" =~ \\ir?[[:space:]]+.*psql\.sql ]]; then
+      psql_sql=yes
+    elif [[ "$line" =~ \\unset[[:space:]]+ON_ERROR_STOP([[:space:]]|$) ]]; then
+      disabled=yes
+    elif [[ "$line" =~ \\set[[:space:]]+ON_ERROR_STOP[[:space:]]+([^[:space:]]+) ]]; then
+      case "${BASH_REMATCH[1],,}" in
+        on|true|1|yes) direct_on=yes ;;
+        off|false|0|no) disabled=yes ;;
+      esac
+    fi
+  done < "$f"
 
-  if grep -q 'ON_ERROR_STOP' "$f"; then
-    continue
-  fi
-  if grep -qE '\\ir? +.*psql\.sql' "$f"; then
-    continue
+  if [ "$direct_on" = yes ]; then
+    return
+  elif [ "$psql_sql" = no ]; then
+    echo "never sets ON_ERROR_STOP"
+  elif [ "$disabled" = yes ]; then
+    echo "includes psql.sql but also disables ON_ERROR_STOP"
   fi
+}
 
-  missing+=("$f")
+for f in "$install_dir"/*.sql; do
+  [ -f "$f" ] || continue
+  problem=$(error_stop_problem "$f")
+  if [ -n "$problem" ]; then
+    missing+=("$f: $problem")
+  fi
 done
 
 if [ "${#missing[@]}" -gt 0 ]; then
-  error "the following test/install/*.sql files don't set ON_ERROR_STOP:"
+  error "the following test/install/*.sql files don't reliably set ON_ERROR_STOP:"
   printf '  %s\n' "${missing[@]}" >&2
   error "test/install files run in their own self-comparing pg_regress entry" \
     "(see the test/install comments in base.mk) -- without ON_ERROR_STOP, a" \
     "hard SQL error is silently swallowed instead of failing the build."
-  die 1 "Add '\\set ON_ERROR_STOP on' near the top of the file, or" \
-    "'\\i test/pgxntool/psql.sql' (which already sets it)."
+  die 1 "Add '\\set ON_ERROR_STOP on' near the top of the file. Including" \
+    "test/pgxntool/psql.sql (which also sets it) is enough only if the file" \
+    "never turns ON_ERROR_STOP off."
 fi
 
 # vi: expandtab ts=2 sw=2