Repository navigation
Conversation
Add src/test/prop/sql/ with a shared random-query generator (query_gen.c) and three metamorphic query-level areas that test the planner and executor rather than individual functions: query_tlp (Ternary Logic Partitioning), query_norec (non-optimizing reference), and query_planadvice (the same query run with the planner free and steered by contrib/pg_plan_advice, plus a tiny-work_mem spill variant, must return identical rows). These shrink any counterexample to a minimal query via the same engine as the rest of the suite.
Four more query-level areas built on the shared query_gen.c generator, each a metamorphic or known-fact oracle over randomly generated tables and queries: * query_tlp_agg -- the ternary predicate partition applied to count/sum/min/max aggregates, which must fold back to the whole-table aggregate. * query_tlp_distinct -- the same partition under DISTINCT and the set operators (UNION distributes, the p and NOT-p partitions are disjoint, EXCEPT recovers the NULL partition). * query_cardinality -- result-size monotonicity laws (adding a conjunct cannot grow, a disjunct cannot shrink, the partition tiles the table, DISTINCT never grows, UNION ALL is additive). * query_pqs -- Pivoted Query Synthesis: pick a real pivot row, synthesise a predicate guaranteed true for it, and require it in the result (and across a self-join). Checks one query against one known-true fact rather than two queries against each other. These are the oracles most likely to surface a live wrong-results bug.
Add a NoREC property that pushes the generated predicate across a self-join (join_filter_matches_norec), comparing the optimisable filtered join against the un-optimisable twin that evaluates the predicate per surviving join row -- the join/pushdown interaction where NoREC found real optimiser bugs elsewhere. Add a plan-advice property that steers a three-way self-join to a specific JOIN_ORDER (join_order_plan_invariant); join reordering is a distinct code path from join-method selection and the result must not depend on it.
…rage Add query_planadvice/mergejoin_plan_invariant: the same self-join steered to MERGE_JOIN_PLAIN, whose sorted-input lockstep path is distinct from the nested-loop and hash paths and has its own NULL-key handling. On a branch-enabled coverage build this takes executor/nodeMergejoin.c from 0 percent (never entered) to 57.6 percent lines, 63.6 percent functions, 29.5 percent branches with the plan-advice area alone. Record the function and branch coverage of the query-level areas in DESIGN; branch coverage is the metric that surfaced the merge-join gap in the first place.
Let an extension participate in the suite without touching core. Two paths:
(a) Add its own area -- already supported: a prop_<ext>.c with PG_PROP_AREA
and PG_PROP() compiled into the pg_prop binary registers itself.
(b) Augment an existing area -- new: a core area publishes a named extension
point and calls pg_prop_run_hooks() there; an extension registers a
contributor with PG_PROP_HOOK(point, fn). The harness gains a small hook
registry (pg_prop_register_hook / run_hooks / run_hook_n / hooks_count)
alongside the property registry.
Rework the SQL query generator around a schema (qg_schema) instead of a
hard-coded column list, and publish three points: query_gen.setup (run DDL such
as CREATE EXTENSION before each table), query_gen.column (add a typed column),
and query_gen.predicate (emit a predicate leaf over it). Core now also
generates numeric(5,2) and boolean columns for wider type coverage. An
extension that registers those three hooks gets its type exercised by every
query-level oracle -- TLP, NoREC, cardinality, PQS, plan-advice -- with no new
oracle code. Document the whole mechanism in README (Extensions) and pg_prop.h.
Add sql/prop_query_ext_citext.c, the reference for how an extension plugs its type into the query generator. It registers all three query_gen hooks (setup: CREATE EXTENSION citext; column: a mixed-case citext column ci; predicate: case-insensitive leaves over ci), after which the generated tables carry a citext column and the generated predicates exercise case-insensitive comparison through every query-level oracle. It also adds one citext-specific area (eq_matches_lower, a differential against lower()-folded text) to show both the new-area and augment-existing-area styles in one file. Verified on a live node: the generated qg_t gains the ci citext column and the oracles run citext predicates (ci =, <>, <, IS NULL) over it; full TAP suite green at 246 Tier-B areas.
📜 Change history & discussion (Agora / pg.ddx.io)Confirmed: nothing in the archives for 🧵 Related discussion
🔗 Related commits / prior art
📋 Commitfest
🧭 Context for reviewers
Generated by pg-history via the Agora MCP server (pg.ddx.io). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 OCR found 109 issue(s).
- 25 inline, 84 in summary (inline capped at 25)
⚠️ 1 warning(s) during review
📄 src/test/prop/prop_nbtree.c (L198-L203)
The forced-index-scan oracle is not verified and can silently degrade to seqscan-vs-seqscan. enable_seqscan=off only penalizes the seqscan cost; the planner still falls back to a sequential scan when no index path is viable. build() draws nrows from 0..60 (frequently 0, and single-digit the rest of the time), so for most generated tables the planner will choose a seqscan despite these GUCs. When that happens, idx and seq are produced by the same plan and the comparison is a tautology that holds even if an nbtree scan-key bug is reintroduced -- a test that passes with the feature reverted, which the review rules call worthless. The DESIGN itself notes enable_* GUCs are "blunt" and may be ignored. To make the oracle sound, confirm the "index" run actually used an index scan (e.g. parse EXPLAIN (FORMAT text) / EXPLAIN (FORMAT json) for the plan node type and pg_prop_assume(false) otherwise), or bias nrows high enough that an index path is reliably chosen. (high confidence)
📄 src/test/prop/prop_spillpaths.c (L48-L50)
work_mem = '64kB' does not reliably force a spill at this table size, so the spill-vs-in-memory differential is frequently in-memory-vs-in-memory and would pass even with a spill-path regression. build_table() draws nrows from 0..60; a sort/hashagg/hashjoin over at most 60 narrow rows commonly fits in 64kB and never spills to tape/disk. The DESIGN confirms this: "the spill paths ... the tiny-work_mem variants reach only intermittently at this table size." An oracle that only intermittently exercises the code it targets cannot be relied on to catch a regression in that code. Consider forcing much larger row counts for these properties (enough that 64kB cannot hold the working set), and/or verifying via EXPLAIN (ANALYZE) that the operator reported disk usage, so the property actually tests the spill path it claims to.
📄 src/test/prop/prop_bool.c (L103-L106)
This require_true helper is copied verbatim into all four files under review (and, per a tree-wide search, ~210 prop_*.c files in src/test/prop). The framework in pg_prop.h already exposes the primitives (pg_prop_sql_scalar, pg_prop_fail) but no shared "require boolean true" assertion, so every test file re-declares an identical static copy. This is a DRY violation; the helper (scalar query + fail on error/NULL/non-"t") should live once in pg_prop.c/.h and be reused. (confidence: high; framework-wide, not specific to this file)
📄 src/test/prop/prop_boolcast.c (L127-L127)
Comment/coverage drift: the comment claims "the generic rule (0 false, nonzero true) is checked against the n=0 vs n<>0 split", but n = pg_prop_draw_bool(tc) ? 1 : 0 restricts n to {0,1}. The only nonzero value ever exercised is exactly 1, so the generic nonzero case (e.g. 5, -3) is never tested. Either draw a wider range via pg_prop_draw_int(tc, INT32_MIN, INT32_MAX) to actually exercise the generic rule, or soften the comment to say only {0,1} is covered. (confidence: high)
💡 Suggested change
Before:
n = pg_prop_draw_bool(tc) ? 1 : 0;
After:
n = (int) pg_prop_draw_int(tc, INT32_MIN, INT32_MAX);
📄 src/test/prop/prop_boolcast.c (L70-L73)
Test effectiveness: six of the seven properties in this file (bool_text_variants, bool_to_int, bool_text_roundtrip, not_toggles, bool_and_or_identity, case_insensitive_input) use entirely fixed SQL with no draws, so they are constant assertions rather than property-based tests that explore the input space. These overlap with what belongs in src/test/regress and add little over the drawn-input variants in prop_bool.c. Consider whether the fixed-input ones earn their place here. (confidence: moderate)
📄 src/test/prop/prop_nbtree.c (L84-L88)
Using pg_prop_assume(tc, false) here silently discards the case when DDL fails, but this DDL is entirely static and valid: fixed column names a,b,c, int types, and legal ASC/DESC NULLS FIRST/LAST options. No input a correct server accepts could legitimately reject it. If the server actually errors or crashes on a particular index shape (precisely the nbtree/skip-scan bug class this test targets), the failure is swallowed as a "rejected precondition" instead of being reported. This should be pg_prop_fail, matching build_table() in prop_spillpaths.c. (high confidence)
💡 Suggested change
Before:
pg_prop_note(tc, "schema rejected: %s", err ? err : "");
if (err)
pg_free(err);
pfree(ddl.data);
pg_prop_assume(tc, false);
After:
pfree(ddl.data);
pg_prop_fail(tc, "schema build failed: %s", err ? err : "");
📄 src/test/prop/prop_nbtree.c (L111-L115)
Same masking problem on the insert path: the rows only ever contain integer literals or NULL into int columns, so a correct server cannot reject them. Discarding the case via assume hides any backend error/crash triggered by the generated data. Use pg_prop_fail here too. (high confidence)
💡 Suggested change
Before:
pg_prop_note(tc, "insert rejected: %s", err ? err : "");
if (err)
pg_free(err);
pfree(ins.data);
pg_prop_assume(tc, false);
After:
pfree(ins.data);
pg_prop_fail(tc, "insert failed: %s", err ? err : "");
📄 src/test/prop/prop_cube.c (L124-L125)
The fixed-size lo[4]/hi[4] arrays are indexed by k < d, where d is bounded only implicitly: every caller passes d from pg_prop_draw_int(tc, 1, 4) (inclusive bounds, so max 4). The array size and that 4 are coupled with nothing tying them together, so a future caller passing a larger dimension would silently overflow the stack. Consider defining a MAX_CUBE_DIM constant used both for the array size and the draw bound (as prop_cubedist.c already does with MAX_DIM), or add an Assert(d <= 4).
📄 src/test/prop/prop_bool.c (L308-L319)
comparison_consistent draws no inputs; it is a single fixed assertion, so the engine's generator/shrinker adds nothing and it is a plain constant unit test wearing a property's clothing (same class as the fixed-SQL properties already flagged in prop_boolcast.c, but this one lives in prop_bool.c). As written it would still pass if everything else about boolean comparison were broken, since it only touches the two literal orderings. Consider either folding this anchor check into a plain assertion or driving it with a drawn pair of boolean literals so it actually explores the comparison operators.
📄 src/test/prop/prop_boxops.c (L333-L338)
This function-level comment is stale and contradicts the code it documents. It claims the invariant is area(polygon(B)) = area(B) and that the test "compare[s] within a tolerance", but the property is named box_to_polygon_contains_center and the body actually tests polygon(box) @> center(box). The inline comment at lines 357-360 even admits "area(polygon) is not defined", directly contradicting this block. Rewrite the comment to describe the center-containment invariant the code actually checks.
📄 src/test/prop/prop_boxops.c (L11-L12)
The file-header description is also stale: it claims "a polygon built from a box encloses the same area" and lists "polygon(box) area against box area" as an oracle. Since area(polygon) is not defined (per the inline comment at line 357) and the shipped property instead checks center containment, update this header to match the implemented invariant.
📄 src/test/prop/prop_condexpr.c (L321-L322)
This oracle compares coalesce(a, coalesce(b, c)) against the flat coalesce(a, b, c), but both sides evaluate the same CoalesceExpr executor node with identical first-non-NULL semantics. It only verifies that nesting is equivalent to flattening, not that COALESCE's first-non-NULL rule is itself correct: a bug in CoalesceExpr evaluation would corrupt both sides identically and this property would still pass. The header comment's claim that this "pins" COALESCE semantics overstates what is checked. Consider making the oracle independent of CoalesceExpr -- e.g. a hand-written searched CASE on IS NOT NULL, as prop_coalesce.c's coalesce_first_nonnull already does -- so a genuine COALESCE regression would be caught. (Low confidence as a blocker; the metamorphic style is an acknowledged framework tradeoff per DESIGN, and sibling files do carry an independent oracle.)
📄 src/test/prop/prop_collate.c (L268-L268)
This oracle is weak to the point of being near-tautological: require_same_rows(tc, conn, q.data, q.data) runs the identical deterministic query twice on the same connection, so it will match regardless of whether ORDER BY x COLLATE "C" actually produces the correct order. It would still pass if C-collation ordering were consistently wrong. The determinism it checks (same input -> same output within one backend) is almost guaranteed by construction. Consider making it catch real regressions, e.g. compare the collated ORDER BY against an independent oracle (the bytea/convert_to byte-order sort already used in c_collation_is_byte_order), rather than comparing a query to itself.
📄 src/test/prop/prop_coalescefn.c (L127-L129)
This coalesce_associative oracle compares coalesce(a, coalesce(b, c)) against coalesce(coalesce(a, b), c), but both sides evaluate the same CoalesceExpr executor node with identical first-non-NULL semantics. It only verifies that nesting folds the same way left or right; it does not pin down COALESCE's first-non-NULL rule, so it would still pass if that rule were implemented incorrectly (as long as it stays deterministic). This is the same tautology class already confirmed for nested_coalesce in prop_condexpr.c. The genuine oracle lives in coalesce_first_nonnull_wins (COALESCE vs hand-written CASE); consider stating associativity against that CASE-based form, or dropping this property as redundant. (moderate confidence)
📄 src/test/prop/prop_numcast.c (L405-L411)
The oracle here is tautological on the non-overflow path. The SELECT compares n::int8::int2 against the identical expression n::int8::int2, so when the narrowing fits, the result is X = X -- trivially true regardless of whether int8->int2 produced the correct value. The only thing this property actually exercises is the overflow-error path (via require_true_or_skip); the "fits exactly" half claimed in the header comment is never checked. A narrowing cast that silently produced a wrong-but-deterministic value would still pass. Use a round-trip oracle that re-widens and compares to the original, e.g. n::int8::int2::int8 = n::int8, so a wrong narrowed value is caught. (moderate confidence)
💡 Suggested change
Before:
appendStringInfoString(&q, "SELECT (");
append_int_literal(&q, n);
appendStringInfoString(&q, "::int8::int2) = ");
append_int_literal(&q, n);
appendStringInfoString(&q, "::int8::int2");
require_true_or_skip(tc, conn, q.data);
After:
appendStringInfoString(&q, "SELECT (");
append_int_literal(&q, n);
appendStringInfoString(&q, "::int8::int2::int8) = ");
append_int_literal(&q, n);
appendStringInfoString(&q, "::int8");
require_true_or_skip(tc, conn, q.data);
📄 src/test/prop/prop_defaults.c (L236-L238)
The drawn values k1/k2 only populate column v, which the asserted invariant min(id) < max(id) never references. Identity generation is deterministic, so every iteration runs an identical effective test and the random draws are dead inputs that add no regression-catching value. Either assert something that depends on the drawn data, or drop the draws and run this as a single deterministic case.
Also the comment's claim that "ordered by insertion value is not reliable" is moot here since the assertion doesn't order by v at all; the explanation describes a concern the code doesn't actually have.
📄 src/test/prop/prop_inetarith.c (L58-L61)
The is_overflow() matcher is too broad and will mask genuine bugs. inet +/- int only ever raises errmsg("result is out of range") (network.c:1914, 1990), which the "out of range" substring already catches. The "cannot" branch, however, also matches real errors such as "cannot subtract inet values of different sizes", "cannot AND/OR inet values of different sizes", and "cannot merge addresses from different families" (network.c:1413, 1821, 1853, 1955). If a regression ever produced one of those on these supposedly single-family IPv4-host inputs, require_true_or_skip would silently skip it instead of failing — exactly the class of bug this oracle exists to catch. Also note "would overflow" is never emitted by network.c and "result is out of range" is redundant with "out of range". Tighten this to the single string actually emitted.
💡 Suggested change
Before:
return strstr(msg, "out of range") != NULL ||
strstr(msg, "would overflow") != NULL ||
strstr(msg, "cannot") != NULL ||
strstr(msg, "result is out of range") != NULL;
After:
return strstr(msg, "result is out of range") != NULL;
📄 src/test/prop/prop_inetarith.c (L69-L74)
This require_true helper is byte-for-byte identical across all seven files in this group (prop_cidrops.c:56, prop_inet.c:90, prop_inet6.c:65, prop_inetarith.c:70, prop_macaddr.c:159, prop_macaddr8.c:93, prop_networkops.c:55), and the same copy appears in dozens of other prop_*.c files in this patch. This is a boolean-assert-against-oracle wrapper over the already-shared pg_prop_sql_scalar; it belongs in the harness (pg_prop.h / pg_prop_sql.c) so every Tier-B area reuses one implementation rather than carrying its own copy. Consolidating also removes the risk of the copies drifting apart. The near-identical require_true_or_skip + is_overflow pair is a further candidate for the harness.
📄 src/test/prop/prop_checkconstraint.c (L9-L10)
The file header claims each property "builds a fresh scratch table (and domain, where relevant) with a unique name", but every property here uses a fixed table name (cc_rv, cc_null, cc_dom, cc_mc, cc_ce, cc_nn) and relies on DROP TABLE IF EXISTS, not a unique name. Comments must describe what the code does now; please drop the "with a unique name" claim. (low confidence it causes any functional issue; it is a doc-accuracy nit.)
📄 src/test/prop/prop_checkconstraint.c (L29-L29)
This file includes lib/stringinfo.h but never uses any StringInfo API (no dynamic SQL is built here; all SQL is static string literals). Drop the unused include. (prop_defaults.c and prop_foreignkey.c do use StringInfo, so the include is correct there.)
📄 src/test/prop/prop_defaults.c (L168-L170)
The drawn value k is a dead input for this property's oracle. The only asserted invariant is g = 2m after the UPDATE; the intermediate value g = 2k produced by the INSERT is never checked, so the test result is independent of k and the property passes identically for every draw. This is a distinct occurrence from the already-noted dead-draw in serial_increments. Either assert g = 2k right after the INSERT (before the UPDATE), or remove the k draw and insert a constant.
📄 src/test/prop/prop_datecast.c (L239-L250)
Properties 4 (timestamp_to_timestamptz) and 5 (timestamptz_to_timestamp) generate byte-for-byte equivalent SQL -- the only difference is a redundant outer pair of parentheses around the left operand. Both assert ((make_timestamp(...)::timestamptz) AT TIME ZONE 'UTC') = make_timestamp(...), exercising the exact same code path. The comment itself concedes they are "Equivalent to property 4 by UTC's symmetry". A property-based test that can't catch any regression its sibling misses is dead scaffolding (YAGNI). To genuinely assert the reverse direction, property 5 should build the operand as a timestamptz and convert to timestamp (e.g. start from make_timestamptz(...) and compare its AT TIME ZONE 'UTC' result against a plain make_timestamp(...)), rather than restating property 4. Confidence: high.
📄 src/test/prop/prop_tochar.c (L42-L46)
is_overflow() treats any error containing the substring "cannot" as a legitimate, skippable condition. Combined with require_true_or_skip, this means a genuine formatting.c regression whose error text contains "cannot" (e.g. "cannot convert ..." and similar) would be silently discarded via pg_prop_assume(tc, false) instead of being reported as a failure. That makes these properties unable to catch a whole class of real bugs -- a test that skips the very errors it should flag is worthless. The sibling helper in prop_float.c deliberately matches only "out of range"/"overflow"; the bounded input ranges here (per the comment) mean no legitimate overflow is expected at all. Recommend dropping the "cannot" clause so only true out-of-range/overflow is skipped.
💡 Suggested change
Before:
if (errmsg == NULL)
return false;
return strstr(errmsg, "out of range") != NULL ||
strstr(errmsg, "overflow") != NULL ||
strstr(errmsg, "cannot") != NULL;
After:
if (errmsg == NULL)
return false;
return strstr(errmsg, "out of range") != NULL ||
strstr(errmsg, "overflow") != NULL;
📄 src/test/prop/prop_toformat.c (L40-L44)
Same issue as prop_tochar.c: matching the substring "cannot" as a skippable "overflow" condition is too broad. A genuine formatting.c error whose message contains "cannot" would be silently assumed away by require_true_or_skip rather than failing the property, weakening its ability to catch regressions. Match only "out of range"/"overflow" (as prop_float.c does).
💡 Suggested change
Before:
if (errmsg == NULL)
return false;
return strstr(errmsg, "out of range") != NULL ||
strstr(errmsg, "overflow") != NULL ||
strstr(errmsg, "cannot") != NULL;
After:
if (errmsg == NULL)
return false;
return strstr(errmsg, "out of range") != NULL ||
strstr(errmsg, "overflow") != NULL;
📄 src/test/prop/prop_date.c (L162-L162)
This overflow-skip branch is dead code and the rationale is factually wrong. The supported DATE range is 4713 BC .. 5874897 AD. draw_date() picks D in year 2000-2050, and k is drawn from only [-100000, 100000] days (~274 years), so D + k always lands roughly within year 1726..2324 -- far inside the supported range. date + integer therefore can never raise an out-of-range error here, so is_overflow(err) can never be true and the pg_prop_assume(false) path never executes. Either widen k enough that overflow is actually reachable (and keep the skip), or drop the skip entirely and treat any error as a failure. The comment claiming k "can legally run off the ends of the supported date range" should be corrected. [low confidence]
📄 src/test/prop/prop_date.c (L261-L261)
Same dead-code / misleading-comment issue as add_sub_days_inverse: with D in year 2000-2050 and k in [1, 100000] days (~274 years), D + k stays well inside the supported DATE range (max ~5874897 AD), so date + integer never overflows and this is_overflow skip branch is unreachable. The comment "A large k can legally overflow the date range" does not hold for these bounds. Either make overflow reachable or remove the dead branch and its comment. [low confidence]
📄 src/test/prop/prop_sortlimit.c (L32-L32)
Comment says "~15% NULL", but the code draws NULL when pg_prop_draw_int(tc, 0, 6) == 0. That range is inclusive [0,6] (7 outcomes), so the NULL probability is 1/7 ≈ 14.3%, not 15%. The header comment above ("~15% NULL") carries the same drift. Minor, but worth correcting so the stated figure matches the code.
💡 Suggested change
Before:
/* v domain: small ints -5..5, ~15% NULL. */
After:
/* v domain: small ints -5..5, ~1-in-7 (14%) NULL. */
📄 src/test/prop/prop_decimalfmt.c (L245-L262)
normalize_text is a byte-for-byte duplicate of text_roundtrip: identical draw_magnitude(tc, &x) input and identical SQL "SELECT (('%s'::numeric)::text)::numeric = '%s'::numeric". It exercises nothing that text_roundtrip doesn't already cover, so it is dead/redundant test code. Either drop one of the two properties, or make normalize_text actually test the distinct invariant its comment claims (e.g. assert scale((X::text)::numeric) = scale(X) so the "no trailing-zero noise beyond the value's scale" statement is really verified).
📄 src/test/prop/prop_numfmt.c (L276-L279)
This property cannot detect the silent-wrap bug it names as its target. The SQL compares (A::int8 + B::int8) against the oracle ((A::numeric + B::numeric)::int8) in a single statement. A silent int8 wrap only occurs when the true sum is outside int8 range -- but in exactly those cases the oracle's trailing ::int8 cast raises "out of range", so the whole statement errors, is_overflow(err) is true, and the case is skipped via pg_prop_assume(tc, false). Thus a hypothetical int8 add that wrapped instead of erroring would be skipped, never reported. The equality branch (result != "t") is only reachable when the true sum fits int8, where no wrap is possible. To actually catch a wrap, drop the ::int8 on the oracle and compare the int8 result cast up to numeric against the numeric sum (e.g. (A::int8 + B::int8)::numeric = A::numeric + B::numeric), reserving the overflow-skip for the LHS only. The same flaw applies to int8_mul_matches_numeric_or_overflows.
📄 src/test/prop/prop_floatspecial.c (L85-L85)
The property name contradicts what it asserts. The body requires 'NaN'::float8 = 'NaN'::float8 to be TRUE (and the comment above correctly states "NaN equals NaN under PostgreSQL's total-order semantics"), but the name says nan_not_equal_itself. This name will mislead anyone scanning --list output or a failure report. Rename to reflect the asserted invariant, e.g. nan_equals_itself_and_is_not_distinct.
💡 Suggested change
Before:
PG_PROP(nan_not_equal_itself_but_is_not_distinct)
After:
PG_PROP(nan_equals_itself_and_is_not_distinct)
📄 src/test/prop/prop_floatspecial.c (L46-L46)
frac is drawn in 0..999 but printed with %d, so leading zeros are dropped: frac=7 and frac=70 both come from distinct draws yet frac=7 yields .7 while a draw of 7 vs 70 produces magnitudes .7 vs .70, and e.g. an intended .007 is unreachable (prints .7). The fractional magnitude therefore does not match the drawn integer, skewing the value distribution away from what a reader expects. This does not break any invariant (every emitted literal is still a valid, well-defined decimal), but if even fractional coverage matters, format with a fixed width, e.g. appendStringInfo(buf, "(%d.%03d)::float8", whole, frac);.
💡 Suggested change
Before:
appendStringInfo(buf, "(%d.%d)::float8", whole, frac);
After:
appendStringInfo(buf, "(%d.%03d)::float8", whole, frac);
📄 src/test/prop/prop_sortlimit.c (L263-L263)
The property name sort_stable_cardinality and the comment "ORDER BY preserves the multiset of rows" claim stability, but the oracle only compares per-value count(*) groupings between the ordered subquery and the base table. That checks multiset equality (cardinality per value), not sort stability (preservation of the relative order of equal keys). The name overstates what is verified; consider renaming to e.g. sort_preserves_multiset or dropping "stable" so the identifier matches the invariant actually tested. Low confidence / low severity.
📄 src/test/prop/prop_hashfn.c (L236-L236)
The property name contradicts what the test does. The name advertises hashint8/hashint4 consistency on small values, but the body only checks hashint8(n) = hashint8(n) (self-determinism over the full int64 range), and the comment just above explicitly states int4 and int8 "are not expected to agree." This name is surfaced verbatim in the property registry (PG_PROP stringifies the identifier) and via --list, so it misleads maintainers into believing cross-function consistency is covered when it is not. Rename to reflect the actual property, e.g. hashint8_deterministic.
(high confidence)
💡 Suggested change
Before:
PG_PROP(hashint8_consistent_with_int4_on_small)
After:
PG_PROP(hashint8_deterministic)
📄 src/test/prop/prop_hashfns2.c (L238-L238)
Redundant property. hash_distributes_nothing builds the byte-for-byte identical query to hashtext_equal_inputs above (SELECT hashtext(S) = hashtext(S) over the same [abc] alphabet), so it adds no distinct coverage -- a regression that broke one would break both. It is also effectively identical to prop_hashfn.c's hashtext_deterministic. Consider dropping it (or folding it into a single determinism property) to reduce reviewer/maintenance burden.
(moderate confidence)
📄 src/test/prop/prop_hashfn.c (L62-L62)
append_sql_literal and require_true (and draw_text) are copy-pasted verbatim here and in ~27 other prop_*.c files (the comment even says "Modelled on prop_like.c" / "prop_numeric.c"). pg_prop.h exposes no SQL-literal-quoting or scalar-boolean-assert helper, so each file re-pastes identical code. This duplication belongs in the shared harness (e.g. pg_prop_append_sql_literal() and a pg_prop_require_true() on top of pg_prop_sql_scalar), so a fix to the quoting/assert logic lands in one place rather than ~27. Not blocking, but worth consolidating before this lands.
(moderate confidence)
📄 src/test/prop/prop_float.c (L55-L61)
The NaN/Inf branches here are dead code, and the surrounding claims that the generator "yields the special values too (NaN, +/-Inf, ...)" and that "those are the point" are false.
pg_prop_draw_float() (pg_prop.c) hard-disables both specials: it passes allow_nan as isinf(min) || isinf(max) ? false : false (always false) and allow_infinity as a literal false. So with any bounds -- including the -INFINITY, INFINITY used here -- the draw can never return NaN or an infinity. Consequently isnan(v) and isinf(v) are never true, lines 57-61 are unreachable, and the elaborate CASE WHEN %s = 'NaN'::float8 ... NaN special-casing in text_roundtrip_f8/f4, order_consistent_with_double, negation_involution and abs_nonnegative never exercises its NaN path.
The headline property of this area (float8 -> text -> float8 identity "including subnormals, +/-0, +/-Inf and NaN") is therefore not actually tested for the specials via the random draw; only the hand-written fixed-literal properties cover any of them. Either fix the generator so it can emit specials when the range is unbounded, or correct the comments/remove the dead branches so the file does not overstate its coverage.
📄 src/test/prop/prop_hstore2.c (L8-L8)
The header claims this file exercises avals/skeys/svals, but no property in the file uses any of them (only akeys, delete, slice, ||/hstore, hstore_to_json, defined/?, each, and @> are tested). Comments must describe what the code does now. Either add properties covering avals/skeys/svals or drop them from the summary to keep the comment accurate. (low confidence this was intentional over-claiming vs. dropped properties.)
💡 Suggested change
Before:
* akeys/avals/skeys/svals, each() as a set-returning function, hstore_to_json,
After:
* akeys, each() as a set-returning function, hstore_to_json,
📄 src/test/prop/prop_hstore.c (L334-L335)
vals[6][2] is sized exactly to the inclusive upper bound of pg_prop_draw_int(tc, 1, 6) (max nkeys = 6, so the writes to vals[0..5] in draw_hstore are in bounds today). However the 6 is duplicated as an implicit magic number in two places with no shared constant or bounds check in draw_hstore, which takes char (*vals)[2] with no length. If the draw bound is later widened without resizing vals, draw_hstore silently overflows the stack buffer. Consider a shared constant (e.g. #define MAX_HSTORE_KEYS 6) used for both the array size and the draw bound to keep them coupled.
📄 src/test/prop/prop_hashfn.c (L219-L222)
hashint4_deterministic will fail spuriously on a correct backend at the INT32 boundary. hashint4 only accepts int4 (per pg_proc.dat), but writing -2147483648 as a bare literal does not produce an int4 constant: the lexer reads the magnitude 2147483648, which overflows int4 and is typed as bigint, so the unary minus yields an int8 value. Function resolution then looks for hashint4(bigint), which does not exist (int8->int4 is assignment-only, not an implicit cast), and the SELECT errors out. Since pg_prop_draw_int favours boundary values, INT32_MIN is drawn and require_true reports the SQL error as a failure. This is exactly the problem the sibling append_int8_literal helper was written to avoid for int8; the same fix is needed here, e.g. cast the literal explicitly to int4.
💡 Suggested change
Before:
initStringInfo(&q);
appendStringInfo(&q,
"SELECT hashint4(%lld) = hashint4(%lld)",
(long long) n, (long long) n);
After:
initStringInfo(&q);
appendStringInfo(&q,
"SELECT hashint4((%lld)::int4) = hashint4((%lld)::int4)",
(long long) n, (long long) n);
📄 src/test/prop/prop_hashfns2.c (L207-L230)
hash_int_consistency is a near-duplicate of hashint_deterministic above and adds no distinct coverage. Both draw n over INT32_MIN..INT32_MAX and compare hashtext((n)::int4::text) against a second rendering of the same expression -- the only difference is the extra parentheses in ((n)::int4)::text, which produce byte-identical SQL semantics and identical rendered text. A regression breaking one would break both. Consider dropping this property (same redundancy class as hash_distributes_nothing).
📄 src/test/prop/prop_hstore2.c (L109-L109)
The vals out-parameter of draw_hstore (and the per-key capture block below) is dead code here: all eight call sites in this file pass NULL, so vals[i][0]/[1] is never written. Unlike prop_hstore.c (whose get_matches_arrow actually consumes vals), nothing in prop_hstore2.c reads back a stored value. Drop the vals parameter and the capture block (and the paragraph of comment describing it) to keep the helper minimal, or add a property that uses it. [moderate confidence]
📄 src/test/prop/prop_indexscan.c (L97-L101)
The differential oracle never verifies the intended plan was chosen. enable_* GUCs are soft cost penalties, not hard forces, and on a 0-40 row table the planner is free to pick a seqscan for all three presets. If every preset silently runs the same plan (e.g. seqscan), run_three_scan() compares identical outputs and passes tautologically -- it would still pass with an index-scan or bitmap-scan bug present, which violates the "a test that passes with the feature reverted is worthless" rule. Add an EXPLAIN assertion (e.g. via pg_prop_sql_rows on EXPLAIN (COSTS OFF) and checking the node type) that each preset actually produced the intended scan node, or pg_prop_assume() it and discard the case otherwise. Confidence: high. (Same gap applies to prop_indexdiff.c require_index_eq_seqscan() and prop_partprune.c pruning_preserves_rows().)
📄 src/test/prop/prop_indexscan.c (L117-L119)
The return value of pg_prop_sql_exec() for the GUC presets and RESET ALL is ignored, and &err is reused across all three loop iterations plus the RESET. pg_prop_sql_exec() pg_strdup()s *errmsg on failure (pg_prop_sql.c:188), so a failed SET both (a) silently runs the subsequent query under the wrong planner settings -- making the oracle meaningless and able to false-pass -- and (b) overwrites the previous err pointer, leaking it. Here the SET failure is checked but RESET ALL is not; and err from a successful path is never freed before being reused. Check the RESET return too, and free/reset err between calls. Confidence: moderate.
📄 src/test/prop/prop_indexdiff.c (L55-L60)
connect_with_ext() calls pg_prop_fail() (a noreturn longjmp) if btree_gin/btree_gist are not installable, hard-failing the gin_equality/gist_range properties instead of skipping. The build wiring does not guarantee these contrib modules are present: neither src/test/prop/Makefile nor meson.build declares EXTRA_INSTALL for contrib/btree_gin or contrib/btree_gist, and t/002_sql_props.pl neither installs them nor skips when absent. Under make check the temp install will not contain these extensions, so these properties fail rather than skip cleanly -- violating the "skip cleanly when prerequisites are missing" rule. Either add the contrib modules to EXTRA_INSTALL (build wiring) and/or treat a missing extension as pg_prop_assume(false)/skip rather than pg_prop_fail. Confidence: high.
📄 src/test/prop/prop_indexdiff.c (L150-L155)
The return value of pg_prop_sql_exec() for the forced-path GUC sets and the final RESET ALL is ignored, and &err is reused across all three calls. pg_prop_sql_exec() pg_strdup()s *errmsg on failure, so a silently-failed SET leaves the subsequent query running under unintended planner settings -- defeating the index-vs-seqscan oracle and producing a meaningless pass -- while also overwriting/leaking the prior err. Check the return of each SET/RESET and free err between calls. Confidence: moderate.
📄 src/test/prop/prop_indexdiff.c (L303-L304)
Comment refers to const2, which is not a variable in this function; the drawn value is r and the squared value is sq. Fix the comment to name sq to avoid drift. (High confidence, low impact.)
💡 Suggested change
Before:
* same expression (k*k = <square>), must match a seqscan. const2 is a square
* in [0, 25] so it is a value the expression can actually take.
After:
* same expression (k*k = <square>), must match a seqscan. sq is a square
* in [0, 25] so it is a value the expression can actually take.
📄 src/test/prop/prop_isn.c (L246-L247)
This property is a no-op: it does not exercise hyphen handling at all. It draws v from valid_ean13[], whose entries are all hyphen-free 13-digit strings ("9780123456786", ...). So replace('%s','-','') strips nothing, and the generated SQL reduces to '<v>'::ean13 = '<v>'::ean13, which is trivially true. The test would still pass if the parser's hyphen handling were completely broken -- i.e. it passes with the behavior-under-test reverted, so it catches no regression.
To actually test "hyphens are cosmetic", one side must be hyphenated. Either add a parallel pool of hyphenated spellings of the same values and compare '<hyphenated>'::ean13 = '<bare>'::ean13, or derive the hyphenated form from the canonical output and strip it, e.g. compare ('<v>'::ean13)::text::ean13 = replace(('<v>'::ean13)::text, '-', '')::ean13 so the hyphens actually present in the EAN13 canonical output drive the stripping branch. (confidence: high)
📄 src/test/prop/prop_jsonapi.c (L175-L176)
Style/consistency nit (low): other property modules in this suite format int64 with INT64_FORMAT (e.g. prop_jsonbroundtrip.c, prop_cash.c), matching the project convention of never using %ld/%lld for int64. The explicit (long long) cast here is portable and correct, so this is only a consistency point, but INT64_FORMAT would align with the rest of the tree and the sibling tests. Same applies to the %lld pair in incremental_chunk_size_invariant below.
💡 Suggested change
Before:
"(chunk_size=%lld input=\"%s\")",
(int) one, (int) inc, (long long) chunk_size, shown);
After:
"(chunk_size=" INT64_FORMAT " input=\"%s\")",
(int) one, (int) inc, chunk_size, shown);
📄 src/test/prop/prop_jsonbroundtrip.c (L68-L68)
This comment references draw_jsonb_scalar_arg, but no such function exists in this file. The parenthesization it describes is actually done inline in the scalar_roundtrips property (to_jsonb((...))). Fix the dangling reference so the comment matches the code it documents.
💡 Suggested change
Before:
* '-' never misparses -- see draw_jsonb_scalar_arg.
After:
* '-' never misparses -- see scalar_roundtrips.
📄 src/test/prop/prop_xmlfmt.c (L212-L212)
The = 2 oracle assumes the serialized element always carries two raw < tag delimiters. But draw_content draws 0..10 characters, so empty content is reachable, and PostgreSQL may serialize xmlelement(name N, '') as the self-closing form <N/> (one raw <, oracle expects 2) rather than <N></N>. The sibling xmlelement_empty_content property in this same file deliberately accepts BOTH <N/> and <N></N> precisely because the empty-content serialization is not fixed, so this property is inconsistent with it and can report a spurious failure on an empty draw. Either require non-empty content here (e.g. draw via pg_prop_draw_int(tc, 1, 10) or pg_prop_assume(tc, c.len > 0)), or make the oracle tolerate the self-closing form. (moderate confidence)
📄 src/test/prop/prop_jsoneach.c (L195-L198)
pg_prop_assume(tc, false) always transfers control via siglongjmp (see pg_prop.c:363-367), so it never returns. The pfree(j.data) / pg_prop_disconnect(conn) / return that follow are dead code and misleadingly suggest cleanup happens on this path. (The connection is not actually leaked: run_one_case() calls pg_prop_sql_cleanup() after every case.) Drop the unreachable lines and just call pg_prop_assume(tc, false);. (high confidence)
📄 src/test/prop/prop_jsonconstruct.c (L128-L137)
Style/consistency (low): the rest of this JSON suite draws int4-range values with the named constants INT32_MIN/INT32_MAX (e.g. prop_jsonbnum.c, prop_jsonbtype.c, and the sibling prop_jsonvsjsonb.c:118). Here the raw literals -2147483648, 2147483647 are used in all three properties (lines 128, 177, 230). Besides the inconsistency, -2147483648 is parsed as negation of 2147483648, a literal that does not fit in int; the named constant is clearer and matches the convention. Suggest INT32_MIN, INT32_MAX.
💡 Suggested change
Before:
int n = (int) pg_prop_draw_int(tc, -2147483648, 2147483647);
StringInfoData q;
conn = pg_prop_connect(&err);
if (conn == NULL)
pg_prop_fail(tc, "connect failed: %s", err ? err : "");
initStringInfo(&q);
appendStringInfo(&q,
"SELECT jsonb_build_object('k', (%d)::int) -> 'k' "
After:
int n = (int) pg_prop_draw_int(tc, INT32_MIN, INT32_MAX);
StringInfoData q;
conn = pg_prop_connect(&err);
if (conn == NULL)
pg_prop_fail(tc, "connect failed: %s", err ? err : "");
initStringInfo(&q);
appendStringInfo(&q,
"SELECT jsonb_build_object('k', (%d)::int) -> 'k' "
📄 src/test/prop/prop_jsonbexists.c (L11-L12)
This header comment is inaccurate: it claims "both quantifiers over an empty key list are false," but only ?| (any) is false over an empty key list. ?& (all) over an empty key list is vacuously true in PostgreSQL ('{"a":1}'::jsonb ?& ARRAY[]::text[] returns t). Only the ?| empty case is actually tested (empty_any_false), so there is no test failure, but the comment mis-describes ?&. Fix the wording so it only asserts the ?|-empty-is-false fact (or spell out that ?& over an empty list is true).
💡 Suggested change
Before:
* false, "any" needs one hit, "all" needs every hit, and both quantifiers over
* an empty key list are false. Every property is a SELECT of a boolean that
After:
* false, "any" needs one hit, "all" needs every hit, and "any" over an empty
* key list is false (while "all" over an empty key list is vacuously true).
* Every property is a SELECT of a boolean that
📄 src/test/prop/prop_jsonpathmath.c (L54-L57)
The arr_out parameter is dead output: every property builds its array oracle in SQL (via jsonb_array_elements/jsonb_array_length on J->'arr'), and none of size_method/type_method/arithmetic_in_path/filter_gt/exists_filter/abs_method/double_method/min_filter_count_le ever reads arr.data -- each caller only initStringInfos it and pfrees it. The parameter, the two appendStringInfoString(arr_out, ...)/appendStringInfo(arr_out, "%d", e) calls below, and the per-caller init/pfree churn are unused plumbing. Drop arr_out from draw_doc and remove the arr StringInfo from the eight callers (YAGNI).
📄 src/test/prop/prop_jsonpathmath.c (L66-L72)
These arr_out appends feed a buffer no property consumes (see the draw_doc signature). They can be removed along with the parameter.
📄 src/test/prop/prop_xmlfmt.c (L6-L7)
This entire area has a hard runtime dependency on libxml, but nothing gates it on build configuration. On a --without-libxml build every XML function (xmlelement, xmlconcat, xmlagg, IS DOCUMENT) raises ERROR: unsupported XML feature (NO_XML_SUPPORT() in src/backend/utils/adt/xml.c:234). require_true turns that SQL error into a hard pg_prop_fail, so the whole xmlfmt area fails rather than skips. The core regression suite handles this with alternative expected files (xml_1.out, xmlmap_1.out); this harness has no equivalent, so the test is not portable and violates the "skip cleanly when prerequisites are missing" rule. The xmlfmt area must be conditioned on libxml availability (e.g. gate it in t/002_sql_props.pl / the build, or have the area detect missing XML support and skip the cases).
(high confidence)
📄 src/test/prop/prop_xmlfmt.c (L228-L228)
Property name xmlconcat_length_additive_ish is misleading: the body checks a textual equality (xmlconcat(E1,E2)::text = E1::text || E2::text), not any length-additivity property. This name is what appears in the example database as xmlfmt/xmlconcat_length_additive_ish. Rename to describe what it asserts, e.g. xmlconcat_concatenates.
(low confidence)
💡 Suggested change
Before:
PG_PROP(xmlconcat_length_additive_ish)
After:
PG_PROP(xmlconcat_concatenates)
📄 src/test/prop/prop_regexmatch.c (L251-L253)
Dead guard / comment-code mismatch. S is drawn with draw_letters(tc, &s, "abc", 1, 12) (minimum length 1), so length(S) = 0 is never true and the WHEN length(...) = 0 THEN true branch is unreachable. The property comment above justifies the guard with "we keep the explicit guard so the oracle is obviously sound even on the empty case" -- but the empty case cannot occur here. For a minimal diff, drop the CASE wrapper and emit the bare equality (S ~ (c || '$')) = (right(S, 1) = c), and adjust the comment accordingly. (low confidence it's harmful; it is correct, just needless.)
📄 src/test/prop/prop_mvcc.c (L80-L88)
scalar_int() here parses with atol() into a long and the failure messages print the result with %ld. These counts/values originate from count(*) / sum(), which are bigint (int64) in the backend. long is 32-bit on LLP64 platforms (Windows/MSVC), so this is not width-portable, and %ld is the wrong specifier for an int64 value per the project's portability gate. Every sibling file that parses a scalar count (e.g. prop_mergecmd.c, prop_dmlcte.c) uses atoll()/long long/%lld. Match that convention here (switch the helper's return type and all five %ld call sites) for consistency and portability. The runtime values are tiny (<= 20 rows), so there is no actual truncation today; this is a convention/portability consistency fix.
💡 Suggested change
Before:
long val;
res = pg_prop_sql_scalar(conn, sql, &failed, &err);
if (failed)
pg_prop_fail(tc, "SQL error for: %s\n error: %s",
sql, err ? err : "(unknown)");
if (res == NULL)
pg_prop_fail(tc, "unexpected NULL result for: %s", sql);
val = atol(res);
After:
long long val;
res = pg_prop_sql_scalar(conn, sql, &failed, &err);
if (failed)
pg_prop_fail(tc, "SQL error for: %s\n error: %s",
sql, err ? err : "(unknown)");
if (res == NULL)
pg_prop_fail(tc, "unexpected NULL result for: %s", sql);
val = atoll(res);
📄 src/test/prop/prop_regexpreplace.c (L309-L311)
Comment/code mismatch (high confidence). This comment claims "the CASE is left in only to document the non-empty reasoning while remaining true at length 0," but the generated SQL below is just regexp_replace(S, '^.*$', 'Y') = 'Y' with no CASE expression. The comment drifted from the code; drop the CASE wording (the property itself is correct, including for empty S).
💡 Suggested change
Before:
* For an empty S the single empty match still yields 'Y', so the identity
* holds for every drawn S; the CASE is left in only to document the
* non-empty reasoning while remaining true at length 0.
After:
* For an empty S the single empty match still yields 'Y', so the identity
* holds for every drawn S.
📄 src/test/prop/prop_mvcc.c (L321-L327)
This property cannot catch an MVCC regression. With a single session and no concurrent writer, reading count(*) twice is identical under READ COMMITTED and REPEATABLE READ; the comment itself admits "Single-session this is trivially true." The test would still pass with the RR snapshot machinery it claims to exercise reverted, so it provides no coverage of the stated invariant. Snapshot stability under a concurrent writer belongs in src/test/isolation (a .spec with permutations), which can drive two sessions deterministically. Consider dropping this single-session property or moving the real check to an isolation test.
📄 src/test/prop/prop_mvcc.c (L333-L335)
Two problems. (1) The comment references "Shape A" with no corresponding Shape B elsewhere in the file -- a drifted/incomplete comment fragment. (2) More importantly, this property has nothing to do with MVCC snapshot/visibility (the stated subject of this file): that count(*) is invariant under ORDER BY is a trivial SQL identity that would hold regardless of any MVCC behavior and cannot catch a visibility regression. It is dead coverage here. Either remove it or relocate it to a general query-equivalence area.
📄 src/test/prop/prop_numericdiv.c (L344-L346)
This comment misdescribes NUMERIC rounding. PostgreSQL's round_var() (src/backend/utils/adt/numeric.c) rounds half away from zero (digits[ndigits] >= HALF_NBASE rounds up), not "truncates" and not "round half to even." The phrase "truncates toward even rounding" is self-contradictory and inaccurate. The property itself is still valid (round-half-away-from-zero is symmetric about zero, so negating the dividend negates the rounded quotient), but the justification should state that correctly.
💡 Suggested change
Before:
* numeric division truncates toward even rounding of the display scale
* symmetrically about zero, so negating the dividend negates the rounded
* quotient; comparing at scale 6 absorbs any last-digit rounding.
After:
* numeric rounds half away from zero, which is symmetric about zero, so
* negating the dividend negates the rounded quotient; comparing at scale 6
* absorbs any last-digit rounding.
📄 src/test/prop/prop_numericdiv.c (L80-L84)
require_true() (and the draw_numeric helpers) are copy-pasted verbatim across ~200 prop_*.c files in this directory. For a pgsql-hackers submission this violates the DRY expectation: a single require_true helper (SQL-error / NULL / non-"t" check) belongs in the shared harness (pg_prop.h / pg_prop_sql.c), with each area reusing it. Consider promoting it to shared infrastructure to avoid 200 identical copies that must be kept in sync.
📄 src/test/prop/prop_pglsn.c (L41-L42)
is_overflow matching the bare substring "cannot" is too broad and defeats the test's purpose. The only legitimate errors pg_lsn arithmetic can raise here are pg_lsn out of range (ERRCODE_INVALID_PARAMETER_VALUE from numeric_pg_lsn) and the NaN/infinity guards cannot add NaN to pg_lsn / cannot subtract NaN from pg_lsn / cannot convert NaN|infinity to pg_lsn. But draw_lsn only ever emits concrete hex LSN literals and the arithmetic operand d is a bounded numeric, so the NaN/infinity guards can never fire in these properties. Meanwhile the loose cannot match will silently swallow unrelated server errors whose text happens to contain "cannot" and discard the case via pg_prop_assume(tc, false) -- masking exactly the wrong-result bugs this suite is meant to catch (confidence: high). Tighten the match to the exact legitimate wordings, or just to out of range since NaN inputs are not generated here.
💡 Suggested change
Before:
return strstr(errmsg, "out of range") != NULL ||
strstr(errmsg, "cannot") != NULL;
After:
return strstr(errmsg, "out of range") != NULL;
📄 src/test/prop/prop_pglsn.c (L53-L63)
The raw out-parameter is dead scaffolding: all nine draw_lsn() call sites pass NULL, so the documented "C-side oracle" (*raw) is never consumed anywhere in the file. Per YAGNI, drop the unused parameter (and the hi<<32 | lo computation and the paragraph documenting it) until an actual caller needs it (confidence: high).
💡 Suggested change
Before:
static void
draw_lsn(pg_prop_tc *tc, StringInfo buf, uint64 *raw)
{
uint32 hi = (uint32) pg_prop_draw_int(tc, 0, 4294967295);
uint32 lo = (uint32) pg_prop_draw_int(tc, 0, 4294967295);
appendStringInfo(buf, "'%X/%X'::pg_lsn", hi, lo);
if (raw)
*raw = ((uint64) hi << 32) | (uint64) lo;
}
After:
static void
draw_lsn(pg_prop_tc *tc, StringInfo buf)
{
uint32 hi = (uint32) pg_prop_draw_int(tc, 0, 4294967295);
uint32 lo = (uint32) pg_prop_draw_int(tc, 0, 4294967295);
appendStringInfo(buf, "'%X/%X'::pg_lsn", hi, lo);
}
📄 src/test/prop/prop_trimfns.c (L48-L51)
pg_prop_draw_from returns pg_malloc-allocated memory (see pg_prop.c:draw_string, which uses pg_malloc). The directly analogous sibling helpers free this with pg_free(s) (e.g. prop_textlength.c:draw_str_literal, prop_collate.c:draw_str_literal). Here draw_body uses plain free(s). In the frontend (postgres_fe.h) build these route to the same libc free, so this is not a correctness bug, but it diverges from the established convention in this test suite. Use pg_free(s) for consistency with the pg_malloc/pg_free pairing used throughout src/test/prop. The same applies to draw_mixed below.
💡 Suggested change
Before:
appendStringInfoChar(buf, '\'');
appendBinaryStringInfo(buf, s, (int) len);
appendStringInfoChar(buf, '\'');
free(s);
After:
appendStringInfoChar(buf, '\'');
appendBinaryStringInfo(buf, s, (int) len);
appendStringInfoChar(buf, '\'');
pg_free(s);
📄 src/test/prop/prop_padtrim.c (L363-L364)
The comment is inaccurate (comment-code drift). It states that "n is drawn in SQL as a fraction of length('') via a drawn offset so the call and oracle see the same length," but the code draws n directly in C via strlen(s.data) and pg_prop_draw_int(tc, 0, slen) and interpolates the literal integer into both the lpad(..., %d, ...) and left(..., %d) calls. There is no SQL length('<S>') expression, no fraction, and no drawn offset. Reword the comment to describe what the code actually does.
💡 Suggested change
Before:
* for n in [0, length(S)]. n is drawn in SQL as a fraction of length('<S>')
* via a drawn offset so the call and oracle see the same length; left(S, n)
After:
* for n in [0, length(S)]. n is drawn in C in [0, strlen(S)] and the same
* literal is used in both calls so the call and oracle see the same length;
* left(S, n)
📄 src/test/prop/prop_numericalg.c (L227-L229)
This formulation of the trunc-division identity uses trunc(a/b) rather than the exact integer quotient div(a,b). / first rounds a/b to a display scale, so trunc(a/b) can differ from the true truncated quotient by a full integer at a boundary (quotient rounded up across an integer), in which case the reconstruction is off by ~b (up to 20) and the 1e-9 tolerance would not absorb it. More importantly, the stated target is the div/mod/gcd/lcm paths, and sibling file prop_numeric.c's div_mod_identity already expresses this exact identity correctly with div(X,Y)*Y + mod(X,Y) = X (no tolerance). Using trunc(a/b) here is a strictly weaker, division-rounding-dependent check that can mask a div/mod carry bug while risking a false positive. Prefer div(a, b) and drop the tolerance.
💡 Suggested change
Before:
"SELECT abs(%s - (trunc(%s / (%d)) * (%d) "
"+ mod(%s, (%d)))) < 1e-9::numeric",
a.data, a.data, b, b, a.data, b);
After:
"SELECT (div(%s, (%d)) * (%d) + mod(%s, (%d))) = %s",
a.data, b, b, a.data, b, a.data);
📄 src/test/prop/prop_pglsn.c (L161-L175)
This overflow-skip branch is dead code and its justifying comment is factually wrong. pg_lsn + numeric (pg_lsn_pli) computes the whole sum as numeric and only range-checks the final result via numeric_pg_lsn. In L1 + (L2 - L1) the final result is always exactly L2, which is a valid LSN in [0, 2^64-1] by construction, so numeric_pg_lsn can never raise "out of range" here. The intermediate (L2 - L1) being negative does not matter -- it is only an operand to numeric_add, never clamped. Thus is_overflow(err) can never be true in this property, and the comment "the add can carry L1 below 0 -- that underflow is a legitimate out-of-range error" (plus the matching sentence in the property's header comment) describes an impossible condition. Any error reaching this branch is a real bug being silently skipped. Drop the is_overflow branch here and treat every error as a failure (as require_true does for the other properties). Confidence: high.
📄 src/test/prop/prop_range.c (L147-L149)
Minor leak on the connect-failure path: pg_prop_connect sets *err via pg_strdup (owned by the caller), but pg_prop_fail is pg_noreturn and longjmps away, so this err is never freed. Negligible in practice (a connect failure aborts the run), but worth freeing before failing, or passing NULL since the message is already incorporated. This same pattern is repeated in every PG_PROP body in all three files.
📄 src/test/prop/prop_record.c (L164-L170)
The alphabet includes a backslash (\), and the SQL literal builder here only doubles single quotes, not backslashes. The literal is only "always well-formed" when standard_conforming_strings is on. pg_prop_connect() sets just client_min_messages and does not force standard_conforming_strings = on; if a server/role/database default turns it off, a drawn backslash is treated as an escape introducer inside the '...' literal, silently altering the field value and defeating the round-trip (or triggering a WARNING). The default has been on since 9.1 so this passes in practice, but the comment's "always well-formed" claim is overstated. Consider using the E'' form with backslash doubling, dollar-quoting, or forcing standard_conforming_strings=on on the connection to make it robust regardless of session defaults. (low confidence)
📄 src/test/prop/prop_rangeops.c (L62-L65)
require_true is copied verbatim here and across 200+ prop_*.c files (several of which even carry a "Copied from prop_numeric.c" comment). The harness header pg_prop.h exposes pg_prop_sql_scalar but no boolean-assert helper; this function is the natural thing to lift into pg_prop_sql.c/pg_prop.h so every Tier-B area shares one implementation. Flagging here since prop_rangeops.c is one of the duplicates; the fix is framework-wide.
📄 src/test/prop/prop_rangeops.c (L41-L42)
The element domain here is -10..10 and bounds are emitted as int4range((%d), (%d)) with redundant parentheses, whereas the sibling files prop_range.c and prop_multirange.c use -8..8 and plain int4range(%d, %d). Harmless, but aligning the domain and dropping the parens would keep counterexamples consistent and reproducible across these related modules.
📄 src/test/prop/prop_upsert.c (L390-L390)
The property name multi_insert_last_wins contradicts what the test actually asserts. The body expects the statement to FAIL ("ON CONFLICT DO UPDATE command cannot affect row a second time"), which is the opposite of "last wins" semantics. Both the identifier and the inline comment /* a new key, so the two rows conflict with each other */ combined with the "last wins" name mislead a reader about the invariant. Rename to something that states the real property, e.g. multi_insert_same_key_errors. (Confidence: high; the file-header comment already correctly describes this as an error case, so only the identifier is wrong.)
💡 Suggested change
Before:
PG_PROP(multi_insert_last_wins)
After:
PG_PROP(multi_insert_same_key_errors)
📄 src/test/prop/prop_returning.c (L253-L253)
These two properties (upsert_insert_or_update and upsert_do_nothing) do not test RETURNING at all; they assert ON CONFLICT state transitions (row count unchanged / value becomes 999 / DO NOTHING leaves the row). That duplicates prop_upsert.c's conflict_do_update_sets, no_conflict_inserts, and conflict_do_nothing_keeps, and contradicts this file's own header ("INSERT/UPDATE/DELETE ... RETURNING") and prop_upsert.c's claim that upsert "is distinct from prop_returning (RETURNING row accounting)". Consider dropping the duplicated upsert cases here and keeping ON CONFLICT coverage solely in prop_upsert.c, so this area stays scoped to RETURNING row accounting.
📄 src/test/prop/prop_upsert.c (L41-L42)
This file's scalar_int returns long and parses with atol, while the sibling Tier-B files (prop_returning.c, prop_updatesem.c, prop_mergecmd.c) use long long/atoll. The values here are tiny so there is no practical overflow, but for consistency across the suite (and to avoid the 32-bit long on LLP64/Win64) prefer long long/atoll and %lld here too.
💡 Suggested change
Before:
static long
scalar_int(pg_prop_tc *tc, pg_prop_conn *conn, const char *sql)
After:
static long long
scalar_int(pg_prop_tc *tc, pg_prop_conn *conn, const char *sql)
📄 src/test/prop/prop_substring.c (L242-L242)
This property only asserts two upper bounds (<= L and <= available tail). Both inequalities hold trivially for any implementation that returns no more characters than requested, and would still pass even if substring(S from m for L) returned the empty string unconditionally. Per the suite's own discipline (a test that passes against a broken implementation is worthless), this oracle is too weak to catch a regression in substring length semantics. Consider asserting the exact extracted length, e.g. length(substring(S from m for L)) = greatest(least(L, length(S) - m + 1), 0) with the clamped m, which pins both the lower and upper bound.
📄 src/test/prop/prop_substring.c (L161-L161)
The header comment contradicts the code and the inline comment below. It states "for n in [0, length(S) + 2]" and "n is drawn 0..16", but the code draws pg_prop_draw_int(tc, 1, 16) (i.e. n in [1, 16]), and the inline comment at the draw site explicitly explains that n must be >= 1 because left(s, -0) degenerates to left(s, 0). The header's "[0, ...]" / "0..16" wording is stale and misleads the reader about which values are exercised. Fix the range to start at 1.
💡 Suggested change
Before:
* for n in [0, length(S) + 2]. left(S, -n) returns all but the last n
After:
* for n in [1, length(S) + 2]. left(S, -n) returns all but the last n
📄 src/test/prop/prop_tstzarith.c (L223-L230)
Stale, self-contradictory header comment. This block is titled "4. age_plus_back" and claims the identity TS1 + age(TS2, TS1) = TS2, but the function is age_sign_matches_order and its own inner comment (just below) correctly states age is "not invertible by addition" and that the sound invariant is the sign, which is what the SQL actually tests. The header describes a different (and incorrect) property, mismatches the registered property name, and will mislead readers. Rewrite the header to match the implemented sign invariant.
💡 Suggested change
Before:
* 4. age_plus_back
*
* age(TS2, TS1) is a calendar interval (variable-length months/days) that
* spans from TS1 to TS2. Adding it back recovers TS2:
* TS1 + age(TS2, TS1) = TS2. This holds exactly in UTC because there is no
* DST crossing to make the calendar interval ambiguous.
*/
PG_PROP(age_sign_matches_order)
After:
* 4. age_sign_matches_order
*
* age(TS2, TS1) is a symbolic calendar interval and is not invertible by
* addition, so TS1 + age(TS2, TS1) need not recover TS2. The sound invariant
* is the sign: age(TS2, TS1) is positive, zero, or negative exactly as TS2 is
* after, equal to, or before TS1. Checked in UTC so there is no DST crossing.
*/
PG_PROP(age_sign_matches_order)
📄 src/test/prop/prop_timeops.c (L224-L225)
Both draw_timestamp() calls reuse the same scalar out-params (y, mo, d, h, m, s), so the second draw overwrites the first's components. Harmless here because the components are not consumed after drawing, but draw_timestamp's doc comment explicitly invites building a make_date()/extract() oracle from these components -- a latent footgun if someone later does so for ts1. Consider using separate out-params per timestamp.
📄 src/test/prop/prop_tsrank.c (L352-L352)
Property name/comment mismatch. rank_weights_monotone asserts a default-weights equivalence (2-arg ts_rank equals the explicit-weights form), and the comment itself says it does NOT test monotonicity ("without relying on any subtle monotonicity"). The name misleads anyone selecting it on the command line (pg_prop tsrank rank_weights_monotone). Also the comment says "4-arg ts_rank" twice, but the form built here is the 3-arg ts_rank(weights, vector, query) (pg_proc oid 3704). Rename to e.g. rank_default_weights_equiv and fix "4-arg" -> "3-arg".
💡 Suggested change
Before:
PG_PROP(rank_weights_monotone)
After:
PG_PROP(rank_default_weights_equiv)
📄 src/test/prop/prop_tsvector.c (L140-L146)
Property name overstates what is checked: concat_contains_both only verifies lexemes of the left operand (words of D1 are unnested; D2's words are never checked). The comment itself admits this ("contains every lexeme of its left operand", "|| drops no lexeme from the left side"). Same mismatch class as the already-noted rank_weights_monotone. Either extend the oracle to also check D2's words, or rename to reflect the left-only check (e.g. concat_keeps_left).
📄 src/test/prop/prop_tsvector2.c (L127-L129)
Same name/scope mismatch as prop_tsvector.c: concat_contains_both draws w only from words1 (left operand D1) and checks a single left-side word, so D2 is never exercised. The comment says "keeps the left operand's matches". Rename to reflect the left-only check, or also assert a word drawn from D2 matches the concatenation.
| /* allow_nan */ isinf(min) || isinf(max) ? false : false, | ||
| /* allow_infinity */ false, |
There was a problem hiding this comment.
The allow_nan argument is isinf(min) || isinf(max) ? false : false, which is false for both branches of the ternary. NaN is therefore never generated, regardless of the range, directly contradicting the comment above ("Full doubles including NaN ... only when the range is unbounded"). Float properties silently never exercise NaN inputs - exactly the kind of edge case this harness exists to catch. [high confidence]
| /* allow_nan */ isinf(min) || isinf(max) ? false : false, | |
| /* allow_infinity */ false, | |
| /* allow_nan */ isinf(min) || isinf(max), | |
| /* allow_infinity */ isinf(min) || isinf(max), |
| if (blob) | ||
| printf("%sreplay: PG_PROP_SEED unavailable; rerun with " | ||
| "--blob to reproduce: %s\n", pfx, blob); |
There was a problem hiding this comment.
This instructs the user to "rerun with --blob", but no --blob option exists: long_options has no "blob" entry and the getopt string is "n:s:vqhL". A user following this guidance hits the default: case and gets usage + exit 2. The replay path advertised here is not implemented. Either implement a --blob/replay option or correct the message to describe the actual reproduction mechanism (e.g. -s <seed>). [high confidence]
| case 'n': | ||
| iterations = strtou64(optarg, NULL, 10); | ||
| break; |
There was a problem hiding this comment.
iterations is parsed with strtou64 but never validated. A malformed value (e.g. non-numeric env var or CLI arg) yields 0, and iterations = 0 makes hegel_settings_set_test_cases run zero cases - the property reports PASSED while testing nothing, masking regressions. Reject or default a zero/invalid iteration count. The same lack of errno/endptr checking applies to the PG_PROP_ITERATIONS env parse above. [medium confidence]
| #ifndef pg_attribute_constructor | ||
| #define pg_attribute_constructor __attribute__((constructor)) | ||
| #endif |
There was a problem hiding this comment.
pg_attribute_constructor is not a PostgreSQL-tree macro (it exists nowhere else in src/include); it is invented here and expands to the GCC/Clang-only __attribute__((constructor)), which MSVC does not support. Every prop_*.c that uses PG_PROP() will fail to compile on the Windows/MSVC build gate. If this harness is intentionally not built on MSVC, that gating belongs in the Makefile/meson with a documented note; otherwise a portable constructor shim is required. Separately, the fallback #define sits after both PG_PROP and PG_PROP_HOOK, which is confusing even though macro bodies expand lazily - move it above the first user. [high confidence]
| /* | ||
| * avg(v) = sum(v) / count(v). avg returns numeric; the exact identity divides | ||
| * sum by the same count avg used, so it holds to the last digit (unlike | ||
| * sum = count*avg, which rounds). Empty / all-NULL: avg IS NULL. | ||
| */ | ||
| /* | ||
| * avg(v) is sum(v)/count(v) by definition. avg returns a finite-scale | ||
| * numeric, so sum = count*avg does NOT hold exactly (the multiply reintroduces | ||
| * rounding); the sound exact identity is avg = sum::numeric / count when there | ||
| * is at least one non-NULL value, and avg IS NULL otherwise. | ||
| */ |
There was a problem hiding this comment.
Two stacked block comments sit above avg_equals_sum_over_count, both describing the same property with near-identical wording. The first block is a stale/leftover duplicate (it even repeats the "sum = count*avg rounds" caveat spelled out in the second). Per PostgreSQL comment hygiene, collapse these into a single accurate block.
Confidence: high — verified against the diff; both blocks are consecutive and redundant.
| /* | |
| * avg(v) = sum(v) / count(v). avg returns numeric; the exact identity divides | |
| * sum by the same count avg used, so it holds to the last digit (unlike | |
| * sum = count*avg, which rounds). Empty / all-NULL: avg IS NULL. | |
| */ | |
| /* | |
| * avg(v) is sum(v)/count(v) by definition. avg returns a finite-scale | |
| * numeric, so sum = count*avg does NOT hold exactly (the multiply reintroduces | |
| * rounding); the sound exact identity is avg = sum::numeric / count when there | |
| * is at least one non-NULL value, and avg IS NULL otherwise. | |
| */ | |
| /* | |
| * avg(v) is sum(v)/count(v) by definition. avg returns a finite-scale | |
| * numeric, so sum = count*avg does NOT hold exactly (the multiply reintroduces | |
| * rounding); the sound exact identity is avg = sum::numeric / count when there | |
| * is at least one non-NULL value, and avg IS NULL otherwise. | |
| */ |
| pg_prop_sql_exec(conn, "SET enable_incremental_sort = on", &err); | ||
| on = pg_prop_sql_rows(conn, q, &f1, &err); | ||
| if (f1) | ||
| pg_prop_fail(tc, "incsort-on query failed: %s", err ? err : ""); | ||
|
|
||
| pg_prop_sql_exec(conn, "SET enable_incremental_sort = off", &err); |
There was a problem hiding this comment.
The return value of each GUC-forcing pg_prop_sql_exec is discarded here (and again in index_sort_matches). This is the same silent-false-pass defect confirmed in prop_joinmethods.c/prop_outerjoin.c run_three, but it also applies to this file: if SET enable_incremental_sort ever fails (renamed/removed GUC, or an error leaving the session in a known-bad state), both the on and off runs execute under identical settings, strcmp(on, off) compares equal, and the oracle silently stops exercising the incremental-sort vs full-sort distinction -- a pass that would survive reverting the feature. Check each pg_prop_sql_exec return and pg_prop_fail on failure.
| * Pick one row (the min id) and a value lo <= p <= hi inside it; the half-open | ||
| * int4range(lo, hi) excludes hi, so test p = lo which is always a member. The |
There was a problem hiding this comment.
Stale/contradictory comment. This outer doc block justifies the property with a "half-open int4range(lo, hi) excludes hi, so test p = lo" rationale, but the actual query below (and the inner comment at lines 161-165) uses the inclusive '[]' aggregate range_agg(int4range(lo, hi, '[]')). The half-open explanation describes code that isn't here and will mislead a reader about why the property holds. Rewrite to describe the '[]' inclusive form that is actually used (low confidence it masks a real bug; this is purely a comment-drift defect).
| static void | ||
| require_true(pg_prop_tc *tc, pg_prop_conn *conn, const char *sql) | ||
| { | ||
| bool failed = false; | ||
| char *err = NULL; | ||
| char *res; |
There was a problem hiding this comment.
This require_true() helper is duplicated verbatim across every prop_*.c in this suite (confirmed in ~200 files; several even carry the comment "Copied from prop_numeric.c's require_true"). The same applies to the per-file connect boilerplate and connect_intarray() (duplicated between prop_intarray.c and prop_intarrayext.c). The shared harness header pg_prop.h deliberately exists as the single entry point and already exposes pg_prop_sql_scalar/pg_prop_fail, but no shared boolean-assertion helper. Hoisting require_true (and a connect-or-fail helper) into pg_prop.c/pg_prop.h would remove hundreds of identical copies and is the kind of copy-paste the community will push back on. Confidence: high.
| PG_PROP(union_contains_both) | ||
| { | ||
| pg_prop_conn *conn; | ||
| StringInfoData a, | ||
| b, | ||
| q; | ||
|
|
||
| conn = connect_intarray(tc); |
There was a problem hiding this comment.
The comment and property name call int[] + int[] the "intarray union", but that operator is intarray_push_array (concatenation/append), not set union. intarray's set union is the | operator (_int_union), as correctly used by prop_intarrayext.c. The property is still sound (concatenation does contain both operands), but the terminology misstates intarray semantics and could mislead a reader into thinking + deduplicates. Consider renaming to e.g. concat_contains_both and fixing the comment, or switch to the | operator if a true union check was intended.
| /* | ||
| * A single-element slice arr[i:i] contains exactly the i-th element. A | ||
| * slice is re-based so its own subscript is not i in general; compare the | ||
| * element it unnests to against arr[i] rather than subscripting the slice. | ||
| */ |
There was a problem hiding this comment.
The two comments here contradict each other. The function header asserts (A[i:i])[i] = A[i] (slices preserve subscripts), while this inline comment asserts the slice is "re-based so its own subscript is not i in general". In PostgreSQL array slices retain the original subscript bounds, so the header is correct and this inline comment is factually wrong. The SQL (comparing via unnest) is sound either way, but the contradictory/incorrect comment should be reconciled.
pg_prop.h and sql/query_gen.h are internal to the pg_prop test binary and are only ever compiled by that binary's translation units, which add -Isrc/test/prop (and -Isrc/test/prop/sql) to the include path. headerscheck and cpluspluscheck compile every header standalone, so they cannot find query_gen.h's #include "pg_prop.h", and pg_noreturn (the C23 [[noreturn]] attribute) is not accepted by the C++ front end. Both are expected for a non-public, non-standalone test header, so add them to the skip list alongside the other intentionally-non-standalone headers.
Extend query_gen with qg_build_db(): a small random *database* of 2..N related tables, alongside the existing single-table qg_build_table (which is unchanged, so the existing areas keep working). Tables carry declared primary and foreign keys and correlated data -- a child's FK column is drawn from the parent's actual dense key range (plus a slice of NULLs), parents built before children -- so joins have a real referential structure for the planner to reorder and unnest. The schema generator mixes column types and cardinalities deliberately: low- cardinality int columns (tiny domain, heavy duplication, grouping/hashagg stress), high-cardinality int columns (near unique, index/sort stress), text, numeric, and TOAST-eligible text whose value length is drawn to land below, around, or above the ~2KB out-of-line threshold at a tunable frequency. Two parameter profiles: a small, fast, deterministic default for the committed TAP run, and a wide/deep profile (many wide tables, up to thousands of rows, TOAST pressure) for the opt-in high-iteration bug hunt. A schema- and alias-aware predicate drawer (qg_draw_predicate_on) emits predicates over a specific table inside a multi-table query.
Add sql/prop_query_multi.c, the first area to use qg_build_db. Three oracles over a random FK-related database: TLP over a foreign-key join (the predicate partition reconstructs the join's row multiset), inner-join cardinality is commutative and bounded by the cross product, and a FK equi-join returns exactly the child rows whose FK is non-NULL. All are theorems, so a disagreement is a planner/executor wrong-results bug. Validated: green at 3000 iterations on the default profile and 400 on the deep profile (confirmed firing: up to 11 tables, 22-column tables, and thousands of ~3KB TOAST-stored values per case); full TAP suite green at 247 Tier-B areas. Document the multi-table model in DESIGN.
The to_char_numeric_scale property drew an integer part up to 999999 and checked a format/parse round-trip against round(x, 3). It missed that a fractional part >= .9995 carries into the integer part when rounded, so 999999.9995 rounds to 1000000.000 -- seven integer digits, which overflows the six-position 'FM999999.000' mask and makes to_char emit overflow fill that to_number cannot parse. Cap the drawn integer part at 999998 so the rounded value always fits six integer digits. Surfaced by the full TAP seed stream; property-soundness only, no server bug.
Extend the multi-table generator in three ways for the deep bug hunt: * TOAST storage: emit SET STORAGE per varlena column, cycling PLAIN / MAIN / EXTERNAL / EXTENDED (TOAST-eligible text columns get EXTERNAL or EXTENDED, never PLAIN), so the out-of-line and compressed-TOAST machinery is exercised in all its modes. * Triggers: optionally attach a result-neutral BEFORE INSERT/UPDATE row trigger (a plpgsql function that returns NEW unchanged) to some tables, so the relation carries trigger metadata and the executor runs the trigger-firing path while SELECT results are unaffected -- keeping the SELECT oracles sound. * qg_build_query(): a recursive query builder that follows the foreign-key graph to assemble a random left-deep join tree (inner and left joins) of a configurable depth, projecting a stable k0,k1,... key list and exposing one mutable predicate over the anchor relation, so a metamorphic oracle can vary just that predicate while the surrounding join stays fixed. The deep profile enables triggers and TOAST pressure; the committed TAP run uses the small, fast, deterministic default profile.
Add sql/prop_query_deep.c with five metamorphic oracles over the deep query builder: * deep_tlp -- the mutable predicate partitioned three ways reconstructs the join's rows (TLP over an arbitrary join shape). * deep_norec -- the predicate in WHERE versus wrapped in a CASE optimization barrier must count the same rows. * deep_cte_inline -- the filtered query as an inline subquery, a plain CTE, and a MATERIALIZED CTE must return identical rows (subquery-pullup / CTE-inlining). * deep_const_fold -- ANDing a plan-time-foldable tautology (1=1) and a run-time tautology (col - col = 0) must not change the result (constant folding). * deep_corr_subquery -- a parent-filtered semijoin as IN, as EXISTS, and as a DISTINCT join must agree (subquery unnesting). Validated green at 2000 iterations and under the full TAP seed stream; suite at 248 Tier-B areas. Document the deep builder and oracles in DESIGN.
There was a problem hiding this comment.
🔍 OCR found 144 issue(s).
- 25 inline, 119 in summary (inline capped at 25)
⚠️ 1 warning(s) during review
📄 src/test/prop/prop_aggdistinct.c (L163-L167)
This comment is self-contradictory and inaccurate. count(DISTINCT v) ignores NULL, while SELECT DISTINCT v keeps a NULL row, so count(*) over the distinct projection would be one larger whenever any v is NULL -- the two sides would NOT agree. The SQL is only correct because build_table() here never generates a NULL v (pg_prop_draw_int(tc, 0, 4)). The comment should state that v is never NULL (so the NULL-row caveat does not arise), rather than describe NULL handling that does not apply and claim both sides "agree exactly" in general.
📄 src/test/prop/pg_prop.c (L390-L390)
Same appendStringInfoVA() misuse as in pg_prop_note(): a single call that ignores the return value. If the failure message exceeds the initial buffer, appendStringInfoVA leaves tc->failmsg unmodified (empty) and the counterexample report prints an empty "falsified:" line -- defeating the whole point of the failure message on exactly the long-message cases. Must loop with enlargeStringInfo() per the documented contract.
📄 src/test/prop/pg_prop.c (L502-L502)
current_tc is written here but never read anywhere in the file (stop_test takes tc as a parameter). Dead state with a misleading comment ("for the stop/overrun jump"). Remove the variable and this assignment, or wire it into the mechanism it claims to serve.
📄 src/test/prop/pg_prop.c (L11-L11)
Stale CLI shape in the file header: the harness dispatches on a positional AREA argument (area = argv[optind++]), so the invocation is pg_prop <area> ..., not pg_prop_<area> .... usage() and README already use the correct pg_prop AREA form; fix this header comment to match.
💡 Suggested change
Before:
* pg_prop_<area> [options] [PROPERTY ...]
After:
* pg_prop <area> [options] [PROPERTY ...]
📄 src/test/prop/prop_antisemijoin.c (L86-L89)
These oracle helpers (require_same_rows, and the sibling require_true / run_two / run_three in the other new prop_*.c files) are copy-pasted verbatim across ~40 new files in this suite, all built on the shared pg_prop_sql_rows/pg_prop_sql_scalar/pg_prop_sql_exec primitives already in pg_prop.h. This is a significant DRY violation for a test harness: a fix to the oracle (e.g. the error/alloc handling) would have to be applied in dozens of places. Consider lifting the common differential/boolean oracles into pg_prop.h/pg_prop_sql.c (next to the primitives they wrap) and having the area files call them. (maintainability, moderate confidence — the duplication is confirmed by grep across src/test/prop.)
📄 src/test/prop/prop_array.c (L77-L82)
The require_true() helper is copy-pasted verbatim into this file and 200+ other prop_*.c files in this suite (the comment in prop_arraynull.c/prop_arrayops.c/etc. even says "Copied from prop_numeric.c"). Since the Tier-B contract already lives in the shared harness (pg_prop.h / pg_prop_sql.c), consider hoisting this boolean-assertion primitive (e.g. pg_prop_sql_require_true(tc, conn, sql)) into the harness and dropping the per-file copies. This is a suite-wide concern, not specific to this file; flagging once here to avoid repeating it across the group.
📄 src/test/prop/prop_intarrayext.c (L38-L38)
Minor wording imprecision: pg_prop_draw_int(tc, 0, 9) is inclusive of 0, so the drawn values are in 0..9 (non-negative), not "positive" (0 is not positive). The stated range 0..9 is correct; only the word "positive" is inaccurate. The same wording also appears in the file header comment ("0..6 small positive ints (0..9)"). Consider dropping "positive" to keep the comment precise about what the code does.
💡 Suggested change
Before:
* ::int[]. n is drawn in 0..6 elements, each a small positive int in 0..9 so
After:
* ::int[]. n is drawn in 0..6 elements, each a small int in 0..9 so
📄 src/test/prop/prop_joinmethods.c (L165-L170)
The failure diagnostic here is unreliable. A single err is reused across all three pg_prop_sql_exec/pg_prop_sql_rows calls plus the final RESET ALL;. Per the helper contract (pg_prop_sql.c), pg_prop_sql_rows/pg_prop_sql_exec only write *errmsg on failure and leave it untouched on success. So when e.g. the mergejoin run fails (f2), the error it set is overwritten to nothing by the subsequent nestloop run (if it succeeds) and the RESET ALL; call, and the report prints a stale/empty message instead of the failing query's error. In addition, every reassignment of err on a prior failure leaks the previously strdup'd string. Check f1/f2/f3 immediately after each run (as prop_selfjoin.c does), or use a distinct error variable per run.
📄 src/test/prop/prop_outerjoin.c (L209-L214)
Same stale-error-reporting defect as prop_joinmethods.c run_three: a single err is shared across the three runs and the trailing RESET ALL;. Because pg_prop_sql_rows/pg_prop_sql_exec only set *errmsg on failure, a failing run's message is overwritten (to nothing) by the later successful calls and RESET ALL;, so the counterexample report prints a stale/empty error; intermediate failures also leak the prior err. Check each f1/f2/f3 right after its run, or use separate error variables.
📄 src/test/prop/prop_arrayconstruct.c (L79-L80)
Several "properties" in this file draw no input at all and run a single fixed SQL string: array_subquery_eq_agg, array_ndims, array_length_each_dim, cardinality_multidim, and array_append_dim. Registering a constant assertion as a PG_PROP() is degenerate for a property-based harness: the engine generates and shrinks thousands of identical cases per run with zero input variation, so there is no counterexample space to explore and the cost (per-case connect/query against the live cluster) buys nothing a one-shot regression check wouldn't. Either parameterize these over drawn element values/dimensions (the way array_fill_dims and array_dims_1d already do in this same file), or move the fixed facts to a plain regression test. (confidence: moderate)
📄 src/test/prop/prop_intops.c (L96-L97)
Stale comment: the code draws a and b in -100000..100000 (i.e. -1e5..1e5), not -1e9..1e9 as the comment claims. Comments must describe what the code does now.
💡 Suggested change
Before:
* a + b = b + a for int8 a, b in a safe range (-1e9..1e9) where every sum
* stays well within int8.
After:
* a + b = b + a for int8 a, b in a safe range (-1e5..1e5) where every sum
* stays well within int8.
📄 src/test/prop/prop_intops.c (L134-L135)
Stale comment: the code draws a, b, c in -10000..10000 (i.e. -1e4..1e4, total magnitude <= 3e4), not -1e6..1e6/<= 3e6 as described.
💡 Suggested change
Before:
* (a + b) + c = a + (b + c) for int8 a, b, c in -1e6..1e6, so the total sum
* (<= 3e6 in magnitude) fits int8 with room to spare.
After:
* (a + b) + c = a + (b + c) for int8 a, b, c in -1e4..1e4, so the total sum
* (<= 3e4 in magnitude) fits int8 with room to spare.
📄 src/test/prop/prop_intops.c (L225-L227)
Stale/aspirational comment contradicting the code: b is not "drawn in -1000..1000" with a "loop [that] re-draws until b <> 0" — the code draws b in 1..1000 then randomly negates (no reject loop, see the inline comment two lines below). Also a is drawn in -1e4..1e4, not -1e6..1e6. Fix the comment to match the actual generation.
💡 Suggested change
Before:
* zero, and % is the matching remainder, so this identity is exact. b is
* drawn in -1000..1000; the loop re-draws until b <> 0 so no division by zero
* is attempted. a in -1e6..1e6. Every intermediate fits int8.
After:
* zero, and % is the matching remainder, so this identity is exact. b is
* drawn in 1..1000 then randomly negated, so b <> 0 without a reject loop and
* no division by zero is attempted. a in -1e4..1e4. Every intermediate fits
* int8.
📄 src/test/prop/prop_arithcmp.c (L48-L53)
This require_true helper is duplicated verbatim across all six files under review (and ~200 others in src/test/prop). Since the body only depends on the public pg_prop API (pg_prop_sql_scalar / pg_prop_fail / pg_free), it would be a good candidate to promote into pg_prop.h/pg_prop_sql.c as a shared helper, eliminating the copy-paste and the drift risk. Low priority / framework-wide decision, but worth raising on the thread.
📄 src/test/prop/prop_prng.c (L67-L69)
These format strings use %llu/%lld for uint64/int64 values. PostgreSQL's convention is to use the portable UINT64_FORMAT/INT64_FORMAT macros for 64-bit integers rather than the ll length modifier. The explicit (unsigned long long)/(long long) casts make this technically safe (int64 is long long int on supported platforms), but no other prop_*.c file deviates from the macro convention, so this stands out. Confidence: moderate.
📄 src/test/prop/prop_encoding.c (L65-L65)
sprintf is on PostgreSQL's discouraged-functions list; prefer snprintf with the remaining buffer size as the bound. The loop guard off + 3 < sizeof(hex) and hex[64*3+1] sizing do prevent an actual overflow for the <=64-byte buffers these callers draw, so this is a style/convention fix rather than a correctness bug. Confidence: high (that it is safe), moderate (that snprintf is preferred).
📄 src/test/prop/prop_nbtree.c (L198-L203)
This differential test can pass vacuously and would not catch nbtree regressions. enable_seqscan = off is a soft cost penalty (it adds disable_cost), not a hard switch: for the tiny tables generated here (0-60 rows) and predicates that produce no usable index path (e.g. a lone col IS NOT NULL, col <> k, or an OR chain the planner cannot turn into an index qual), the planner can still pick a sequential scan for the "forced index scan" run. When that happens both runs execute identical seqscans, strcmp(idx, seq) trivially matches, and the property never exercises nbtree scan-key handling at all -- i.e. it would still pass with the skip-scan code reverted.
Add a plan-shape guard: run EXPLAIN (FORMAT ...)/EXPLAIN on q.data under the index-forcing GUCs and pg_prop_assume() (or pg_prop_fail) that the plan actually contains an Index Scan / Index Only Scan node before comparing results. Otherwise the suite reports green coverage it does not have.
📄 src/test/prop/prop_nbtree.c (L203-L203)
err is reused across every pg_prop_sql_exec/pg_prop_sql_rows call without being freed between them. pg_prop_sql_rows/pg_prop_sql_exec return *errmsg via pg_strdup (owned by the caller). If the index-scan pg_prop_sql_rows on this line sets err, the following SET ... enable_indexscan = off exec and the seq pg_prop_sql_rows overwrite err, leaking the earlier allocation; and when pg_prop_fail fires it longjmps over the (absent) free entirely. Since the harness has no per-case memory arena (only connections are reclaimed by pg_prop_sql_cleanup), err, idx, seq, and the two StringInfo buffers all leak on the failure/counterexample path. Free err between uses (and before each pg_prop_fail), e.g. a small helper, to keep the leak bounded.
📄 src/test/prop/prop_spillpaths.c (L72-L75)
On the normal (passing) path err is always NULL here -- every call that could set err is immediately followed by pg_prop_fail, which is pg_noreturn and longjmps out. So this if (err) pg_free(err) is effectively dead, and the actual non-NULL err (plus r_small/r_big) leak on the pg_prop_fail paths because the longjmp skips these frees and the harness has no per-case memory arena to reclaim them (only connections are freed by pg_prop_sql_cleanup). Minor given these only fire on terminal failures, but worth freeing err immediately before each pg_prop_fail that references it.
📄 src/test/prop/prop_crypto.c (L98-L98)
The property is named ..._and_sized and the file header claims "the digest is exactly the type's length" is one of the properties being pinned down, but this test body only checks determinism (memcmp(a, b, ...)). Passing PG_SHA256_DIGEST_LENGTH to pg_hmac_final is just the output buffer capacity, not an assertion that the produced digest length equals it. Either add an explicit length check (e.g. capture/verify the written length) or rename the property to drop "_and_sized" so the name and header comment match what is actually verified.
📄 src/test/prop/prop_powexp.c (L13-L14)
Stale/inaccurate module comment: it claims the bases are "kept small enough ... (b<=5, e<=6, factorial n<=12)", but pow_zero_one draws the base in 2..9 and log10_pow10 uses base 10, so b<=5 is not what the code does. Comments must describe current behaviour; please widen the stated bound (e.g. "b<=10") or qualify it per-property.
💡 Suggested change
Before:
* artefact, because the bases/exponents are kept small enough that no
* overflow or scale loss can occur (b<=5, e<=6, factorial n<=12).
After:
* artefact, because the bases/exponents are kept small enough that no
* overflow or scale loss can occur (b<=10, e<=6, factorial n<=12).
📄 src/test/prop/prop_spillpaths.c (L90-L90)
The spill paths this file is named for are essentially never exercised. build_table generates at most 60 rows of three int columns (a few hundred bytes total), which fits comfortably in the 64kB work_mem used by run_two_mem's "spill-prone" run. So neither the 64kB run nor the 64MB run ever spills to disk -- both execute fully in memory, and run_two_mem just compares two identical in-memory executions. The result: these properties would still pass with every spill path (tape merge, HashAgg disk spill, hash-join batch repartitioning, external DISTINCT/UNION) completely broken, which is exactly what the test claims to catch.
To actually drive the external paths, generate enough data (and/or wide-enough values) that 64kB forces a spill -- e.g. thousands of rows, or pad v/add a wide text column -- or lower the small-side work_mem far enough (combined with distinct keys) that these plans genuinely overflow. As written the test provides false confidence (confidence: high).
📄 src/test/prop/prop_bit.c (L60-L61)
Stale comment: it references a helper draw_bits_len_choice() that does not exist anywhere in the tree. The callers (and_or_not_demorgan, shift_left_right_length_preserved, get_bit_set_bit_consistent) actually draw the length L inline via pg_prop_draw_int(tc, 1, 40), not through a draw_bits_len_choice() function. Comments must describe what the code does now; please fix the reference to match the actual call pattern.
💡 Suggested change
Before:
* caller draws L once with draw_bits_len_choice() and then draws each body of
* exactly that length.
After:
* caller draws L once (via pg_prop_draw_int) and then draws each body of
* exactly that length.
📄 src/test/prop/prop_boxops.c (L333-L338)
This per-property header is stale and contradicts the code. It claims the property checks area(polygon(B)) = area(B), but the inner comment below correctly notes area(polygon) is not defined and the SQL actually checks polygon(box) @> center(box). The function name box_to_polygon_contains_center matches the actual behavior, not this header. Rewrite the header to describe the containment check.
💡 Suggested change
Before:
* A polygon built from a box encloses the same area: area(polygon(B)) =
* area(B). polygon(box) produces the four-corner rectangle, so the two area
* computations must agree; integer corners make both exact up to any float
* rounding, so compare within a tolerance.
*/
PG_PROP(box_to_polygon_contains_center)
After:
* A polygon built from a box covers the box's center point:
* polygon(B) @> center(B). (area(polygon) is not defined, so the area
* identity cannot be checked directly.) polygon(box) produces the
* four-corner rectangle, whose interior contains the box center.
*/
PG_PROP(box_to_polygon_contains_center)
📄 src/test/prop/prop_boxops.c (L11-L12)
File header overstates the oracle set: it lists "a polygon built from a box encloses the same area" and "polygon(box) area against box area", but no property checks polygon area (it is undefined). The box_to_polygon property only checks center containment. Update these two lines to avoid describing behavior the code does not implement.
💡 Suggested change
Before:
* each; the bounding box of two boxes covers both; and a polygon built from a
* box encloses the same area. width()/height() report the side lengths.
After:
* each; the bounding box of two boxes covers both; and a polygon built from a
* box covers the box's center. width()/height() report the side lengths.
📄 src/test/prop/prop_bitstringops.c (L37-L47)
draw_bitstring_len() returns its own len parameter unchanged, and draw_bitstring() returns that value, yet none of the three callers (concat_length, xor_self_zero, shift_length_preserved) use the result. The length that callers do need (and_or_demorgan, setbit_getbit) is drawn separately with pg_prop_draw_int(tc, 1, 16). The return values here are dead; make both helpers return void to avoid implying the result carries information.
💡 Suggested change
Before:
static int
draw_bitstring_len(pg_prop_tc *tc, StringInfo buf, int len)
{
int i;
appendStringInfoString(buf, "B'");
for (i = 0; i < len; i++)
appendStringInfoChar(buf, pg_prop_draw_bool(tc) ? '1' : '0');
appendStringInfoChar(buf, '\'');
return len;
}
After:
static void
draw_bitstring_len(pg_prop_tc *tc, StringInfo buf, int len)
{
int i;
appendStringInfoString(buf, "B'");
for (i = 0; i < len; i++)
appendStringInfoChar(buf, pg_prop_draw_bool(tc) ? '1' : '0');
appendStringInfoChar(buf, '\'');
}
📄 src/test/prop/prop_coalesce.c (L94-L94)
These four new areas (casewhen, condexpr, coalesce, coalescefn) all target the same operator set (CASE/COALESCE/NULLIF/GREATEST/LEAST) with heavily overlapping oracles. For example coalesce_first_nonnull here is essentially the same property as prop_casewhen.coalesce_first_nonnull and prop_coalescefn.coalesce_first_nonnull_wins; nullif_semantics duplicates prop_casewhen.nullif_definition and prop_condexpr.coalesce_nullif_identity; the GREATEST/LEAST NULL-skip oracle recurs in all four files; the CASE-no-match-is-NULL and COALESCE-idempotent oracles each appear three times. This is redundant coverage that bloats the suite and burns CI/reviewer time for little incremental signal. Consider consolidating into one or two areas (e.g. keep the one-step definitions in coalesce and the deeper algebraic laws in coalescefn, and drop the duplicates). (medium confidence)
📄 src/test/prop/prop_condexpr.c (L130-L130)
The property name coalesce_nullif_identity is misleading: this property is purely about NULLIF -- its SQL (nullif(a,b) IS NOT DISTINCT FROM CASE WHEN a=b THEN NULL ELSE a END) contains no coalesce(), and the comment above it describes only NULLIF semantics. Since the name becomes the property's identifier (condexpr/coalesce_nullif_identity) in the example database and on the command line, the coalesce_ prefix is wrong and will confuse anyone selecting or triaging it. Rename to something like nullif_case_identity. (moderate confidence)
💡 Suggested change
Before:
PG_PROP(coalesce_nullif_identity)
After:
PG_PROP(nullif_case_identity)
📄 src/test/prop/prop_collate.c (L317-L326)
This property draws no input and asserts a single fixed SQL fact. Unlike the other properties here it has no pg_prop_draw_* call, so it degenerates to a constant smoke check: it exercises exactly one hardcoded comparison and cannot catch a regression beyond that one pair. Consider either folding this fact into c_collation_is_byte_order (whose byte-order oracle already covers 'A' < 'a' when those characters are drawn) or dropping it, so every property in the file is a genuine over-drawn-input invariant rather than a tautology that passes trivially.
📄 src/test/prop/prop_coerce.c (L301-L304)
The comment itself concedes the oracle has no discriminating power: it admits the equality "would still equal n::numeric" even if the int branch were not promoted. As written this property does not actually test CASE result unification -- CASE WHEN true THEN n ELSE x END = n::numeric holds for n of either int or numeric type (int = numeric comparison promotes operands), so a resolution bug that failed to unify the branches would not be caught. To make this a real invariant, assert the common type (e.g. via pg_typeof(CASE WHEN ... END) = 'numeric'::regtype) rather than relying on a value equality that both resolutions satisfy.
📄 src/test/prop/prop_char.c (L14-L17)
Stale/contradictory comment. The file header claims ((x::char(n))::text) keeps the n-width padding, but the body of bpchar_blank_pads_to_n (and its own inline comment at lines 158-161) states the opposite: a ::text cast trims trailing blanks, which is exactly why the implementation uses octet_length(...::char(n)) instead. This header describes an abandoned approach and is misleading. Confidence: high.
📄 src/test/prop/prop_char.c (L133-L136)
This function-header comment contradicts the actual implementation. It says the property casts to text and takes length() of ((S::char(n))::text) claiming that "preserves the n-width padding", but the body below uses octet_length(...::char(n)) and its inline comment explicitly notes a ::text cast trims the padding. Update the header to describe the octet_length-based check actually performed. Confidence: high.
📄 src/test/prop/prop_strconcat.c (L241-L241)
Misleading test name. concat_ws_eq_string_agg advertises a concat_ws vs string_agg equivalence, but the body never calls concat_ws -- it counts the delimiters in a string_agg and compares against greatest(count(*)-1, 0). Rename to reflect what is actually tested (e.g. string_agg_comma_count) so a failure report is not misleading. Confidence: high.
📄 src/test/prop/prop_numcast.c (L405-L411)
The oracle for this "never silently wrap" property is tautological: the two sides of = are byte-identical expressions, so the query is x = x. If the int8->int2 narrowing cast silently wrapped (the exact bug this property claims to guard against), both sides would wrap to the same value and the comparison would still return t. The test therefore passes whether or not the cast wraps -- it detects nothing beyond "the cast does not error," and the require_true_or_skip skip path masks even that for out-of-range draws. It would still pass with overflow detection reverted, which per the review rules makes it worthless.
Use an independent oracle, matching the sibling pattern in prop_safemath.c: assert the narrowed value equals the original exactly when (and only when) n is within int2 range, requiring an error otherwise. For example compare n::int8::int2::int8 = n::int8 guarded by a range check, so a silent wrap produces a value differing from n and fails the invariant. (high confidence)
📄 src/test/prop/prop_defaults.c (L260-L261)
Weak oracle: the drawn values k1/k2 are inserted only into column v, which is never read, and the assertion is just min(id) < max(id). This passes for any two distinct ids and does not verify that IDENTITY is strictly increasing in insertion order (ids {5,2} with the second insert getting the smaller id would still pass) nor the step. The random draws contribute nothing to the check, so the property is effectively a single deterministic case and would not catch a regression that produced distinct-but-non-monotonic ids. Capture the two ids with RETURNING (or key them off v) and assert the row inserted second has the strictly greater id, e.g. SELECT (SELECT id FROM ds WHERE v = ) > (SELECT id FROM ds WHERE v = ) with distinct k1/k2.
📄 src/test/prop/prop_defaults.c (L232-L232)
Name/comment drift: the DDL uses GENERATED ALWAYS AS IDENTITY, not SERIAL, so the property name serial_increments is misleading. Also the file header promises IDENTITY "yields strictly increasing values", but the oracle below only checks min<max (distinctness), not strict monotonic increment. Rename to e.g. identity_increments and align the header claim with what is actually asserted.
📄 src/test/prop/prop_defaults.c (L12-L12)
Comment accuracy: the header says "Each property builds a fresh scratch table" but the scratch names are hard-coded and reused across properties (df appears in default_applied and default_expression; dg in generated_stored and generated_updates_with_base). This is safe because the runner executes properties serially, but the "fresh" wording overstates isolation. Consider dropping the "fresh" claim or noting the names are fixed and reused under serial execution.
📄 src/test/prop/prop_checkconstraint.c (L10-L10)
Comment accuracy: this claims each property builds a scratch table "with a unique name", but the table names are hard-coded literals (cc_rv, cc_null, cc_dom, cc_mc, cc_ce, cc_nn) rather than uniquely generated. They are safe only because the runner is serial. Drop the "unique name" claim to match the code.
📄 src/test/prop/prop_foreignkey.c (L104-L104)
atol() has no error detection and parses into long, which is 32-bit on LLP64 (Windows/MSVC); it is also inconsistent with the rest of the harness, which parses scalars elsewhere via strcmp or avoids atol entirely. The counts here are bounded (<=10) so there is no practical overflow, but strtol() with validation (or comparing the returned string directly) is more robust and portable.
📄 src/test/prop/prop_checkconstraint.c (L29-L29)
Unused include: this file includes lib/stringinfo.h but never uses StringInfo (none of the properties build dynamic SQL -- all SQL is passed as string literals, unlike prop_defaults.c and prop_foreignkey.c which do use StringInfo). Per the minimal-diff discipline, drop this include. (low confidence on severity, high confidence it is unused.)
📄 src/test/prop/prop_inetarith.c (L60-L61)
is_overflow() is too broad and undermines the test. strstr(msg, "cannot") matches a large set of unrelated PostgreSQL error messages (cannot cast, cannot subtract addresses from different families, etc.), so a genuine bug that produces any "cannot..." error would be silently swallowed as a legitimate overflow and the property would pass — defeating the test's purpose. Also "out of range" already subsumes "result is out of range", so that fourth clause is dead. Narrow this to the exact overflow message(s) actually raised by inet +/- (confirm the real text and SQLSTATE), and drop the redundant/overbroad clauses. [high confidence]
💡 Suggested change
Before:
strstr(msg, "cannot") != NULL ||
strstr(msg, "result is out of range") != NULL;
After:
strstr(msg, "result is out of range") != NULL;
📄 src/test/prop/prop_cidrops.c (L303-L305)
Comment/code drift: the function is named cidr_le_inet_order and the comment says it checks "The cidr's ordering as inet", but the SQL only evaluates network(inet) <= broadcast(inet) and never casts to or exercises cidr at all. As written this is identical in substance to prop_networkops.c's broadcast_ge_network (just <= vs >=) and tests nothing specific to the cidr type. Either rewrite the predicate to actually involve a cidr value or fix the name/comment to describe what it does. [high confidence]
📄 src/test/prop/prop_cidrops.c (L335-L338)
The first conjunct (2::numeric ^ (32 - len)) >= 1 is a tautology: len is drawn 0..32, so the exponent is 0..32 and 2^k >= 1 is always true for every generated case — it can never catch a regression. The second conjunct (masklen(set_masklen(..., 32)) = 32) is the only meaningful part, and it merely duplicates what masklen_matches/host_in_its_network already cover. Replace the dead first conjunct with a real property (e.g. relate host count to masklen for the drawn len), or drop this property. [high confidence]
📄 src/test/prop/prop_charfuncs.c (L273-L274)
ascii_order asserts a strict text ordering chr(n1) < chr(n2) for ASCII codepoints n1 < n2, but the < operator uses the cluster's default collation. t/002_sql_props.pl starts the node with a plain $node->init (no forced locale), so the default collation is whatever the build environment's LC_COLLATE is. Under a non-C collation (glibc/ICU), ASCII punctuation/space codepoints 33..126 do NOT reliably sort in codepoint order (e.g. '-' can be ignorable/variable relative to letters), so this property can fail spuriously depending on the host locale. This is exactly why the sibling areas prop_collate.c and prop_textord.c force COLLATE "C" on every ordering comparison. Force C ordering here too (and update the comment, which claims ASCII codepoint order is string order -- only true under C).
Confidence: high.
💡 Suggested change
Before:
initStringInfo(&q);
appendStringInfo(&q, "SELECT chr(%d) < chr(%d)", n1, n2);
After:
initStringInfo(&q);
appendStringInfo(&q, "SELECT chr(%d) COLLATE \"C\" < chr(%d) COLLATE \"C\"", n1, n2);
📄 src/test/prop/prop_networkops.c (L85-L91)
Name/behavior mismatch and weak oracle. The property is named host_in_its_network (promising a containment check), yet the SQL only evaluates masklen(set_masklen(X, m)) = m -- a pure masklen accessor round-trip that never checks any host/network containment. It can only fail if set_masklen reports back a wrong masklen; it exercises nothing about the host being contained in its network. Either rename to reflect what is tested (e.g. set_masklen_roundtrip) or change the SQL to actually test the stated containment property. Confidence: high.
📄 src/test/prop/prop_networkops.c (L211-L216)
Name does not match behavior. abbrev_roundtrip never calls abbrev(); the SQL is a plain (inet '...')::text::inet = inet '...' round-trip (the comment even calls it a "Text round-trip"). As written it does not exercise the abbreviated/compressed rendering the name implies. Rename to text_roundtrip or make the SQL go through abbrev(). Confidence: high.
📄 src/test/prop/prop_inet6.c (L195-L196)
Comment overstates what is tested, and the name is misleading. The comment claims "set_masklen does not lose or alter the address", but the SQL only checks masklen(set_masklen(X, 128)) = 128 and never verifies the address bits are preserved -- a near-tautological masklen-accessor check. The property name host_in_network also implies a containment check that is not performed. To actually test address preservation, compare host/address bits (e.g. host(set_masklen(X, 128)) = host(X)); otherwise fix the comment and name to reflect the masklen-only check. Confidence: high.
📄 src/test/prop/prop_tstzarith.c (L223-L230)
Stale header comment contradicts the code (high confidence). This block still describes an "age_plus_back" property asserting TS1 + age(TS2, TS1) = TS2, but the function is named age_sign_matches_order and only tests the sign invariant. The inner comment at lines 244-250 even explains why the header's claim is unsound ("age(a, b) is not invertible by addition"). A comment that describes behavior the code deliberately does not test misleads readers; update the header to match the implemented sign-only invariant.
💡 Suggested change
Before:
* 4. age_plus_back
*
* age(TS2, TS1) is a calendar interval (variable-length months/days) that
* spans from TS1 to TS2. Adding it back recovers TS2:
* TS1 + age(TS2, TS1) = TS2. This holds exactly in UTC because there is no
* DST crossing to make the calendar interval ambiguous.
*/
PG_PROP(age_sign_matches_order)
After:
* 4. age_sign_matches_order
*
* age(TS2, TS1) is a calendar interval (variable-length months/days) spanning
* from TS1 to TS2. It is not invertible by addition, so the sound invariant
* is its sign: age(TS2, TS1) is positive, zero, or negative exactly as TS2 is
* after, equal to, or before TS1. This holds exactly in UTC because there is
* no DST crossing to make the calendar interval ambiguous.
*/
PG_PROP(age_sign_matches_order)
📄 src/test/prop/prop_date.c (L47-L54)
Low confidence / low severity, and this mirrors an existing suite-wide pattern (prop_cash.c, prop_interval.c, prop_numcast.c, etc.), so treat as a note rather than a blocker. is_overflow() matches the English substrings "out of range"/"overflow" in the error text. The Tier-B TAP driver (t/002_sql_props.pl) does not pin lc_messages=C on the node, so under a non-English build these overflow-skip paths would stop matching: legitimate range overflow would be reported as a spurious property failure. Matching on the SQLSTATE (e.g. 22003 numeric_value_out_of_range / 22008 datetime_field_overflow via PQresultErrorField(res, PG_DIAG_SQLSTATE)) would be locale-independent. Worth considering as a harness-wide improvement rather than fixing only here.
📄 src/test/prop/prop_distinct.c (L139-L143)
The return values of these SET enable_hashagg statements are ignored, which can make this differential oracle vacuous. If SET enable_hashagg = off silently fails, rs is computed under the same strategy as rh, so strcmp(rh, rs) compares an identical plan against itself and can never catch a hash-vs-sort divergence (a test that passes even when it exercises nothing). The sibling file prop_aggstrategies.c runs the identical on/off dance and correctly checks each SET, e.g. if (!pg_prop_sql_exec(conn, "SET enable_hashagg = on", &err)) pg_prop_fail(...). Follow that pattern here. (Separately, err is reused across three exec calls; pg_prop_sql_exec only writes *errmsg on failure, so a failing earlier SET's message is leaked when a later SET overwrites the pointer — checking each call also fixes that.) [moderate confidence]
💡 Suggested change
Before:
pg_prop_sql_exec(conn, "SET enable_hashagg = on", &err);
rh = pg_prop_sql_rows(conn, query, &fh, &eh);
pg_prop_sql_exec(conn, "SET enable_hashagg = off", &err);
rs = pg_prop_sql_rows(conn, query, &fs, &es);
pg_prop_sql_exec(conn, "RESET ALL", &err);
After:
if (!pg_prop_sql_exec(conn, "SET enable_hashagg = on", &err))
pg_prop_fail(tc, "SET enable_hashagg=on failed: %s", err ? err : "");
rh = pg_prop_sql_rows(conn, query, &fh, &eh);
if (!pg_prop_sql_exec(conn, "SET enable_hashagg = off", &err))
pg_prop_fail(tc, "SET enable_hashagg=off failed: %s", err ? err : "");
rs = pg_prop_sql_rows(conn, query, &fs, &es);
pg_prop_sql_exec(conn, "RESET ALL", &err);
📄 src/test/prop/prop_decimalfmt.c (L245-L250)
normalize_text builds the exact same SQL as text_roundtrip (both emit SELECT (('%s'::numeric)::text)::numeric = '%s'::numeric from the same draw_magnitude input). Despite the differing comments, the two properties assert byte-identical invariants, so one is redundant coverage that only burns test iterations. Either drop normalize_text or give it a genuinely distinct oracle (e.g. assert the text form has no trailing-zero padding beyond the value's scale, or compare ::text output against an independently computed canonical form).
📄 src/test/prop/prop_distinct.c (L226-L228)
This differential oracle is vacuous for DISTINCT ON. The planner never uses hash aggregation for DISTINCT ON regardless of enable_hashagg (see planner.c grouping_planner: else if (parse->hasDistinctOn || !enable_hashagg) allow_hash = false; with the comment "if the query uses DISTINCT ON ... it won't really have the expected behavior if we hash"). run_two_distinct toggles enable_hashagg on/off, but both runs produce the identical sort-based plan, so strcmp(rh, rs) compares a plan against itself and can never detect a hash-vs-sort disagreement. The comment "Hash vs sort must agree on that choice" is therefore inaccurate for this case. Either test DISTINCT ON against a different oracle (e.g. the GROUP BY/min() equivalence already used in prop_distincton.c), or drop this property. (high confidence)
📄 src/test/prop/prop_tochar.c (L44-L47)
The "cannot" substring makes this overflow filter too broad for a test oracle. strstr(errmsg, "cannot") matches many unrelated backend errors (e.g. "cannot cast type ...", "function ... cannot ..."), so a genuine formatting/parse bug whose error message happens to contain "cannot" would be silently skipped via pg_prop_assume(false) instead of being reported as a failure, weakening the test. Prefer matching only the true overflow/range messages ("out of range", "overflow") as prop_float.c's require_true_or_skip does, or match a more specific phrase.
📄 src/test/prop/prop_toformat.c (L42-L45)
Same concern as prop_tochar.c: the "cannot" substring is too broad for an overflow/skip filter. strstr(errmsg, "cannot") matches unrelated errors (cast/coercion failures, etc.), so a real formatting bug emitting such a message would be silently discarded by require_true_or_skip rather than failing the property. Restrict to the genuine range/overflow phrases to keep the oracle sound.
📄 src/test/prop/prop_unionall.c (L268-L272)
This property is a tautology and cannot fail, so it tests nothing (it would pass unchanged with Append/MergeAppend reverted). The docstring claims it verifies that "A UNION ALL with a common ORDER BY is planned as a MergeAppend and its output must be globally sorted," but the inner (SELECT v FROM ua_a UNION ALL SELECT v FROM ua_b) has NO ORDER BY, so the physical Append order is never observed. The ordering is imposed entirely by the window lag(v) OVER (ORDER BY v NULLS LAST), which re-sorts the rows regardless of how the executor emitted them. bool_and(prev IS NULL OR prev <= v OR v IS NULL) over a stream the window already sorted by v is non-decreasing by construction and is always true. To actually exercise MergeAppend, push a common ORDER BY into the set operation itself (e.g. (SELECT v FROM ua_a ORDER BY v) UNION ALL (...) is not legal; use SELECT v FROM (...) UNION ALL (...) ORDER BY v and check that the raw emission order -- captured via WITH ORDINALITY / a row_number() over no ordering on the Append output -- is non-decreasing), or verify ordering without a re-sorting window. As written this is dead coverage. [high confidence]
📄 src/test/prop/prop_numfmt.c (L277-L278)
This property cannot catch the silent-wrap bug it claims to target ("must never wrap silently", "Targets the int8 addition overflow check"). The single query combines the int8 add with the numeric-oracle cast (A::numeric + B::numeric)::int8. Whenever the true sum does not fit int8 (the only situation where a wrap is possible), that oracle cast also raises "out of range", so the whole query errors and is_overflow routes the case to pg_prop_assume(false) -- skipped. A broken (wrapping) int8 add therefore produces no observable failure: the test still passes with the overflow check reverted, i.e. it does not exercise the code it names. Split the two sides so the int8 result is observed independently of the oracle cast -- e.g. compute the oracle as (A::numeric + B::numeric) and compare it against the int8 result cast up to numeric ((A::int8 + B::int8)::numeric), so a wrap yields a mismatch instead of an oracle-side overflow that gets skipped.
📄 src/test/prop/prop_numfmt.c (L347-L348)
Same soundness gap as the add variant: when the true product overflows int8 (the only case where a wrap can occur), the oracle cast (A::numeric * B::numeric)::int8 overflows too, the query errors, and the case is skipped via is_overflow. A silently-wrapping int8 multiply is never caught, so this test would still pass with the int8 multiply overflow check reverted. Observe the int8 result independently of the oracle cast (e.g. compare (A::int8 * B::int8)::numeric to A::numeric * B::numeric).
📄 src/test/prop/prop_tochar.c (L100-L104)
On a non-overflow SQL error, err is freed before pg_prop_fail, so the failure report contains only the SQL text and drops the actual backend error message. require_true (used by the other properties) includes err in its message, which is far more useful for debugging a genuine formatting.c bug. Defer freeing err until after it has been included in the failure message.
💡 Suggested change
Before:
if (err)
pg_free(err);
if (overflow)
pg_prop_assume(tc, false); /* legitimate overflow: skip */
pg_prop_fail(tc, "SQL error for: %s", sql);
After:
if (overflow)
{
if (err)
pg_free(err);
pg_prop_assume(tc, false); /* legitimate overflow: skip */
}
pg_prop_fail(tc, "SQL error for: %s\n error: %s",
sql, err ? err : "(unknown)");
📄 src/test/prop/prop_toformat.c (L65-L69)
Same diagnostic-loss issue as prop_tochar.c: on a non-overflow SQL error, err is freed before pg_prop_fail, so the reported failure omits the actual backend error message. Keep err until it has been printed in the failure message so a real formatting.c bug is debuggable.
💡 Suggested change
Before:
if (err)
pg_free(err);
if (overflow)
pg_prop_assume(tc, false); /* legitimate overflow: skip */
pg_prop_fail(tc, "SQL error for: %s", sql);
After:
if (overflow)
{
if (err)
pg_free(err);
pg_prop_assume(tc, false); /* legitimate overflow: skip */
}
pg_prop_fail(tc, "SQL error for: %s\n error: %s",
sql, err ? err : "(unknown)");
📄 src/test/prop/prop_filteragg.c (L66-L66)
Stale doc reference: there is no area_spec.md in this change set (the design doc added is src/test/prop/DESIGN). This dangling reference sends readers to a nonexistent file. Point it at the actual DESIGN doc (the same stale area_spec.md reference also appears in prop_coerce.c, prop_collate.c and prop_selfjoin.c, so fix them consistently). [low confidence on exact target name, high confidence the file does not exist]
💡 Suggested change
Before:
* compare (see area_spec.md).
After:
* compare (see DESIGN).
📄 src/test/prop/prop_exceptintersect.c (L207-L208)
Comment/code drift: the docstring says the oracle compares against "a FULL JOIN of the grouped counts", but the actual oracle uses greatest(count_a - count_b, 0) over two correlated scalar subqueries driven by SELECT DISTINCT v FROM ei_a -- there is no FULL JOIN here. Per the project's comment-accuracy rule, the comment should describe what the code does now. Suggest rewording to match (e.g. "against greatest(count_a - count_b, 0) over A's distinct values, counts NULL-grouped via IS NOT DISTINCT FROM"). (low confidence on severity; the mismatch itself is certain.)
📄 src/test/prop/prop_floatspecial.c (L85-L85)
Misleading property name: it reads nan_not_equal_itself..., but both the comment above it and the SQL assert the opposite -- that under PostgreSQL's total order 'NaN' = 'NaN' is TRUE. The "not_equal_itself" fragment contradicts the asserted behavior and will confuse readers. Rename to reflect what is tested, e.g. nan_equal_itself_and_is_not_distinct.
📄 src/test/prop/prop_float.c (L42-L44)
This comment claims the float generator "yields the special values too (NaN, +/-Inf, subnormals, +/-0)" and that "those are the point". That is not true for the harness in this patch: pg_prop_draw_float() in pg_prop.c calls hegel_generate_float(..., /* allow_nan */ ... ? false : false, /* allow_infinity */ false, ...), so NaN and +/-Inf are never drawn regardless of range. Consequently the isnan(v)/isinf(v) branches below are dead code, and the NaN special-casing in text_roundtrip_f8/text_roundtrip_f4/negation_involution plus the pg_prop_assume(!isnan(...)) guards in order_consistent_with_double/abs_nonnegative never fire. The headline property (float8 -> text -> float8 identity over NaN/+-Inf) silently does not cover those values at all. Either fix the generator so unbounded ranges emit NaN/Inf (the stated intent), or correct this comment and remove the dead special-value handling. (high confidence)
📄 src/test/prop/prop_float.c (L316-L316)
Misleading property name: zero_signs_equal_but_text_distinct claims the text forms are distinct, but the SQL asserts the opposite -- it only checks that -0 renders as either '-0' or '0' (('-0'::float8)::text IN ('-0','0')), explicitly permitting identical text. Nothing here checks +0 and -0 have distinct text. Rename to reflect what is actually asserted (e.g. zero_signs_equal_text_wellformed). (medium confidence)
📄 src/test/prop/prop_hashfns2.c (L222-L224)
This property is a tautology, not an invariant: (%lld)::int4::text and ((%lld)::int4)::text are the identical cast chain (the extra parentheses are purely syntactic and parse to the same expression tree). The comparison therefore holds by SQL grammar regardless of what hashtext does — it would still pass with the hash function reverted or broken. It provides no regression signal. Either turn it into a genuine equal-values-hash-equally check where the two operands reach the same value by different routes (e.g. hashtext((n)::int4::text) = hashtext((n+0)::int4::text)), or drop it.
📄 src/test/prop/prop_hashfns2.c (L238-L238)
hash_distributes_nothing generates SQL that is byte-for-byte identical to hashtext_equal_inputs above (SELECT hashtext(S) = hashtext(S)). It is pure duplication of an already-weak determinism check and adds no coverage. Note too that hashint_deterministic is the same hashtext(x) = hashtext(x) contract on an int-rendered-to-text input. Three properties in this file assert the identical self-equality invariant; collapse them to a single determinism property and spend the rest on checks with real oracles (e.g. md5/sha256 already do more).
📄 src/test/prop/prop_hashfn.c (L236-L236)
The property name promises int4/int8 consistency on small values, but the body only checks int8 self-stability (hashint8(n) = hashint8(n)), and the comment directly above even states "hashint4 and hashint8 are distinct functions and are not expected to agree". The name contradicts both the comment and the code and will mislead maintainers. Rename to match the actual invariant, e.g. hashint8_deterministic.
📄 src/test/prop/prop_hashfn.c (L269-L269)
The name claims a "matches or differs" semantic, but the body only asserts per-seed determinism (hashtextextended(S,0)=hashtextextended(S,0) AND hashtextextended(S,42)=hashtextextended(S,42)), and the comment confirms "We do not assert anything about cross-seed relations, only per-seed determinism." A "matches or differs" assertion is also vacuously true and meaningless. Rename to reflect what is tested, e.g. hashtextextended_deterministic_per_seed.
📄 src/test/prop/prop_indexdiff.c (L139-L142)
enable_* are soft disables: they add a large cost penalty, not a hard prohibition. When no viable index path exists (e.g. the random table is empty/tiny, or the planner declines the GIN/GiST/partial/expression index), both the "index-forced" and "seqscan-forced" runs execute a seqscan and the comparison trivially passes without ever exercising the index AM -- a test that passes with the feature unused catches no regression. The harness's own sql/prop_query_planadvice.c explicitly rejects the "blunt enable_* GUCs" for exactly this reason and uses pg_plan_advice instead. These properties should verify via EXPLAIN that the intended node (Index/Bitmap Index Scan vs Seq Scan) was actually chosen before trusting the result comparison, or pg_prop_assume() the case away when it was not.
📄 src/test/prop/prop_indexdiff.c (L150-L155)
The return value of these GUC pg_prop_sql_exec() calls is ignored. If a SET fails, execution continues and the subsequent query runs under the wrong (or previous) GUC settings, producing a false pass/fail with no diagnostic. Additionally, &err is reused across all these calls and the two pg_prop_sql_rows() calls without being freed between them: a non-NULL err from an earlier failed SET is silently overwritten (leaking the strdup'd string) by the next failure. Check each exec's return and pg_prop_fail on failure, or at minimum free err between uses.
📄 src/test/prop/prop_indexdiff.c (L55-L60)
connect_with_ext() does CREATE EXTENSION IF NOT EXISTS btree_gin/btree_gist, but nothing guarantees those contrib modules are installed into the cluster the TAP driver (t/002_sql_props.pl) starts -- it just does $node->init; $node->start; on a stock build, and the prop Makefile declares no EXTRA_INSTALL = contrib/btree_gin contrib/btree_gist. When the contrib libraries are absent, CREATE EXTENSION raises an error, so pg_prop_fail() turns a missing-prerequisite into a hard test failure instead of a clean skip. Per TAP conventions the suite should skip when the contrib prerequisite is unavailable (and the Makefile must declare EXTRA_INSTALL so make check installs them).
📄 src/test/prop/prop_indexdiff.c (L104-L108)
This require_same_rows() helper is copy-pasted verbatim into prop_predpushdown.c and (minus the two-query signature) into ~25 other prop_*.c files under src/test/prop. That is a large DRY violation: a differential "require two queries return identical rows" oracle belongs once in pg_prop_sql.c / pg_prop.h so every Tier-B area reuses it. As-is, any fix (e.g. the err/row-buffer handling above) has to be made in dozens of copies.
📄 src/test/prop/prop_isn.c (L246-L247)
The hyphens_are_cosmetic property is a tautology and tests nothing. Every literal in valid_ean13[] (the only pool this property draws from) is an unhyphenated 13-digit string, so replace('%s', '-', '') is always a no-op and the generated SQL degenerates to 'V'::ean13 = 'V'::ean13, which is trivially true. The property would still pass even if the isn parser's hyphen handling were completely broken or removed, so it cannot catch any regression in the behavior it claims to exercise.
To make it meaningful, draw from a pool that includes the canonical hyphenated forms (e.g. '978-0-12-345678-6') and assert they parse equal to their digits-only equivalent. (Confidence: high.)
📄 src/test/prop/prop_indexscan.c (L78-L79)
Same blunt-GUC concern as the index-diff area: these three scan_gucs presets are soft disables, so for a small or empty ix_t the planner may fall back to a seqscan under both the "index scan only" and "bitmap scan only" presets, making all three runs identical seqscans. The comparison then passes without ever exercising the index/bitmap scan paths and would not catch an index-AM regression. Verify via EXPLAIN that path 1 actually produced an Index Scan and path 2 a Bitmap Heap Scan before comparing (or assume the case away when they did not); sibling sql/prop_query_planadvice.c documents why bare enable_* GUCs are insufficient here.
📄 src/test/prop/prop_partprune.c (L314-L316)
The return values of these GUC SETs are ignored and &err is reused across the three pg_prop_sql_rows() calls below without being freed between them. If enable_partition_pruning fails to set (or any earlier call sets err), the subsequent query silently runs under the wrong setting and the error string is overwritten/leaked. Capture and check each exec's result, and reset/free err between the row queries so a stale message cannot mask the real failure.
📄 src/test/prop/prop_predpushdown.c (L116-L122)
PP_SETUP() is a function-like macro that declares locals (conn, err) and runs connect/build/fail statements, then is spliced in as the first line of every PG_PROP body. This hides declarations and control flow (a pg_prop_fail on connect failure happens invisibly inside the macro) and is a one-off divergence from the rest of the suite -- its companion in this same change, prop_indexscan.c, inlines the identical connect/build sequence explicitly, as do all other Tier-B areas. Prefer an explicit static helper that returns the pg_prop_conn* (e.g. connect_and_build(tc)) over a statement-emitting macro.
📄 src/test/prop/prop_indexdiff.c (L302-L304)
The comment references a variable named const2 that does not exist; the actual variable holding the square is sq. Fix the stale identifier so the comment matches the code.
💡 Suggested change
Before:
* 4. expr_index: an expression index on (k*k), used by a query matching the
* same expression (k*k = <square>), must match a seqscan. const2 is a square
* in [0, 25] so it is a value the expression can actually take.
After:
* 4. expr_index: an expression index on (k*k), used by a query matching the
* same expression (k*k = <square>), must match a seqscan. sq is a square
* in [0, 25] so it is a value the expression can actually take.
📄 src/test/prop/prop_jsonformat.c (L337-L337)
s is allocated by pg_prop_draw_from() (via pg_malloc) and every other draw buffer in this file and the sibling prop files is released with pg_free(). Using plain free() here is the lone outlier. It happens to work in the frontend (pg_free is a thin wrapper over free), so this is not a bug, but for consistency with the surrounding convention it should be pg_free(s).
💡 Suggested change
Before:
free(s);
After:
pg_free(s);
📄 src/test/prop/prop_jsonb.c (L261-L264)
Inconsistent context argument: here the SQL text (q.data) is passed as both the sql and the context parameter of require_true, so a counterexample report duplicates the SQL and loses the generated JSON. text_roundtrip (and containment_reflexive) pass the generated JSON (j.data) as context, which is more useful for diagnosing shrunk failures. For readable failure output, pass the generated values here too (e.g. the drawn scalars) rather than the SQL again. Same issue in build_matches_parse below.
📄 src/test/prop/prop_jsonbagg2.c (L103-L104)
Comment drifts from the code. build_jb_t() is called unconditionally and builds whatever row count was drawn (0..12); it does not "build at least one row's worth of data only when nrows >= 1". The nrows >= 1 guard only selects the SQL to run (SELECT true for the empty case). Tighten to describe current behavior, e.g.: "Guarded on nrows >= 1 so k0 is present; with no rows the aggregate is NULL and there is nothing to look up, so the property is a SELECT true no-op in that case."
💡 Suggested change
Before:
* empty case is skipped by building at least one row's worth of data only
* when nrows >= 1 (otherwise the property trivially passes a true constant).
After:
* property runs a `SELECT true` no-op in that case.
📄 src/test/prop/prop_jsonbroundtrip.c (L101-L103)
Dead guard: nkeys is drawn from [1,3] (line 89), so it is always >= 1 and this if (nkeys > 0) is unconditionally true. The comma before the "arr" key is always required anyway. Drop the misleading conditional and append the comma unconditionally.
💡 Suggested change
Before:
/* one nested array of integers */
if (nkeys > 0)
appendStringInfoChar(buf, ',');
After:
/* one nested array of integers; there is always at least one prior key */
appendStringInfoChar(buf, ',');
📄 src/test/prop/prop_jsonbsub.c (L151-L157)
n is written by draw_array(tc, &a, &n) but never read in this property (unlike callers that gate on n == 0). It is dead here. Consider passing a throwaway or just leaving a comment; not a compile warning since the address is taken, but it reads as an oversight. Same in array_oob_null.
📄 src/test/prop/prop_jsonbnum.c (L108-L110)
Comment/code drift (low confidence, documentation). The doc example shows the cast form ('x'::numeric)::jsonb but the code uses the function form to_jsonb(%s::numeric). The two happen to produce the same jsonb value, but the comment should describe what the code actually does so a reader isn't misled into thinking the ::jsonb cast path is under test. Suggest updating the example to (to_jsonb('x'::numeric) #>> '{}')::numeric = 'x'::numeric.
💡 Suggested change
Before:
* jsonb preserves numeric value: a scaled numeric cast to jsonb and back is
* equal (as numeric) to the original.
* (('x'::numeric)::jsonb #>> '{}')::numeric = 'x'::numeric.
After:
* jsonb preserves numeric value: a scaled numeric through to_jsonb and back is
* equal (as numeric) to the original.
* (to_jsonb('x'::numeric) #>> '{}')::numeric = 'x'::numeric.
📄 src/test/prop/prop_jsonvsjsonb.c (L4-L4)
The file-header comment is indented with spaces instead of a tab (^ * Property...), unlike every other prop_*.c in this series which uses ^ *\t . This is whitespace churn that pgindent/git diff --check conventions reject; use a tab after * to match the surrounding files.
📄 src/test/prop/prop_jsonnest.c (L96-L96)
%lld is used to format values originating from pg_prop_draw_int, which returns int64. The tree's portability rule is to format 64-bit integers with INT64_FORMAT (as the sibling prop_jsonbroundtrip.c does), not %lld. It happens to work because the values are stored in long long, but it deviates from the subsystem convention and the project's stated portability guidance.
📄 src/test/prop/prop_jsonpathmath.c (L53-L55)
draw_doc's arr_out output parameter and the comma-joined element list it builds are dead: arr.data is never consumed in the SQL of any of the eight properties here (it is only ever pfree'd). Every caller allocates arr, passes &arr, and frees it without reading it. Drop the arr_out parameter and the arr locals to remove the unused state (YAGNI).
📄 src/test/prop/prop_jsontext.c (L213-L213)
Property name json_vs_jsonb_normalization_differs_or_equal is misleading: the test asserts strict equality (=) of the two jsonb normal forms, and the comment itself states "the values must be EQUAL". The "differs_or_equal" suffix suggests a weaker/disjunctive invariant that isn't tested. Rename to reflect the equality it actually checks (e.g. json_to_jsonb_matches_direct_jsonb).
📄 src/test/prop/prop_xmlfmt.c (L31-L32)
Portability gate: the xmlfmt area is wired unconditionally into t/002_sql_props.pl, but PostgreSQL can be built --without-libxml (-Dlibxml=disabled). In that configuration every xmlelement/xmlconcat/xmlagg/IS DOCUMENT call errors with "unsupported XML feature" (xml.c), which require_true treats as a SQL error and thus a property failure -- so the whole area fails spuriously on a perfectly valid build. Unlike contrib-backed areas (hstore/citext) that are always built in the test tree, libxml is an optional dependency. This test needs to skip gracefully when XML support is absent (e.g. detect via SHOW / pg_config and bail out of the area, or gate the area in the TAP driver on a USE_LIBXML check). Confidence: high.
📄 src/test/prop/prop_macaddr.c (L405-L405)
The macaddr8_text_roundtrip property here is an exact duplicate of the text_roundtrip property in the dedicated prop_macaddr8.c (same invariant: ('%s'::macaddr8)::text::macaddr8 = '%s'::macaddr8), and macaddr8_colon_equals_dash likewise duplicates macaddr8 coverage that belongs with the rest of the macaddr8 suite. Since a separate prop_macaddr8.c area exists, consider dropping these two macaddr8 properties from prop_macaddr.c and keeping all macaddr8 coverage in prop_macaddr8.c so there is one home per type and no redundant cases run on every pass. (low confidence this blocks; maintainability nit.)
📄 src/test/prop/prop_mathfn.c (L4-L5)
The file header overstates/misstates coverage. The summary lists mod as a float8 math function tested here, but no property tests mod -- and the degrees_radians_roundtrip comment below explicitly states "There is no float8 mod() in PostgreSQL ... so the trunc-division identity is tested in the numeric areas, not here." The header also omits degrees/radians, which this file actually does test. Fix the summary to match the functions actually exercised.
💡 Suggested change
Before:
* Property-based tests for the float8 math functions (abs, mod, power,
* sqrt, exp, ln, floor, ceil, round, trunc, sign).
After:
* Property-based tests for the float8 math functions (abs, power,
* sqrt, exp, ln, floor, ceil, round, trunc, sign, degrees, radians).
📄 src/test/prop/prop_mathfn.c (L12-L14)
The oracle paragraph claims "trunc-division reconstructs the dividend" is part of this file's oracle set, but no property here tests trunc-division; the degrees_radians_roundtrip comment states this identity is deliberately tested in the numeric areas, not here. Drop this clause (and consider mentioning the degrees/radians round-trip that is actually checked) so the header reflects what the file does now.
💡 Suggested change
Before:
* recovers its argument, floor/ceil bracket the value and differ by at most
* one, ln and exp are inverses, trunc-division reconstructs the dividend, and
* power agrees with repeated multiplication. Float arithmetic is inexact, so
After:
* recovers its argument, floor/ceil bracket the value and differ by at most
* one, ln and exp are inverses, degrees/radians round-trip, and
* power agrees with repeated multiplication. Float arithmetic is inexact, so
📄 src/test/prop/prop_macaddr.c (L389-L390)
The right-hand oracle ('%s' < '%s') compares two untyped literals, which resolve to text and use the database's default collation. The TAP driver (t/002_sql_props.pl) runs $node->init without --no-locale, so the cluster inherits the environment locale; under a non-C collation the text comparison of 0-9a-f can disagree with the byte-wise macaddr ordering, making this property spuriously fail. Sibling oracle properties (prop_collate.c, prop_textord.c) consistently pin COLLATE "C" for exactly this reason, and the property's own comment claims "SQL text < is the right oracle" -- only true under C collation. Pin the comparison to COLLATE "C". (medium confidence)
💡 Suggested change
Before:
"SELECT ('%s'::macaddr < '%s'::macaddr) = ('%s' < '%s')",
c1.data, c2.data, hex1, hex2);
After:
"SELECT ('%s'::macaddr < '%s'::macaddr) = ('%s' COLLATE \"C\" < '%s' COLLATE \"C\")",
c1.data, c2.data, hex1, hex2);
📄 src/test/prop/prop_upsert.c (L47-L55)
This scalar_int helper diverges from every sibling Tier-B prop file (prop_returning.c, prop_updatesem.c, prop_mergecmd.c, prop_dmlcte.c), which all use long long with atoll(). Here it uses long/atol(), and the format strings below print the result with %ld (e.g. the pg_prop_fail calls in each property). scalar_int reads a bigint (count/min/v) from the backend; on LLP64 platforms (64-bit Windows/MSVC) long is 32-bit, so this both risks silent truncation and uses the %ld-for-64-bit pattern the project forbids. Current draws stay tiny so no live failure, but for portability and consistency with the group, use long long/atoll() and %lld.
💡 Suggested change
Before:
long val;
res = pg_prop_sql_scalar(conn, sql, &failed, &err);
if (failed)
pg_prop_fail(tc, "SQL error for: %s\n error: %s",
sql, err ? err : "(unknown)");
if (res == NULL)
pg_prop_fail(tc, "unexpected NULL result for: %s", sql);
val = atol(res);
After:
long long val;
res = pg_prop_sql_scalar(conn, sql, &failed, &err);
if (failed)
pg_prop_fail(tc, "SQL error for: %s\n error: %s",
sql, err ? err : "(unknown)");
if (res == NULL)
pg_prop_fail(tc, "unexpected NULL result for: %s", sql);
val = atoll(res);
📄 src/test/prop/prop_mathfn.c (L200-L200)
The floor/ceil bracketing invariant tested here duplicates prop_mathround.c::floor_le_ceil, which emits effectively identical SQL (floor(x) <= x <= ceil(x) AND ceil(x)-floor(x) IN (0,1)). The only difference is the input generator. prop_mathround.c's own header claims it is "deliberately distinct from ... mathfn (transcendental float8 functions)" and owns the "cross-function round/trunc/ceil/floor/sign algebra" -- but this property (and trunc_toward_zero below) are exactly that algebra, not transcendental functions. Per DRY/minimalism, keep ceil/floor/round/trunc algebra in mathround.c and limit mathfn.c to the transcendental identities (sqrt/exp/ln/power/degrees/radians) its header advertises. (medium confidence)
📄 src/test/prop/prop_mathfn.c (L296-L296)
Property name power_ln_exp_roundtrip is misleading: the body and comment test only exp(ln(x)), no power() is involved. Rename to e.g. ln_exp_roundtrip to match what is actually asserted. (high confidence)
📄 src/test/prop/prop_mathfn.c (L113-L114)
The benign-error substrings "cannot" and "invalid" are far too broad and risk silently discarding genuine failures (e.g. a malformed query yielding a syntax/"invalid" error, or any unrelated "cannot ..." message would be swallowed as a skip instead of failing the property). For the inputs here (x in [0.01,100] for exp/ln, bounded x/k for power) only "out of range"/"overflow" can legitimately occur -- ln never sees a non-positive argument. Narrow the match to the overflow/out-of-range cases that can actually arise, matching the tighter prop_float.c version. (medium confidence)
📄 src/test/prop/prop_rangeagg.c (L144-L148)
Stale comment: the leading doc comment says "the half-open int4range(lo, hi) excludes hi, so test p = lo which is always a member," but the SQL below actually uses the inclusive '[]' aggregate (and the inner comment on lines 161-165 correctly describes the '[]' variant). Comments must describe what the code does now; this first paragraph contradicts the code it sits above. Reword it to match the '[]' aggregate actually used.
📄 src/test/prop/prop_upsert.c (L390-L390)
The property name multi_insert_last_wins contradicts what the test actually asserts. The body (and its own comment) require the statement to FAIL with the "ON CONFLICT DO UPDATE command cannot affect row a second time" error; "last wins" implies the opposite (the second proposed value survives). Since this identifier surfaces in test output and the example database as upsert/multi_insert_last_wins, the misleading name will cost a maintainer time when triaging a failure. Rename to reflect the real invariant, e.g. multi_insert_same_key_errors.
💡 Suggested change
Before:
PG_PROP(multi_insert_last_wins)
After:
PG_PROP(multi_insert_same_key_errors)
📄 src/test/prop/prop_mvcc.c (L217-L218)
This property's body is an exact duplicate of savepoint_rollback_partial in prop_savepoint.c (BEGIN; INSERT A; SAVEPOINT s; INSERT B; ROLLBACK TO s; COMMIT; assert net +1). The two files are documented as covering distinct oracles (snapshot visibility here vs. subtransaction nesting/undo there), yet this test exercises exactly the subtransaction-undo behaviour prop_savepoint.c owns, not snapshot visibility. Per the minimalism discipline (overlapping coverage), consider dropping this case from the MVCC area so each oracle is tested in exactly one place. (low confidence that it's intended to stay here.)
📄 src/test/prop/prop_range.c (L43-L45)
This doc comment contradicts both the code below it and the inline comment at the default: case. Two inaccuracies:
- "we do not force lo < hi" is false: the
defaultbranch swaps whenlo > hi, forcinglo <= hi. - "when a >= b this yields the empty range" is wrong for
a > b:int4range(a, b)witha > braisesrange lower bound must be less than or equal to range upper bound(it does not yield the empty range). That error is precisely why the swap exists, as the inline comment at thedefault:case correctly states.
Comments must describe what the code does now. Please reword to match the swap-and-order behavior (e.g. the bounded shape orders the ends so lo <= hi; lo == hi is the empty range, lo > hi would error).
📄 src/test/prop/prop_orderbynulls.c (L217-L231)
The three pg_prop_sql_exec calls here ignore their boolean return value, unlike the sibling helper run_three_scan in prop_indexscan.c which does if (!pg_prop_sql_exec(...)) pg_prop_fail(...). If any of these SETs fails (typo, server rejects the GUC, etc.), the test proceeds silently: the "forced index scan" run is then not actually forced, so the property degenerates to comparing two seqscan+sort results and would pass even if the index-ordering path were broken -- defeating the purpose of this property. Check the return value and pg_prop_fail on failure, as prop_indexscan.c does. (high confidence)
💡 Suggested change
Before:
/* Forced seqscan + sort. */
pg_prop_sql_exec(conn, "SET enable_indexscan = off; "
"SET enable_indexonlyscan = off; "
"SET enable_bitmapscan = off; "
"SET enable_seqscan = on", &err);
r_sort = pg_prop_sql_rows(conn, q, &f1, &e1);
/* Forced index scan. */
pg_prop_sql_exec(conn, "SET enable_indexscan = on; "
"SET enable_indexonlyscan = on; "
"SET enable_seqscan = off; "
"SET enable_sort = off", &err);
r_index = pg_prop_sql_rows(conn, q, &f2, &e2);
pg_prop_sql_exec(conn, "RESET ALL", &err);
After:
/* Forced seqscan + sort. */
if (!pg_prop_sql_exec(conn, "SET enable_indexscan = off; "
"SET enable_indexonlyscan = off; "
"SET enable_bitmapscan = off; "
"SET enable_seqscan = on", &err))
pg_prop_fail(tc, "forced-sort GUC set failed: %s", err ? err : "");
r_sort = pg_prop_sql_rows(conn, q, &f1, &e1);
/* Forced index scan. */
if (!pg_prop_sql_exec(conn, "SET enable_indexscan = on; "
"SET enable_indexonlyscan = on; "
"SET enable_seqscan = off; "
"SET enable_sort = off", &err))
pg_prop_fail(tc, "forced-index GUC set failed: %s", err ? err : "");
r_index = pg_prop_sql_rows(conn, q, &f2, &e2);
pg_prop_sql_exec(conn, "RESET ALL", &err);
📄 src/test/prop/prop_orderbynulls.c (L218-L231)
err is reused across all three pg_prop_sql_exec calls (and the two e1/e2 from the row queries). Since pg_prop_sql_exec pg_strdup's into *errmsg on failure, a failure in an earlier call followed by a failure in a later call overwrites and leaks the first err string. Minor given the per-case connection lifetime, but combined with the ignored return values above it is a real resource/clarity issue. (moderate confidence)
📄 src/test/prop/prop_nullsemantics.c (L71-L76)
This require_true helper is duplicated verbatim across ~200 prop_*.c files (several even carry a "Copied from prop_numeric.c" note). Since both reviewed files include pg_prop.h and link pg_prop.c/pg_prop_sql.c, this boilerplate oracle belongs in the shared framework (e.g. a pg_prop_require_true() in pg_prop_sql.c) rather than copy-pasted per file. Same applies to require_same_rows in prop_orderbynulls.c. Not blocking these two files specifically, but worth hoisting given the scale. (moderate confidence)
📄 src/test/prop/prop_mvcc.c (L349-L350)
This property does not exercise MVCC/snapshot visibility at all; it only asserts that count(*) is independent of an ORDER BY in a subquery. That is a trivial relational identity that holds on any SQL engine and would still pass with MVCC entirely reverted, so it cannot catch a visibility regression. The file header claims every property "exercises the snapshot/visibility machinery a single backend relies on" and that a mismatch "is a visibility bug" -- this one contradicts that. Either drop it or restructure it so the oracle actually depends on snapshot semantics (confidence: high).
📄 src/test/prop/prop_mvcc.c (L321-L324)
As the comment itself concedes ("Single-session this is trivially true"), with one connection and no concurrent writer the two reads cannot diverge regardless of whether REPEATABLE READ snapshot reuse works. This test would pass with the RR snapshot path broken, so it does not actually guard the invariant it names. A single-session harness cannot test snapshot stability against concurrent change; concurrency belongs in src/test/isolation. Consider removing or moving (confidence: high).
📄 src/test/prop/prop_savepoint.c (L133-L134)
This trailing cleanup DELETE is dead work: every property starts with build_sv_t(), which issues DROP TABLE IF EXISTS sv_t, so any residual id>=1000 rows are already gone at the next case. It is also not a reliable cleanup, since a pg_prop_fail() earlier in the case longjmps over it. The same redundant line is repeated in all six properties. Recommend removing it (confidence: high).
📄 src/test/prop/prop_nullsemantics.c (L272-L273)
The block comment claims the two integers are made distinct by "redrawing k until x <> k", but the code does not redraw. It instead adds a non-zero offset (k = x + draw(1,10)), and the inline comment on the next line even states "without a reject loop". The header comment contradicts the code. Per the project's comment-accuracy rule, comments must describe what the code does now.
Suggest aligning the header with the actual strategy, e.g. "so x is guaranteed not to equal the only non-NULL list element by adding a non-zero offset (no reject loop)". (high confidence)
💡 Suggested change
Before:
* two distinct non-NULL integers x and k (redrawing k until x <> k) so x is
* guaranteed not to equal the only non-NULL list element, then require
After:
* two distinct non-NULL integers x and k (k is x plus a non-zero offset, so no
* reject loop) so x is guaranteed not to equal the only non-NULL list element,
* then require
📄 src/test/prop/prop_orderbynulls.c (L283-L283)
The property is named reverse_all_keys, but neither the comment nor the SQL below has anything to do with reversing or "all keys": it asserts the first row of ORDER BY a NULLS LAST, id has a = min(a). PG_PROP(ident) registers CppAsString(ident) as the property name, so reverse_all_keys is exactly what appears in TAP output and as the subcommand on failure. This looks like a stale copy-paste name that will mislead anyone debugging a reported failure. Rename it to match its behavior (e.g. first_row_is_min).
Also note this property is nearly identical to the following limit1_is_min: both guard with count(a)=0 and compare the LIMIT 1 row's a to min(a); the only difference is = vs IS NOT DISTINCT FROM, which are equivalent once the CASE guard excludes the all-null/empty case (both sides are then guaranteed non-NULL). Consider dropping one of the two. (high confidence on the name mismatch)
💡 Suggested change
Before:
PG_PROP(reverse_all_keys)
After:
PG_PROP(first_row_is_min)
📄 src/test/prop/prop_textsplit.c (L127-L130)
This comment has drifted from the code. It describes a CASE comparing the two forms with IS NOT DISTINCT FROM to handle k > npieces/NULL array elements, but the generated SQL below uses a plain = and k is bounded to 1..npieces (so k never exceeds the last field). The CASE/IS NOT DISTINCT FROM discussion describes code that is not present and should be removed; the opening "k in 1..5" is also inaccurate since k is drawn 1..npieces. (medium confidence)
💡 Suggested change
Before:
* to npieces where both are a real field, and for k beyond we rely on
* properties 5/6; here the CASE compares the two forms with IS NOT DISTINCT
* FROM so a NULL array element (k > npieces) matches split_part's '' only
* when both agree. To keep it sound we bound k to the drawn piece count.
After:
* to npieces, so both always index a real, equal field; the array form is
* 1-based and matches split_part's 1-based part number. Out-of-range part
* numbers are covered separately by properties 5/6.
📄 src/test/prop/prop_textsplit.c (L151-L153)
Unlike the sibling files in this change (prop_strpos.c, prop_textord.c, prop_textpos.c) and the existing prop_text.c/prop_strfunc.c/prop_substring.c, this file embeds drawn strings directly with '%s' instead of using the defensive append_sql_literal() that doubles embedded quotes. It is safe only because draw_pieces() emits exclusively [a-c] and commas; if the alphabet ever gains a ', every such query becomes malformed. Prefer the uniform append_sql_literal() discipline so the generated SQL is well-formed regardless of alphabet. (medium confidence)
📄 src/test/prop/prop_pglsn.c (L41-L42)
The cannot substring match is far too broad and will silently swallow unrelated genuine failures (e.g. cannot execute ... in a read-only transaction, lock/permission errors, or any message containing the word "cannot"), turning real bugs into skipped cases via pg_prop_assume(false).
Moreover, it is unnecessary for the stated purpose. The actual overflow raised by numeric_pg_lsn is pg_lsn out of range (already caught by out of range). The NaN guards in pg_lsn_pli/pg_lsn_mii (cannot add NaN to pg_lsn / cannot subtract NaN from pg_lsn) can never fire here, because these tests only feed pg_lsn - pg_lsn results and finite ::numeric literals -- never NaN. Every other is_overflow helper in this suite (prop_cash.c, prop_date.c, prop_interval.c, ...) matches only on out of range/overflow.
Narrow the match to the specific overflow/NaN tokens (or drop the NaN branch entirely, since it is unreachable in these properties). Confidence: high.
💡 Suggested change
Before:
return strstr(errmsg, "out of range") != NULL ||
strstr(errmsg, "cannot") != NULL;
After:
return strstr(errmsg, "out of range") != NULL ||
strstr(errmsg, "cannot add NaN") != NULL ||
strstr(errmsg, "cannot subtract NaN") != NULL;
📄 src/test/prop/prop_tsrank.c (L80-L84)
When allow_neg is false, maxkind is capped at 2, so pg_prop_draw_int(tc, 0, maxkind) only ever returns 0, 1, or 2. kind==1 is remapped to a leaf and kind==2 is conjunction, so the default: disjunction branch is unreachable in positive mode. The three properties that call draw_query(..., false) (rank_positive_when_match, rank_cd_zero_when_no_match) therefore never exercise OR-queries, silently narrowing coverage. To include disjunctions in positive mode, draw across the full set and remap only negation, e.g. draw 0..3 and map kind==1 to 0 when !allow_neg (which the existing remap already does), leaving maxkind at 3. (moderate confidence)
💡 Suggested change
Before:
maxkind = allow_neg ? 3 : 2;
if (depth >= 3)
kind = 0;
else
kind = (int) pg_prop_draw_int(tc, 0, maxkind);
After:
if (depth >= 3)
kind = 0;
else
kind = (int) pg_prop_draw_int(tc, 0, 3);
📄 src/test/prop/prop_tsrank.c (L352-L352)
The function name rank_weights_monotone does not describe what the body checks: it tests that the 2-arg ts_rank equals the 4-arg ts_rank with the explicit default weight array {0.1,0.2,0.4,1.0} -- an equivalence, not monotonicity. The comment itself even disclaims monotonicity. Rename to reflect the actual invariant, e.g. rank_default_weights_equivalence. (Verified: default_weights = {0.1f,0.2f,0.4f,1.0f} in src/backend/utils/adt/tsrank.c and the 3-arg signature is ts_rank(_float4, tsvector, tsquery) per pg_proc.dat, so the SQL and argument order are correct; only the name is misleading. high confidence)
💡 Suggested change
Before:
PG_PROP(rank_weights_monotone)
After:
PG_PROP(rank_default_weights_equivalence)
📄 src/test/prop/prop_tsvector.c (L6-L8)
Same motivation-comment concern as prop_tsquery.c: "tsvector concatenation, setweight, strip, and length (tsvector_op.c) have had fixes" is an unverifiable, aspirational-sounding claim with no reference to the change/thread. Drop it or restate as what the tests cover. (low confidence)
📄 src/test/prop/prop_tablesample.c (L200-L203)
The two NOT EXISTS blocks are byte-for-byte identical: both compute (sample REPEATABLE(7)) EXCEPT (sample REPEATABLE(7)) in the same left-to-right order. The comment claims "each direction of EXCEPT is empty", but only one direction (A EXCEPT B) is tested -- twice. Since EXCEPT is not symmetric, a non-deterministic sample where the first evaluation's id set is a strict subset of the second's would make both blocks empty and the test would still pass, failing to catch the non-determinism. The second block should swap the operands to test the reverse direction. As written, the second NOT EXISTS is redundant and the invariant is only half-checked. (moderate confidence)
💡 Suggested change
Before:
"AND NOT EXISTS ("
"(SELECT id FROM ts_t TABLESAMPLE BERNOULLI(50) REPEATABLE(7)) "
"EXCEPT "
"(SELECT id FROM ts_t TABLESAMPLE BERNOULLI(50) REPEATABLE(7)))");
After:
"AND NOT EXISTS ("
"(SELECT id FROM ts_t TABLESAMPLE BERNOULLI(50) REPEATABLE(7)) "
"EXCEPT "
"(SELECT id FROM ts_t TABLESAMPLE BERNOULLI(50) REPEATABLE(7)))");
/* NB: the second block must reverse the operands to test the other
* direction; as written it duplicates the first and leaves A != B,
* A (subset of) B undetected. */
📄 src/test/prop/prop_pglsn.c (L53-L63)
The raw out-parameter (and the *raw = ... write) is dead code: all nine call sites pass NULL, so the raw value is never observed and no "C-side oracle" exists. Per the minimalism/YAGNI discipline, drop the unused parameter and compute raw only when a caller actually needs it. The header comment also claims the output is zero-padded ("e.g. '1A2B/00C3'"), but %X emits no leading zeros (the low half of 0xC3 prints as C3, not 00C3) -- the comment should match the code.
💡 Suggested change
Before:
static void
draw_lsn(pg_prop_tc *tc, StringInfo buf, uint64 *raw)
{
uint32 hi = (uint32) pg_prop_draw_int(tc, 0, 4294967295);
uint32 lo = (uint32) pg_prop_draw_int(tc, 0, 4294967295);
appendStringInfo(buf, "'%X/%X'::pg_lsn", hi, lo);
if (raw)
*raw = ((uint64) hi << 32) | (uint64) lo;
}
After:
static void
draw_lsn(pg_prop_tc *tc, StringInfo buf)
{
uint32 hi = (uint32) pg_prop_draw_int(tc, 0, 4294967295);
uint32 lo = (uint32) pg_prop_draw_int(tc, 0, 4294967295);
appendStringInfo(buf, "'%X/%X'::pg_lsn", hi, lo);
📄 src/test/prop/prop_timeops.c (L4-L4)
The header claims coverage of timetz, but no property in this file constructs or exercises a timetz (TIME WITH TIME ZONE) value -- the only timetz token in the whole file is this comment. All 8 properties use only time and plain timestamp. Comments must describe what the code does now; drop the timetz claim (and the matching "timestamptz"-distinction note is fine, but timetz here is inaccurate). [moderate confidence]
s/time / timetz / timestamp/time / timestamp/
💡 Suggested change
Before:
* Property-based tests for time / timetz / timestamp (without time zone).
After:
* Property-based tests for time / timestamp (without time zone).
📄 src/test/prop/prop_unicode_case.c (L7-L7)
Non-ASCII character in source. The sharp-s ß (U+00DF) in this comment violates the project's ASCII-only rule for source and diffs. git diff --check and header checks aside, PostgreSQL comments must be plain ASCII. Replace with an ASCII spelling.
💡 Suggested change
Before:
* length-based, and a classic bug site: special mappings (ß, final sigma),
After:
* length-based, and a classic bug site: special mappings (sharp s, final sigma),
📄 src/test/prop/prop_unicode_norm.c (L121-L124)
unicode_normalize() returns NULL on OOM in frontend builds (as this file's own header comment states), but a/b here (and direct/nfd/viaD below) are passed straight into cp_equal()/cp_show() and dereferenced without a NULL check. On allocation failure this is a NULL dereference. Given the acknowledged contract, guard the results (e.g. pg_prop_assume(tc, a != NULL && b != NULL) or skip the case) before comparing. This applies to all four properties in this file.
| # Needs repo variables AWS_ROLE_ARN, AWS_REGION, OCR_BEDROCK_MODEL. Auth is | ||
| # GitHub OIDC; no static AWS keys. See the ci-workflows README. | ||
|
|
||
| name: OCR AI Review |
There was a problem hiding this comment.
OIDC auth will fail: this caller grants no id-token: write. A called/reusable workflow can only restrict the permissions the caller passes, never expand them, and the default GITHUB_TOKEN never includes id-token. Since this stub relies on AWS OIDC (vars.AWS_ROLE_ARN), the role-assumption step in ocr-review.yml will fail at runtime unless the caller declares the permission. Add a least-privilege block at the top of this workflow (id-token: write for OIDC, plus pull-requests: write/issues: write/contents: read as the review needs). Omitting an explicit block also leaves the token at the broad repo default, which is wrong for a workflow reacting to untrusted PR/comment events.
(high confidence on the id-token requirement; verify the exact scopes against ci-workflows' needs.)
| name: OCR AI Review | |
| name: OCR AI Review | |
| # Least-privilege; id-token is required for AWS OIDC and cannot be granted by | |
| # the reusable workflow (callees can only restrict, never expand). | |
| permissions: | |
| contents: read | |
| id-token: write | |
| pull-requests: write | |
| issues: write |
| (startsWith(github.event.comment.body, '/open-code-review') || | ||
| startsWith(github.event.comment.body, '@open-code-review') || | ||
| startsWith(github.event.comment.body, '/pg-history'))) | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-review.yml@v1 |
There was a problem hiding this comment.
gburd/ci-workflows is an external repository and @v1 is a mutable tag that can be force-moved to point at different code. Combined with the issue_comment trigger (anyone who can comment /open-code-review can start a run) and AWS OIDC credentials in the call chain, a compromised/retargeted tag could execute attacker-controlled logic with the fork's cloud credentials. Pin the reusable workflow to an immutable commit SHA (e.g. gburd/ci-workflows/.github/workflows/ocr-review.yml@<sha>) and keep a comment noting which v1 it corresponds to. (high confidence)
| uses: gburd/ci-workflows/.github/workflows/ocr-review.yml@v1 | |
| uses: gburd/ci-workflows/.github/workflows/ocr-review.yml@<commit-sha> # tracks v1 |
|
|
||
| on: | ||
| pull_request: | ||
| # Note: no draft filter — drafts are reviewed too. |
There was a problem hiding this comment.
The project standard for this fork is ASCII-only in source and diffs (no em-dashes). This comment uses a Unicode em-dash. Replace with an ASCII hyphen/double-hyphen.
| # Note: no draft filter — drafts are reviewed too. | |
| # Note: no draft filter -- drafts are reviewed too. |
| # when OCR_BEDROCK_MODEL is behind. Notify-only (GITHUB_TOKEN cannot write | ||
| # Actions variables). Logic lives in gburd/ci-workflows. | ||
|
|
||
| name: OCR model self-check |
There was a problem hiding this comment.
Same OIDC gap as the review stub: this caller declares no permissions: block, so no id-token: write is granted. The reusable ocr-model-check.yml cannot grant id-token itself (callees only restrict), so the AWS OIDC role assumption via vars.AWS_ROLE_ARN will fail at runtime. Add an explicit least-privilege block including id-token: write (plus issues: write since the comment says it opens/updates an issue, and contents: read). (high confidence on id-token.)
| name: OCR model self-check | |
| name: OCR model self-check | |
| permissions: | |
| contents: read | |
| id-token: write | |
| issues: write |
|
|
||
| jobs: | ||
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
There was a problem hiding this comment.
External reusable workflow pinned to the mutable tag @v1. As with the review stub, pin to an immutable commit SHA so a force-moved tag cannot inject code into a run that holds AWS OIDC credentials.
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@<commit-sha> # tracks v1 |
| #ifndef pg_attribute_constructor | ||
| #define pg_attribute_constructor __attribute__((constructor)) | ||
| #endif |
There was a problem hiding this comment.
Macro-ordering hazard: PG_PROP(ident) above already expands pg_prop_reg_##ident(void) pg_attribute_constructor;, but the fallback #define pg_attribute_constructor ... is defined here, after the PG_PROP definition. Since PG_PROP is only expanded in area files that include this header in full, this happens to work, but defining the fallback after its first textual use in the same header is fragile. Move the pg_attribute_constructor fallback definition above the PG_PROP macro definition.
| jobs: | ||
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
There was a problem hiding this comment.
Beyond the missing id-token: write for OIDC (already noted), this stub also needs issues: write to fulfill its stated purpose ("opens/updates one issue here"). Caller permissions flow into the reusable workflow and can only be restricted there, never expanded. If the repo's default GITHUB_TOKEN is set to read-only (the recommended/common hardened setting), the issue create/update in ocr-model-check.yml will fail with 403. Add an explicit least-privilege permissions: block granting id-token: write and issues: write.
| jobs: | |
| check: | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 | |
| permissions: | |
| id-token: write | |
| issues: write | |
| jobs: | |
| check: | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
| /* categories: non-NULL empty list -> empty base alphabet */ | ||
| (const char *const *) &alphabet, 0, |
There was a problem hiding this comment.
Fragile and self-contradictory alphabet restriction. (const char *const *) &alphabet takes the address of the local alphabet parameter and reinterprets a char * as a char ** array of category-name strings; passing it with count 0 means the ABI will not read it, but the inline comment claims "non-NULL empty list -> empty base alphabet" -- it is not a list of category names at all, it is &(a char pointer). If the libhegel ABI distinguishes a non-NULL categories pointer with count 0 from NULL (e.g. treats non-NULL as "explicit empty category set"), the semantics are undefined/accidental; if it only looks at count, the cast is pointless and misleading. Either pass NULL for the categories pointer (matching count 0) or pass a real const char * array, and fix the comment. As written, a wrong interpretation would let out-of-alphabet characters through and produce spurious test failures.
| jobs: | ||
| review: |
There was a problem hiding this comment.
In addition to the missing id-token: write for OIDC (already noted), a PR-review bot that posts comments needs pull-requests: write (and typically contents: read). Reusable workflows inherit the caller's permissions and can only narrow them. With no permissions: block here, if the repo default GITHUB_TOKEN is read-only the review step will fail to post. Declare an explicit least-privilege block, e.g. id-token: write, pull-requests: write, contents: read.
| jobs: | |
| review: | |
| permissions: | |
| id-token: write | |
| contents: read | |
| pull-requests: write | |
| jobs: | |
| review: |
| "SELECT (SELECT string_agg(v::text, ',' ORDER BY v, id) FROM ao_t) " | ||
| "IS NOT DISTINCT FROM " | ||
| "(SELECT string_agg(v::text, ',' ORDER BY v, id) FROM ao_t)"); |
There was a problem hiding this comment.
This property runs the byte-identical query on both sides of IS NOT DISTINCT FROM, so it is tautological: a deterministic aggregate over a fixed table always returns the same value regardless of whether the in-aggregate ORDER BY works. The test would still pass if the ORDER BY were reverted/broken (as long as it stayed broken consistently), so it cannot catch any regression in aggregate ordering. To actually exercise ordering, compare against an independent oracle (e.g. string_agg over an externally pre-ordered subquery, as array_agg_order_matches does in prop_stringagg.c) rather than the same query.
1 participant
Summary
An optional, opt-in property-based testing (PBT) layer for PostgreSQL built on the Hegel engine (libhegel), living entirely under
src/test/prop. Off by default, gated on three conditions (explicit configure flag--with-hegel/-Dprop_tests=enabled, the engine being present, and TAP tests enabled); if any piece is missing under the opt-in flag it is a hard configure error, and with the flag off nothing changes.This is a large series (one commit per area) opened primarily so CI runs the suite. Nothing in the server is modified; this only adds a test tree plus the build plumbing to wire it in.
What's here
pg_prop.c/.h,pg_prop_sql.c): a small plain-C wrapper over libhegel's C ABI, aPG_PROP()registry, TAP output, a fork-basedisolate()for code that may crash, and libpq oracle helpers. CLI:pg_prop <area> [-n iters] [-s seed] [property ...].t/001_prop.pl, 12 areas: path, jsonapi, encoding, unicode_case/norm, pglz, base64, prng, crypto, scram, binaryheap, bitutils). Tier B drives SQL against a live node via libpq (t/002_sql_props.pl, ~246 areas).src/test/prop/sql/): a shared random-table + predicate generator feeding metamorphic oracles that test the planner and executor rather than individual functions:query_tlp,query_tlp_agg,query_tlp_distinct-- Ternary Logic Partitioning (rows, aggregates, DISTINCT/set-ops)query_norec-- non-optimizing reference, including across a joinquery_cardinality-- result-size monotonicity lawsquery_pqs-- Pivoted Query Synthesis (single query vs single known-true fact)query_planadvice-- the same query run free vs steered viacontrib/pg_plan_advice(scan/join method, join order, merge join, spill); result must not depend on the chosen planquery_gen.setup/column/predicate), after which every query-level oracle covers that type with no new oracle code.sql/prop_query_ext_citext.cis a complete worked example.Validation
Full TAP suite green on a cassert build: 12 Tier-A + 246 Tier-B subtests, zero warnings, run under the TAP driver's own seed stream. On a branch-coverage build the query-level areas drive real planner/executor coverage (e.g.
clausesel.c~72% branches; a single plan-advice property tooknodeMergejoin.cfrom 0% to ~30% branches).See
src/test/prop/READMEandsrc/test/prop/DESIGNfor the full rationale, the when-to-use/when-not guidance, the measured coverage story, and the extension mechanism.No server bug is fixed or reported here; findings surfaced during development are property/oracle-soundness issues recorded out of tree.