Read the suite list the way the runner reads it, and sort it (#469)

test/harness_selftest.sh

@@ -186,12 +186,108 @@ 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.
+#
+# 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() {
-	awk '/^SUITES=\(/,/\)/' "$RUNNER" | tr ' \t' '\n\n' |
-		sed -e 's/^SUITES=(//' -e 's/)$//' -e 's/\\$//' |
-		grep -E '^[a-z0-9_]+$'
+	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 ----------------------
+#
+# 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"
+
+# ---- 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)"

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=(
+	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
+	concurrent_diff
+	corruption
+	decode_interrupts
+	differential
+	docs_style
+	drop_cleanup
+	encode_effort
+	encode_invariants
+	fk_referencing
+	fsst_margin
+	fuzz
+	fuzz_arrow
+	fuzz_parquet
+	generated_columns
+	hardening
+	harness_selftest
+	import_deferred
+	import_exclusion
+	index_only
+	isolation
+	logical_subscriber
+	native_agg
+	native_agg_addcolumn
+	native_agg_deletes
+	native_backend_crash
+	native_bloom
+	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_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_read_parquet
+	native_reclaim
+	native_reclaim_cycles
+	native_reclaim_frag
+	native_reclaim_reconcile
+	native_recluster
+	native_repack
+	native_rewrite
+	native_rewrite_conc
+	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_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
+	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
+)
+
+
 # ---------------------------------------------------------------------------
 # 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=(