From 66ed019430b6d4ad4f54ad8325fd7178473345fa Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 5 Aug 2026 21:20:57 -0600 Subject: [PATCH 1/2] docs: record why temporal.sh was asserting nothing, and where else that bites I approved the claim on #411 that temporal constraints are "covered by test/temporal.sh", having checked that the file exists and greps for the right things. On the matrix I would have quoted, it asserted nothing on four majors out of five. Measured after #447 made zero-check runs visible: btree_gist in the SOURCE builds PGDG packages pg15 ABSENT postgresql-18 ships it pg16 ABSENT pg17 ABSENT pg18 present pg19 ABSENT btree_gist is contrib. CI apt-installs postgresql-N and therefore has it, so CI was running this suite honestly. Our source builds are configured without contrib, so the five-major matrix -- the gate we treat as authoritative -- was reporting temporal coverage it never ran. After building contrib/btree_gist against each source tree: pg15 SKIPPED (ran no checks) correct: WITHOUT OVERLAPS is 18+ pg17 SKIPPED (ran no checks) correct: same pg19 8 checks, PASSED previously PASSED having run none So the coverage claim was true on 18 and false on 19, and nothing said so. Three changes, none to product code: - temporal.sh's header records that btree_gist is contrib, that a PGDG package has it and a source build may not, and what the SKIPPED line then means. - docs/testing.md gains a section on reading the skipped count, with the two external dependencies that cause the bad kind of skip: pyarrow for the Arrow and Parquet suites, btree_gist for this one. The distinction that matters is between a feature that does not exist on that major, which is expected, and a tool that is missing, which means coverage was lost silently. - the version-adoption table says "done on 18 and 19" and names the dependency, rather than "done". The environment fix is outside the repository and is done: contrib/btree_gist is now built and installed against pg15, pg16, pg17 and pg19 in the dev container. --- design/POSTGRESQL_VERSION_ADOPTION.md | 2 +- docs/testing.md | 19 +++++++++++++++++++ test/temporal.sh | 7 +++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/design/POSTGRESQL_VERSION_ADOPTION.md b/design/POSTGRESQL_VERSION_ADOPTION.md index 2e59ac7..26acbe2 100644 --- a/design/POSTGRESQL_VERSION_ADOPTION.md +++ b/design/POSTGRESQL_VERSION_ADOPTION.md @@ -19,7 +19,7 @@ date it was written. A note without one has not been checked. | --- | --- | | 1. Read stream and AIO | **shipped**, `pgcolumnar.enable_read_stream` (gap 29) | | 2. Virtual generated columns | **done**, read and storage both, `test/generated_columns.sh` | -| 3. Temporal constraints | **done**, `test/temporal.sh` | +| 3. Temporal constraints | **done** on 18 and 19, `test/temporal.sh`. Needs contrib `btree_gist`, or the suite skips (#447) | | 4. btree skip scan | open, no measurement | | 5. REPACK | **investigated and the conclusion was wrong**, see below (#399) | | 6. Statistics injection | open, no measurement | diff --git a/docs/testing.md b/docs/testing.md index a42c03d..054b23e 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -136,6 +136,25 @@ All suites pass on PostgreSQL 15 through 19. PostgreSQL 19 is validated against 19beta2; revalidation against the final PostgreSQL 19 release is pending that release. +**Read the skipped count, not only the verdict.** A suite that runs no checks +reports `SKIPPED (ran no checks)`. The matrix also prints how many suites ran on +each major. A suite skips for two very different reasons. Only one of them is +expected. The feature under test may not exist on that major, which is correct. +Or a tool the suite measures with is missing, which means real coverage was lost +and the run does not say so anywhere else. + +Two dependencies cause the second kind, and both come from outside this +repository: + +| suite | needs | where it comes from | +| --- | --- | --- | +| the Arrow and Parquet suites | `pyarrow` | `pip install pyarrow`, as the user the suites run as | +| `temporal.sh` | `btree_gist` | PostgreSQL **contrib**. A PGDG package ships it; a source build configured without contrib does not | + +Build and install `contrib/btree_gist` against each source tree when the matrix +runs against source builds. Until that was done, `temporal.sh` asserted nothing +on four of five majors and reported success (#447). + ## Cross-major upgrade `pg_upgrade` is the path that a user takes to a new major. It is also the point diff --git a/test/temporal.sh b/test/temporal.sh index 9fed2cd..e56cfe4 100644 --- a/test/temporal.sh +++ b/test/temporal.sh @@ -11,6 +11,13 @@ # same result set. WITHOUT OVERLAPS needs a GiST index over the scalar key part, # so btree_gist must be available; the suite skips with a note if it is not. # +# btree_gist is CONTRIB. A PGDG package ships it with postgresql-N, so CI has it. A +# source build configured without contrib does not, and this suite then asserts nothing. +# It reported PASSED while doing so until #447, and the local five-major matrix was +# claiming temporal coverage on every major while running it on one. Build and install +# contrib/btree_gist against each source tree, or read the SKIPPED line and know what it +# means. +# # Usage: test/temporal.sh [PG_CONFIG] # # Written fresh for pgColumnar; it does not reuse any upstream test file. From 508c653137ccce7e0d9b964ee26cbe97ae0e099e Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 06:49:43 -0600 Subject: [PATCH 2/2] docs: describe what the suite does now, not what it did this morning @ChronicallyJD: every sentence I added was contradicted by line 40 of the file I added them to. Verified, and they are right. #448 merged at 03:17 and this branch sits on top of it. Since then pgc_skip FAILS on a missing dependency rather than skipping, which was the owner's call and the reason the three-state split exists. My prose described the pre-#448 world throughout: "the suite skips" where it fails, "read the SKIPPED line" where there is none, and "the run does not say so anywhere else" where the run says so loudly. It also never mentioned PGC_ALLOW_MISSING_BTREE_GIST, which is the thing a developer without contrib actually needs. Rewritten against measured behaviour rather than remembered behaviour. All four configurations run on PG18 and PG17: btree_gist present rc=0 PASSED, 5 checks btree_gist absent rc=1 FAILED btree_gist absent, waived rc=2 SKIPPED PG17, feature genuinely absent rc=2 SKIPPED (ran no checks) One correction to the review, which suggested the skipped case is "counted in the suites that ran: n of m line". It is not: run_all_versions.sh increments suites_skipped for rc=2 and suites_ran only for rc=0, and prints "suites that ran: n of m (skipped: k)". The docs now say a skip counts in k rather than n, because the point of the section is telling a reader what they will actually see. The distinction the review called worth keeping is kept, with the verdicts attached: a feature absent on this major is expected and skips, a dependency absent fails, and a waived dependency skips so the loss stays visible in the count instead of becoming a pass. --- design/POSTGRESQL_VERSION_ADOPTION.md | 2 +- docs/testing.md | 42 ++++++++++++++++++++------- test/temporal.sh | 12 ++++++-- 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/design/POSTGRESQL_VERSION_ADOPTION.md b/design/POSTGRESQL_VERSION_ADOPTION.md index 26acbe2..c4d274e 100644 --- a/design/POSTGRESQL_VERSION_ADOPTION.md +++ b/design/POSTGRESQL_VERSION_ADOPTION.md @@ -19,7 +19,7 @@ date it was written. A note without one has not been checked. | --- | --- | | 1. Read stream and AIO | **shipped**, `pgcolumnar.enable_read_stream` (gap 29) | | 2. Virtual generated columns | **done**, read and storage both, `test/generated_columns.sh` | -| 3. Temporal constraints | **done** on 18 and 19, `test/temporal.sh`. Needs contrib `btree_gist`, or the suite skips (#447) | +| 3. Temporal constraints | **done** on 18 and 19, `test/temporal.sh`. Needs contrib `btree_gist`; without it the suite fails rather than passing quietly (#447, #448) | | 4. btree skip scan | open, no measurement | | 5. REPACK | **investigated and the conclusion was wrong**, see below (#399) | | 6. Statistics injection | open, no measurement | diff --git a/docs/testing.md b/docs/testing.md index 054b23e..d08873a 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -136,15 +136,21 @@ All suites pass on PostgreSQL 15 through 19. PostgreSQL 19 is validated against 19beta2; revalidation against the final PostgreSQL 19 release is pending that release. -**Read the skipped count, not only the verdict.** A suite that runs no checks -reports `SKIPPED (ran no checks)`. The matrix also prints how many suites ran on -each major. A suite skips for two very different reasons. Only one of them is -expected. The feature under test may not exist on that major, which is correct. -Or a tool the suite measures with is missing, which means real coverage was lost -and the run does not say so anywhere else. +**A suite has three outcomes, and two of them are not failures.** Read which one +you got before deciding whether something is wrong. -Two dependencies cause the second kind, and both come from outside this -repository: +| outcome | exit | when | +| --- | :-: | --- | +| `PASSED` | 0 | the suite ran its checks and they held | +| `SKIPPED (ran no checks)` | 2 | the feature under test does not exist on this major | +| `FAILED` | 1 | a check failed, **or** a tool the suite measures with is missing | + +The matrix prints `suites that ran: n of m (skipped: k)` for each major, and names +the skipped suites. A skip counts in `k`, not in `n`. + +**A missing dependency fails rather than skips, and that is deliberate.** The box +cannot gate that suite. Reporting a pass would claim coverage nobody has. Two +dependencies come from outside this repository: | suite | needs | where it comes from | | --- | --- | --- | @@ -152,8 +158,24 @@ repository: | `temporal.sh` | `btree_gist` | PostgreSQL **contrib**. A PGDG package ships it; a source build configured without contrib does not | Build and install `contrib/btree_gist` against each source tree when the matrix -runs against source builds. Until that was done, `temporal.sh` asserted nothing -on four of five majors and reported success (#447). +runs against source builds. To run knowingly without a dependency, waive it: + +```sh +PGC_ALLOW_MISSING_BTREE_GIST=1 test/temporal.sh /path/to/pg_config +PGC_ALLOW_MISSING=1 test/run_all_versions.sh +``` + +A waived suite reports `SKIPPED` and exits 2, so the coverage loss stays visible +in the skipped count rather than becoming a pass. + +Measured on PostgreSQL 18, one suite, four configurations: + +| configuration | exit | verdict | +| --- | :-: | --- | +| `btree_gist` present | 0 | `PASSED`, 5 checks | +| `btree_gist` absent | 1 | `FAILED` | +| `btree_gist` absent, waived | 2 | `SKIPPED` | +| PostgreSQL 17, feature absent | 2 | `SKIPPED (ran no checks)` | ## Cross-major upgrade diff --git a/test/temporal.sh b/test/temporal.sh index e56cfe4..f3d65be 100644 --- a/test/temporal.sh +++ b/test/temporal.sh @@ -14,9 +14,15 @@ # btree_gist is CONTRIB. A PGDG package ships it with postgresql-N, so CI has it. A # source build configured without contrib does not, and this suite then asserts nothing. # It reported PASSED while doing so until #447, and the local five-major matrix was -# claiming temporal coverage on every major while running it on one. Build and install -# contrib/btree_gist against each source tree, or read the SKIPPED line and know what it -# means. +# claiming temporal coverage on every major while running it on one. +# +# Since #448 a missing btree_gist FAILS rather than skips, because the box cannot gate +# this suite and a pass would claim coverage nobody has. Build and install +# contrib/btree_gist against each source tree, or set PGC_ALLOW_MISSING_BTREE_GIST=1 to +# run knowingly without it, which reports SKIPPED and keeps the loss visible. +# +# A skip on 15, 16 and 17 is different and is correct: WITHOUT OVERLAPS does not exist +# there, so there is nothing to gate. # # Usage: test/temporal.sh [PG_CONFIG] #