schema: review-report-v2
slug: commitfest-7094-review
source: postgresql-master
target: CommitFest 7094 — coerce_type string-category->unknown passthrough
base: 534db08f972852b77ea5e96e77d5fe45f3d8df93
head: 534db08f972852b77ea5e96e77d5fe45f3d8df93
verdict: waiting-on-author
blocker_counts:
  verified: 1
  unverified: 0
model: glm-5.2:cloud
diff_sha: 2e02c9be6b35f40bee3d29e96c71e0132d44f9fc9289976d3e1568bd75d36d72

CommitFest 7094 Review — coerce_type string→unknown passthrough

Summary

The staged patch (10 lines, src/backend/parser/parse_coerce.c:415-422) adds a special case in coerce_type: when the target type is UNKNOWNOID and the input type's base category is TYPCATEGORY_STRING, return the node unchanged. This is the V1 approach proposed by Jian He on pgsql-hackers thread 635318 (2026-01-29), which Tom Lane explicitly objected to ("I certainly don't like the cast pathway you suggest adding to make it not be one — that seems likely to cause lots of not-very-desirable behaviors"). The author later moved to a V2 (2026-04-22) that disallows unknown targets in find_coercion_pathway instead. The staged patch is stale relative to thread consensus.

Verdict: waiting-on-author — 1 verified BLOCKER (F1: stale vs thread / live objection), 5 MAJOR findings, 1 coverage MAJOR (COV-1), 4 MINOR findings. The V2 repair (un-stage V1; add if (targetTypeId == UNKNOWNOID) return COERCION_PATH_NONE; in find_coercion_pathway) resolves all findings simultaneously and was validated by compile + full make installcheck (243/243 pass) + behavioral probes.

Patch under review

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index d3240f4b265..f75f808da3b 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -411,6 +411,16 @@ coerce_type(ParseState *pstate, Node *node,
 		}
 		return result;
 	}
+
+	if (targetTypeId == UNKNOWNOID)
+	{
+		Oid			inputBaseTypeId = getBaseType(inputTypeId);
+		TYPCATEGORY s_typcategory = TypeCategory(inputBaseTypeId);
+
+		if (s_typcategory == TYPCATEGORY_STRING)
+			return node;
+	}
+
 	pathtype = find_coercion_pathway(targetTypeId, inputTypeId, ccontext,
 									 &funcId);

Findings

Verification Summary

Counts: verified 11, cannot-verify 1, refuted 0. Verified BLOCKER split: 1/1.

Repair Validation Summary

All verified findings share a single substantive repair: the V2 find_coercion_pathway guard (un-stage V1 coerce_type block; add if (targetTypeId == UNKNOWNOID) return COERCION_PATH_NONE; in find_coercion_pathway).

• Patch sha256: 18b93f26a2257cd7f2c95b85c2ef12f800b5757c35d939c2f9acf00ee910397c
• applies_to_head: 534db08f972852b77ea5e96e77d5fe45f3d8df93
• Validation: passed — apply-check (exit 0), diff-check (byte-identical), compile (parse_coerce.o + postgres relink exit 0), targeted-test (psql probes: text::unknown→ERROR, int::unknown→ERROR, 'foo'::unknown→unknown, domain::unknown→ERROR), repro (baseline/V1/V2 comparison), full make installcheck 243/243 pass.
• F6 (docs): typed design-choice waiver (V2 introduces no new coercion rule; SGML placement deferred to doc maintainer).
• F9/F10/F11 (maintainability/performance): typed not-instrumentable waivers for test_example (inspection-only findings); validation includes static-check.

Changed-Code Coverage

Coverage requirement: required (1 changed instrumentable path: src/backend/parser/parse_coerce.c).

Execution lane: host-disposable (passed). Fresh coverage-instrumented build (CFLAGS=-O0 -g -fprofile-arcs -ftest-coverage) of V1-patched source in disposable scratch; initdb cluster on disposable port 55809; full pg_regress installcheck 243/243 tests passed; gcov -b -f parse_coerce.c collected with 813 runs.

Metrics

Identity check: covered (0) + compiler-uncovered (5) = changed executable (5) ✓. Uncovered (5) + structural (0) = compiler-uncovered (5) ✓.

Uncovered regions

coerce_type lines 415-421 — coverability: requires-special-harness-or-injection-point

Reason: The V1 branch is unreachable via the primary coerce_to_target_type path because can_coerce_type (line 88 gate) returns false for target=UNKNOWNOID before coerce_type is called. Reaching the branch requires a direct coerce_type caller that bypasses the can_coerce_type gate. This confirms finding F2: the branch is dead code.

Uncovered branches

Touched functions

Test examples

src/test/regress/sql/cast_unknown.sql (new) — exercises unknown::unknown, text::unknown (error), int4::unknown (error), domain-over-string ::unknown (error), unknown::text (pass). Fails on V1 (text::unknown returns text), passes on V2 (text::unknown errors). The V2 repair removes the dead branch entirely.

pgsql-hackers Thread Ledger

Status: reconciled. Ledger report: commitfest-7094-review-thread-ledger.

Thread "ERROR: failed to find conversion function from unknown to text" (Thread-ID 635318, pgsql-hackers):

Finding links: F1 → already-raised (Tom Lane objection); F2-F11, COV-1 → new (harness findings not previously raised).

Catversion exception: Development branch; no catversion bump required (no pg_cast/pg_type/pg_proc row changes).

Aggregated Test Plan

1. src/test/regress/sql/cast_unknown.sql (new) — content provided in F5/COV-1 repair test_example. Register serially after create_cast.
2. src/test/regress/expected/cast_unknown.out (new) — generate from a V2 make installcheck run per the expected-out approval rule.
3. Oracle: fail-on-V1 / pass-on-V2 for text::unknown; pass-on-baseline-and-V2 for unknown::unknown and unknown::text.

Verified-clean / non-findings

• No catalog version bump needed (no pg_proc/pg_type/pg_cast/pg_amop/pg_amproc row changes).
• No pg_dump impact (parser-only runtime change).
• No memory-context issue (getBaseType/TypeCategory use syscache; no palloc leak).
• No assertion crash reachable (TypeCategory asserts typcategory != INVALID; getBaseType(UNKNOWNOID) returns UNKNOWNOID; TypeCategory(UNKNOWNOID) returns TYPCATEGORY_UNKNOWN, not INVALID).
• No SQL injection / SECURITY DEFINER / search_path / catalog race surface.
• No C memory-safety surface (pure control-flow branch returning an existing node).

Provenance / Model Routes

• Model: glm-5.2:cloud (user-fixed override for all seats)
• Seats: pg-patch-reviewer, pg-security-reviewer, pg-test-author, pg-finding-verifier, pg-review-harness (coordinator)
• Coverage execution: host-disposable lane (fresh gcov epoch, 813 runs)

Residual Risk

• The staged V1 patch is dead code (0% coverage) and stale relative to thread consensus. If committed as-is, it has no observable effect through the normal coercion path but introduces a latent inconsistency for any future direct coerce_type caller.
• The V2 repair resolves all findings but requires the author to un-stage V1 and adopt the find_coercion_pathway guard. The V2 patch is a proposal; the author should confirm it matches their intent.

Maintainer CC

Jian He (author, jian.universality@gmail.com); Tom Lane (tgl@sss.pgh.pa.us, objector).