Repository navigation
Add periodic assert-build CI pass - #63
Conversation
- Add a weekly, non-blocking workflow that builds PostgreSQL with --enable-cassert and runs the full regression and TAP suites against it, so invariant violations that Assert() catches (but a release build would silently tolerate or crash differently on) get caught somewhere - Classify every assertion trap against one already-known, already- accepted case (a standby replaying a dropped index) so the job does not go permanently red over a condition everyone already knows about, while still failing loudly on anything else - Never runs on pull_request or push, and is not a required check Signed-off-by: Matt Welch <matt.welch@intel.com>
|
Verified independently:
One open question before merge, not a code bug: the testing notes mention a second, genuinely new finding from your own Minor, non-blocking: in classify_log, if a single TAP file had both a real test-logic failure (is()/ok()) and an unrelated |
- classify_assert_results.sh skipped checking a TAP file's node logs for TRAP/crash signals whenever that file also reported a real test-logic failure, so an unrelated crash in the same file would never show up in the printed summary - Scan node logs regardless of whether the file also had a test-logic failure, and only suppress the generic "no trap found" fallback message when a cause has already been identified - Does not change the verdict or exit code in any case: a test-logic failure already made the count nonzero either way Signed-off-by: Matt Welch <matt.welch@intel.com>
2 participants
Description
Adds a periodic, non-blocking CI pass that builds PostgreSQL with
--enable-cassertand runs this extension's full regression and TAP suites against it, so invariant violations thatAssert()catches (but a release build would silently tolerate or crash differently on) get caught somewhere. The existing build-and-test workflow never builds with assertions enabled, so this is new coverage, not a change to any existing job. Two files are added:.github/workflows/assert-build.yml(the workflow itself, triggered only byscheduleandworkflow_dispatch, neverpull_requestorpush, and not a required status check) andci/classify_assert_results.sh(used by that workflow to tell apart one specific, already-known, already-accepted assertion trap from anything else). The known trap is a standby replaying a dropped index tripping an assertion in this codebase; the guarded code path is a deliberate no-op during recovery either way, so it has no effect on a release build, and a decision on whether to fix the assertion's contract or the call site is a separate, still-open question. A naive "fail on any trap" rule would make this job permanently red the moment that known case shows up in the test suite, so the classifier instead labels every trap or crash signal it finds as either that known case (including any TAP test exit that is a direct, traceable consequence of it) or something new, and only the second category fails the job.A follow-up commit tightens
classify_assert_results.sh's summary output: when a TAP file reported both a real test-logic failure and an unrelated crash signal in its own node logs, only the test-logic failure used to get itemized, because the loop returned early as soon as it found one. Node logs are now always scanned regardless, so a crash alongside a test-logic failure shows up in the printed summary instead of being silently dropped from it. This does not change the verdict or exit code in either case, since a test-logic failure alone was already enough to mark the run UNEXPECTED; it only affects how much detail the summary shows.Related Issues
None.
Type of Change
Pre-Merge Checklist
Build
makecompletes without errors or warningsmake installcompletes successfullyTests
make installcheck) and TAP tests (test/t/) pass with no failurestest/sql/and/ortest/t/test/modules/: it builds and passes (make -C test/modules/<module> installcheck)This PR touches no extension source and introduces no new product behavior, so the first box is the applicable one, but it is intentionally left unchecked rather than marked as a clean pass: see Testing Notes below for exactly what did and did not pass, and why.
Documentation
docs/updated if architecture or usage changedNo architecture or usage change; nothing under
docs/needed updating.Testing Notes
Validated locally end to end against a from-scratch build with
PG_CONFIGURE_EXTRA_ARGS="--enable-debug --enable-cassert --enable-injection-points --enable-tap-tests"andCFLAGS="-Og -g3 -fno-omit-frame-pointer", using the exactci/build_and_test.shsteps the new workflow calls (not a GitHub Actions dispatch).pg_config --configureconfirmed the flags landed in the built PostgreSQL. The canary step the workflow runs after the PostgreSQL install passed: a server withsvsinshared_preload_librariesstarts cleanly,SHOW debug_assertionsreportson, andCREATE EXTENSION injection_pointssucceeds.make installcheckpassed all 7 SQL regression tests with no assertion traps in the server log.make prove_installcheckran all 51 TAP files and found the known trap (Assert("IsTransactionState()")insrc/vamanacache.c) exactly as described, in two different TAP files whose scenario is a standby replaying a dropped index;ci/classify_assert_results.shcorrectly labeled both occurrences KNOWN and did not fail on them. The same run also found one genuinely unexpected result, unrelated to the known trap and unrelated to anything this PR changes: a TAP file exercising error-handling return-value integrity around a checkpoint failure trips a plain SIGSEGV in the background worker, with no assertion trap at all, and this reproduced deterministically on an isolated rerun of just that one file. The classifier correctly flagged this UNEXPECTED and failed on it, which is the intended behavior for exactly this kind of finding; this failure is pre-existing inmain's test suite under an assert build and is being reported separately rather than fixed as part of this PR, since this PR's own diff is CI-config and a classifier script only.actionlintreported no issues against the new workflow file, andshellcheckreported no issues against the new script.One operational note for whoever does the first live trigger: the SVS install step clones
ScalableVectorSearchatmainand builds it against a pinned nightly release tarball of the C bindings. During this validation,mainhad drifted past that pin's API and failed to compile; this is a pre-existing risk in the already-mergedbuild-and-test.ymljob as well; not something introduced by this PR, since both jobs share the same pin, but worth knowing about if the SVS install cache is ever cold when this job runs.The follow-up fix to
classify_assert_results.shwas verified three ways:shellcheckis still clean against the changed script; the real captured logs from the original validation run above produce identical output to before, confirming no regression on the normal path; and a synthetic case (a fake TAP summary reporting one failed assertion alongside a fabricated SIGSEGV line in that same test's node log) now correctly itemizes both findings in the printed summary, where before only the test-logic failure would have appeared and the crash signal would have been silently omitted.