From 40f2cf513df507dbfd0246402ead30ac352c9b53 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 19:53:37 -0600 Subject: [PATCH 1/3] test: read the suite list the way the runner reads it, and one name per line (#469) Two problems, and the second was the serious one. The suite list was a single backslash-continued line, so every pull request that added a suite edited that line and any two conflicted by construction. #469 counted four in one day; four more happened the night #446, #468 and #444 landed. It is now one name per line, so two such branches touch two different lines. The dangerous part was never the conflict, it was the resolution. Appending the new name after the closing paren is valid shell that `bash -n` accepts. I had recorded it as "leaves a stray command that fails at run time"; that is wrong, and measuring it while writing this test is what corrected it: SUITES=(alpha beta gamma) stray_name -> stray_name: command not found -> ${#SUITES[@]} is 0 `NAME=value cmd` scopes the assignment to that one command and an array literal is no exception, so the array is left UNSET and the matrix runs no suites at all. The runner's "NO SUITES RAN" guard is the backstop. And the check that should have caught it could not. harness_selftest derived the list with an awk range plus sed plus grep, which is a reimplementation of bash's array parsing, and the two disagree on precisely this mistake: bash sees no array, awk sees a full list plus the stray name as a member. So "every suite is registered" passed while the array was destroyed. Measured: against a runner carrying the mistake, the awk parser reports 130 names where bash reports 0. The runner now answers `--list-suites`, handled before the run lock because harness_selftest calls it from inside a running matrix. The gate asks the runner instead of parsing it, so there is one parser, bash's, and no way for the two to drift. Verified by removal: restoring the awk parser turns the new checks red. The reformat holds the list byte-identical and in order, compared through `--list-suites` before and after rather than by reading the diff, which is the one thing this file's history says never to trust. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky --- test/harness_selftest.sh | 63 ++++++++++++++- test/run_all_versions.sh | 168 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 224 insertions(+), 7 deletions(-) diff --git a/test/harness_selftest.sh b/test/harness_selftest.sh index 4881557..761d4f9 100755 --- a/test/harness_selftest.sh +++ b/test/harness_selftest.sh @@ -186,12 +186,69 @@ not_a_suite() { } # the SUITES=( ... ) array, flattened to one name per line +# Ask the runner, rather than parsing its source. stderr is dropped because a +# runner carrying the stray-name mistake reports "command not found" on the way +# past it, which is the diagnosis and not this function's output. listed_suites() { - awk '/^SUITES=\(/,/\)/' "$RUNNER" | tr ' \t' '\n\n' | - sed -e 's/^SUITES=(//' -e 's/)$//' -e 's/\\$//' | - grep -E '^[a-z0-9_]+$' + local _r="${1:-$RUNNER}" + bash "$_r" --list-suites 2>/dev/null } +# ---- the list must be read the way the RUNNER reads it ---------------------- +# +# The two checks below rest on listed_suites, so what listed_suites believes is +# load-bearing. It used to believe its own parser: an awk range plus sed plus +# grep, which is a reimplementation of bash's array parsing, and the two disagree +# on exactly the mistake this project keeps making. +# +# Appending a name AFTER the closing paren is valid shell. `bash -n` passes. To +# bash the name is a stray COMMAND and not a member, so the suite never runs. To +# the awk parser it was a member, so "every suite is registered" passed and the +# suite silently did not run. The tally cannot catch it either, because +# "suites that ran: N of M" takes M from ${#SUITES[@]} and is self-consistent +# with the suite missing. +# +# Measured before this was fixed: bash reported "stray_suite: command not found" +# while listed_suites reported it as registered. +# +# So the fixture below is the real runner with that exact mistake applied, and +# the assertion is that the extraction agrees with bash rather than with awk. +_fx="$(mktemp /tmp/pgc-runner-fixture.XXXXXX.sh)" +awk ' + /^SUITES=\(/ { inarr = 1 } + inarr && /\)/ && !seen { print $0 " stray_not_a_suite"; seen = 1; inarr = 0; next } + { print } +' "$RUNNER" > "$_fx" + +check_num "premise: the fixture really does carry the stray name" \ + "$(grep -c 'stray_not_a_suite' "$_fx")" "1" +check_num "a name after the array's closing paren is not read as a registered suite" \ + "$(listed_suites "$_fx" | grep -cx stray_not_a_suite)" "0" + +# And the mistake is worse than a stray command, which is worth pinning because +# the first version of this test assumed otherwise and asserted the opposite. +# +# SUITES=(alpha beta gamma) stray_name +# -> stray_name: command not found +# -> ${#SUITES[@]} is 0 +# +# `NAME=value cmd` scopes the assignment to that one command, and an array +# literal is no exception. So the name after the paren does not join the array, +# it DESTROYS it: every suite disappears and the matrix would run none of them. +# The runner's "NO SUITES RAN" guard is the backstop for that, and this is what +# stops the registration check above from calling the wreck healthy. +check_num "and the mistake empties the whole array rather than appending to it" \ + "$(listed_suites "$_fx" | grep -c .)" "0" + +# The control has to be a runner that is NOT sabotaged, because for the fixture +# above an empty answer is the correct one. Reading the real runner is what shows +# the extraction can return names at all. +check_num "positive control: the real runner's list is read, and contains isolation" \ + "$(listed_suites | grep -cx isolation)" "1" +check "positive control: and it is a whole list, not one lucky line" \ + "$([ "$(listed_suites | grep -c .)" -gt 50 ] && echo yes || echo no)" "yes" +rm -f "$_fx" + unregistered="" for f in "$TESTDIR"/*.sh; do name="$(basename "$f" .sh)" diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 393c006..d6ff42a 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -23,6 +23,154 @@ set -uo pipefail +# One name per line, and it must stay that way. +# +# This was a single backslash-continued line, so every pull request that adds a +# suite edited the same line and any two of them conflicted by construction. It +# happened four times in one day (#459 vs #462, #460 vs #462, #462 vs #468, and +# #444 behind them) and four more times the night #446, #468 and #444 landed. +# +# The resolution was the dangerous part, not the conflict. Appending the new name +# after the closing paren is valid shell that `bash -n` accepts, and it does not +# merely leave a stray command: `NAME=value cmd` scopes the assignment to that +# command, and an array literal is no exception, so `SUITES=(...) my_suite` leaves +# SUITES UNSET and the matrix runs nothing at all. harness_selftest pins that (#469). +# +# One name per line means two pull requests adding two suites touch two different +# lines and merge cleanly. Do not re-flow this into one line to save space. +SUITES=( + harness_selftest + docs_style + smoke + phase2 + phase3 + phase4 + phase5 + phase6 + audit + concurrency + unique_conc + differential + recovery + replication + native_backend_crash + fuzz + fuzz_parquet + fuzz_arrow + hardening + concurrent_diff + parallel + sorted_projection + arrow_export + parquet_export + read_stream + corruption + generated_columns + temporal + arrow_import + index_only + projections + arrow_nested + parquet_import + parquet_nested + arrow_nested_import + parquet_nested_import + native_writer + native_roundtrip + native_encoding + native_fastdecode + native_zonemap + write_minmax_fastpath + write_fsst_compressed + fsst_margin + encode_invariants + encode_effort + native_skip + pushdown_report + zonemap_cost + native_agg + native_agg_deletes + native_agg_addcolumn + native_groupagg + ungrouped_vector_agg + parallel_vector_agg + native_bloom + bloom_sizing + bloom_setting + bloom_lazy + native_vecskip + native_index + native_index_projection + native_fetch_position + native_dml + alter_column_type + native_ios + native_projection + native_cluster + pg19_vacuum_options + native_repack + native_compact + native_recluster + recluster_extent + native_vacuum_race + native_sort_by + sort_status + native_reclaim + native_ownership + drop_cleanup + pg_dump_roundtrip + native_reclaim_cycles + native_reclaim_frag + native_reclaim_reconcile + native_gap + native_format + native_truncate + native_rewrite + native_rewrite_conc + rewrite_group_scan + native_parquet_schema + native_read_parquet + native_parquet_fdw + native_parquet_pushdown + native_parquet_hardening + server_file_privilege + native_parquet_stack + native_parquet_units + native_parquet_flba + native_parquet_codecs + native_parquet_projection + native_parquet_multifile + native_parquet_streaming + native_parquet_partition + native_cancel + cancel_decode + wal_envelope + decode_interrupts + import_exclusion + import_deferred + parallel_copy + parallel_export_parquet + fk_referencing + row_triggers + native_lazy_slot + native_ctas + native_fetch_cache + native_fetch_bigcap + native_fetch_interrupt + analyze_stats + analyze_reltuples + native_fetch_projection + column_projection + advisory_lock_class + logical_subscriber + parallel_degree + planner_choice_quality + objstore_module + objstore_stash_recovery + isolation +) + + # --------------------------------------------------------------------------- # Run from a private copy of this script, and refuse to run twice at once. # @@ -52,6 +200,22 @@ PGC_RUN_LOCK="${PGC_RUN_LOCK:-/tmp/pgcolumnar-run_all_versions.lock}" # pattern misses it, and killing postmasters directly bypasses pg_ctl. This reads # the lock, signals the owner, and lets the owner's own trap stop the suites and # their clusters properly. +# --list-suites: print the matrix's suite list, one name per line, then exit. +# +# It exists so that nothing has to parse this array a second time. A gate that +# re-implements bash's array parsing in awk disagrees with bash on the very +# mistake this array invites: a name after the closing paren is a stray COMMAND +# to bash and a member to a text parser, so the gate passed while the suite +# silently never ran. Asking the runner means there is one parser, bash's, and +# no way for the two to drift. +# +# Handled BEFORE the run lock on purpose. harness_selftest calls this from +# inside a running matrix, and taking the lock there would refuse to answer. +if [ "${1:-}" = "--list-suites" ]; then + printf '%s\n' "${SUITES[@]}" + exit 0 +fi + if [ "${1:-}" = "--stop" ]; then if [ ! -e "$PGC_RUN_LOCK" ]; then echo "no matrix run in progress (no lock at $PGC_RUN_LOCK)" @@ -205,10 +369,6 @@ pgc_run_cleanup() { trap pgc_run_cleanup INT TERM SRCDIR="${PGC_RUN_SRCDIR:-$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)}" -SUITES=(harness_selftest docs_style smoke phase2 phase3 phase4 phase5 phase6 audit concurrency unique_conc \ - differential recovery replication native_backend_crash fuzz fuzz_parquet fuzz_arrow hardening concurrent_diff parallel sorted_projection \ - arrow_export parquet_export read_stream corruption \ - generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report zonemap_cost native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_sizing bloom_setting bloom_lazy native_vecskip native_index native_index_projection native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster pg19_vacuum_options native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_bigcap native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection advisory_lock_class logical_subscriber parallel_degree planner_choice_quality objstore_module objstore_stash_recovery isolation) # Default matrix: one assert-enabled pg_config per major, 15 through 19. DEFAULT_CONFIGS=( From ea3808ecf35efa7b3b7632c2a56263ea1fbde146 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 19:57:33 -0600 Subject: [PATCH 2/3] test: sort the suite list, which is what actually reduces the conflicts (#469) One name per line was not enough, and measuring it is what showed that. Branching twice off a base, adding one suite on each branch, and merging: one line, both additions on the same line CONFLICT one per line, both appended at the end CONFLICT one per line + sorted, names far apart clean one per line + sorted, names that sort adjacently CONFLICT Everyone appends at the end. That is the shape all four of #469's conflicts had and all four of this session's, so one-per-line on its own would have left every one of them still conflicting. Sorting is what gives a new suite an insertion point decided by its name, so two unrelated additions land in different places. Stated honestly: this is a large reduction, not a cure. Two suites whose names sort next to each other still collide. What has gone for good is the destructive resolution, because the closing paren now sits on its own line, and harness_selftest catches that mistake whatever the layout. The sortedness check is what keeps the property. Without it the order decays the first time somebody appends by hand and the reduction quietly disappears. Run order changes: the array is walked in order, so the summary now prints alphabetically and ports are assigned in that order. Neither is a correctness property. The parallel batch runs six at a time with no ordering guarantee already, and runs_alone still selects the sequential ones. Verified that the SET of suites is unchanged, through --list-suites rather than by reading the diff: 129 before, 129 after, identical. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky --- test/harness_selftest.sh | 23 +++++ test/run_all_versions.sh | 212 +++++++++++++++++++-------------------- 2 files changed, 129 insertions(+), 106 deletions(-) diff --git a/test/harness_selftest.sh b/test/harness_selftest.sh index 761d4f9..4161823 100755 --- a/test/harness_selftest.sh +++ b/test/harness_selftest.sh @@ -249,6 +249,29 @@ check "positive control: and it is a whole list, not one lucky line" \ "$([ "$(listed_suites | grep -c .)" -gt 50 ] && echo yes || echo no)" "yes" rm -f "$_fx" +# ---- the list stays sorted, which is what actually stops the conflicts ------ +# +# One name per line was not enough on its own. Measured, on this repository, by +# branching twice and merging: +# +# one line, both additions on the same line CONFLICT +# one per line, both appended at the end CONFLICT +# one per line + sorted, names far apart clean +# one per line + sorted, names that sort adjacently CONFLICT +# +# Everyone appends at the end, which is the shape all four of #469's conflicts +# had, so one-per-line alone would have left them all conflicting. Sorted gives a +# new suite an insertion point decided by its NAME, so two unrelated additions +# land in different places and merge. It is a large reduction and not a cure: +# two names that sort next to each other still collide. +# +# This check is what keeps the property true. Without it the order decays the +# first time somebody appends by hand, and the reduction quietly goes away. +_sorted_expected="$(listed_suites | sort)" +_sorted_actual="$(listed_suites)" +check "the suite list is sorted, so two new suites land in different places" \ + "$([ "$_sorted_actual" = "$_sorted_expected" ] && echo sorted || echo "not sorted")" "sorted" + unregistered="" for f in "$TESTDIR"/*.sh; do name="$(basename "$f" .sh)" diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index d6ff42a..0b3e4f6 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -39,135 +39,135 @@ set -uo pipefail # One name per line means two pull requests adding two suites touch two different # lines and merge cleanly. Do not re-flow this into one line to save space. SUITES=( - harness_selftest - docs_style - smoke - phase2 - phase3 - phase4 - phase5 - phase6 + advisory_lock_class + alter_column_type + analyze_reltuples + analyze_stats + arrow_export + arrow_import + arrow_nested + arrow_nested_import audit + bloom_lazy + bloom_setting + bloom_sizing + cancel_decode + column_projection concurrency - unique_conc + concurrent_diff + corruption + decode_interrupts differential - recovery - replication - native_backend_crash + docs_style + drop_cleanup + encode_effort + encode_invariants + fk_referencing + fsst_margin fuzz - fuzz_parquet fuzz_arrow - hardening - concurrent_diff - parallel - sorted_projection - arrow_export - parquet_export - read_stream - corruption + fuzz_parquet generated_columns - temporal - arrow_import + hardening + harness_selftest + import_deferred + import_exclusion index_only - projections - arrow_nested - parquet_import - parquet_nested - arrow_nested_import - parquet_nested_import - native_writer - native_roundtrip - native_encoding - native_fastdecode - native_zonemap - write_minmax_fastpath - write_fsst_compressed - fsst_margin - encode_invariants - encode_effort - native_skip - pushdown_report - zonemap_cost + isolation + logical_subscriber native_agg - native_agg_deletes native_agg_addcolumn - native_groupagg - ungrouped_vector_agg - parallel_vector_agg + native_agg_deletes + native_backend_crash native_bloom - bloom_sizing - bloom_setting - bloom_lazy - native_vecskip + native_cancel + native_cluster + native_compact + native_ctas + native_dml + native_encoding + native_fastdecode + native_fetch_bigcap + native_fetch_cache + native_fetch_interrupt + native_fetch_position + native_fetch_projection + native_format + native_gap + native_groupagg native_index native_index_projection - native_fetch_position - native_dml - alter_column_type native_ios + native_lazy_slot + native_ownership + native_parquet_codecs + native_parquet_fdw + native_parquet_flba + native_parquet_hardening + native_parquet_multifile + native_parquet_partition + native_parquet_projection + native_parquet_pushdown + native_parquet_schema + native_parquet_stack + native_parquet_streaming + native_parquet_units native_projection - native_cluster - pg19_vacuum_options - native_repack - native_compact - native_recluster - recluster_extent - native_vacuum_race - native_sort_by - sort_status + native_read_parquet native_reclaim - native_ownership - drop_cleanup - pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile - native_gap - native_format - native_truncate + native_recluster + native_repack native_rewrite native_rewrite_conc - rewrite_group_scan - native_parquet_schema - native_read_parquet - native_parquet_fdw - native_parquet_pushdown - native_parquet_hardening - server_file_privilege - native_parquet_stack - native_parquet_units - native_parquet_flba - native_parquet_codecs - native_parquet_projection - native_parquet_multifile - native_parquet_streaming - native_parquet_partition - native_cancel - cancel_decode - wal_envelope - decode_interrupts - import_exclusion - import_deferred + native_roundtrip + native_skip + native_sort_by + native_truncate + native_vacuum_race + native_vecskip + native_writer + native_zonemap + objstore_module + objstore_stash_recovery + parallel parallel_copy - parallel_export_parquet - fk_referencing - row_triggers - native_lazy_slot - native_ctas - native_fetch_cache - native_fetch_bigcap - native_fetch_interrupt - analyze_stats - analyze_reltuples - native_fetch_projection - column_projection - advisory_lock_class - logical_subscriber parallel_degree + parallel_export_parquet + parallel_vector_agg + parquet_export + parquet_import + parquet_nested + parquet_nested_import + pg19_vacuum_options + pg_dump_roundtrip + phase2 + phase3 + phase4 + phase5 + phase6 planner_choice_quality - objstore_module - objstore_stash_recovery - isolation + projections + pushdown_report + read_stream + recluster_extent + recovery + replication + rewrite_group_scan + row_triggers + server_file_privilege + smoke + sort_status + sorted_projection + temporal + ungrouped_vector_agg + unique_conc + wal_envelope + write_fsst_compressed + write_minmax_fastpath + zonemap_cost ) From 5a55c8c24c24f2f219bb1abb3cba91119e9915a6 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 20:26:42 -0600 Subject: [PATCH 3/3] test: ask the runner for its suite list once, not once per file (#469) The five-major matrix failed harness_selftest on PG16 and PG17 while PG15, PG18 and PG19 passed: FAIL every suite is registered in run_all_versions.sh: got [unregistered: differential parallel_copy parallel_vector_agg planner_choice_quality] want [none] Those four are registered. Different names on each major, and only under the matrix, which is the signature of load rather than of a missing entry. listed_suites forked `bash run_all_versions.sh --list-suites` on every call, and the two checks below it call it once per test file: about 250 forks, inside a six-way parallel matrix. A transient failure to fork returns an empty list, and an empty list reads as "this suite is unregistered", naming whichever files happened to be in hand at the time. So ask once, for the real runner, and cache it. The fixture path still takes an explicit argument, because that one has to parse a different file. An empty answer is now a named premise rather than a silent cause of an intermittent red against innocent suites. That is the failure mode this whole change exists to remove, so leaving my own version of it in place would have been a poor joke. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky --- test/harness_selftest.sh | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/test/harness_selftest.sh b/test/harness_selftest.sh index 4161823..981d1cd 100755 --- a/test/harness_selftest.sh +++ b/test/harness_selftest.sh @@ -189,9 +189,25 @@ not_a_suite() { # Ask the runner, rather than parsing its source. stderr is dropped because a # runner carrying the stray-name mistake reports "command not found" on the way # past it, which is the diagnosis and not this function's output. +# +# Asked ONCE, for the real runner, and cached. The checks below call this inside +# two loops over every test file, so the first version forked a fresh bash 250-odd +# times. Under a six-way matrix that is slow and, worse, fragile: a transient +# failure to fork returns an empty list, and an empty list reads as "that suite is +# unregistered". It did exactly that in the #473 matrix, failing on PG16 and PG17 +# with four names each, different names each time, while PG15/18/19 passed. An +# intermittent red naming innocent suites is the worst kind, so the premise below +# makes an empty answer say what it is. +_SUITE_LIST="$(bash "$RUNNER" --list-suites 2>/dev/null)" +check "premise: the runner answered --list-suites, so the two checks below mean something" \ + "$([ -n "$_SUITE_LIST" ] && echo yes || echo "no (empty)")" "yes" + listed_suites() { - local _r="${1:-$RUNNER}" - bash "$_r" --list-suites 2>/dev/null + if [ $# -gt 0 ]; then + bash "$1" --list-suites 2>/dev/null + else + printf '%s\n' "$_SUITE_LIST" + fi } # ---- the list must be read the way the RUNNER reads it ----------------------