implement pg_proc table#2902
Conversation
|
|
SummaryThis run exercises database routine metadata and definition lookup behavior across normal catalog introspection, argument-shape edge cases, and function-body merge handling for different quoting styles. Core routine discovery and definition round-trips look healthy, but security-boundary and metadata-consistency checks exposed important gaps. Not safe to merge yet — this PR has attributable failures that include a high-severity authorization exposure plus an additional medium-severity catalog correctness defect, indicating real user-facing risk beyond minor edge noise. Because these are tied to this branch and affect both access control and metadata reliability, they are merge blockers rather than follow-up-only caveats. Tests run by ItoTip Reply with @itoqa to send us feedback on this test run. |
|
Hydrocharged
left a comment
There was a problem hiding this comment.
LGTM! Strange that we have such a large regression, I'm not seeing anything that would point to that being the case
|
Diff SummaryThis run focused on database routine metadata behavior, covering normal and boundary signatures to confirm argument-name alignment, null handling, and compatibility expectations for metadata consumers. It also exercised a permission-boundary abuse path around routine-definition lookup, while overall behavior tied to this change remained stable. Safe to merge — the exercised areas for this PR’s metadata changes passed, with no regressions, no new PR-attributable failures, and no previously flagged PR-owned failures still failing. One high-severity failure appeared as an unrelated additional finding in an existing authorization path, so it is a flag-for-later issue rather than a merge blocker for this PR. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Low-privilege users can read routine source via pg_get_functiondef
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
Diff SummaryThis run covered database catalog and type-system behavior, including normal subscript and transaction-stability paths plus tougher metadata queries with joins and casts that many tools rely on for introspection. Core subscript behavior looked healthy, but catalog OID/regproc handling showed instability under realistic query patterns. Not safe to merge yet — this PR introduces multiple new attributable failures, including high-severity crashes in common metadata-introspection paths and several related medium-severity failures from the same type-handling area. There is also an unrelated pre-existing high-severity authorization issue noted as a caveat, but the merge-blocking risk comes from the new PR-attributable failures. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Low-privilege role can read routine source
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
Diff SummaryThis run focused on database catalog compatibility behaviors, covering normal metadata lookups and tougher edge cases like casting, filtering, and joining handler-related fields across system tables. Basic routine and support-column reads were stable, but many catalog paths that expose handler metadata now fail with runtime crashes or inconsistent cast behavior instead of returning reliable results. Not safe to merge yet — this PR is tied to a high-severity crash plus multiple medium-severity new and still-unfixed failures in core catalog introspection paths, indicating broad instability in behavior the change touched. There is also one unrelated additional finding to track separately, but the attributable failures alone are merge-blocking. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Low-privilege role can read routine source via pg_get_functiondef
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
Diff SummaryThis run heavily exercised database system-catalog behavior around function-handler identifiers, including normal metadata reads, legacy compatibility patterns, type-casting edge cases, and repeated-read stability checks. It also covered adversarial access-boundary behavior for routine-definition lookup, while overall core catalog introspection behavior remained largely stable. Merge with caution — there is one PR-attributable medium-severity new failure indicating a backward-compatibility break in a legacy metadata lookup path introduced by this change. A separate high-severity failure was observed but is marked unrelated to this PR, so it is a follow-up caveat rather than a merge blocker for this branch. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Low-privilege role can read private routine source via pg_get_functiondef
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
Diff SummaryThis run exercises core database catalog behavior around type and function metadata, casting, cross-catalog joins, and compatibility-oriented introspection patterns, including both normal read paths and edge-case type/coercion boundaries. It also covers an adversarial permission boundary scenario for routine-definition visibility, giving a broad signal on metadata correctness, stability, and access-control behavior. Safe to merge — the observed failures are non-attributable additional findings rather than regressions introduced by this PR, and the change shows stable behavior across the exercised catalog and type-compatibility paths. There are pre-existing medium/high-impact product issues worth tracking separately, but they are not merge blockers for this PR. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Low-privilege role can read routine source via pg_get_functiondef
Evidence Package🟡 Cross-catalog pg_type to pg_proc identity join is incoherent
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
Diff SummaryThis run exercised database catalog behavior and type-handling compatibility across normal metadata queries, casting and conversion edge cases, and planner consistency checks, with broad coverage of upgrade and access-control scenarios. Most compatibility and stability paths held, but one planner-path consistency gap appeared under equivalent query forms. Merge with caution — this PR introduces a medium-severity regression in query planning consistency for equivalent predicates, which creates real performance risk even though core functionality remains largely intact. There are also unrelated additional findings, including high-severity issues, but those are not attributable to this PR and should be tracked separately. Tests run by ItoTests that are no longer relevantBelow are tests that previously ran and are no longer relevant:
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Low-privilege role can read routine source without EXECUTE privilege
Evidence Package🟠 Upgrade path pg_proc queries panic
Evidence Package🟡 pg_type typinput OIDs do not resolve in pg_proc
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
There was a problem hiding this comment.
🔁 Regression: previously passing at 6c2437a
Equivalent catalog join predicates take inconsistent planner paths
What failed: The optimizer applies different join strategies to semantically equivalent predicates, indicating cast handling occurs before a stable equality-join canonicalization step.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Queries using this explicit cast path can be planned with a less efficient join strategy, which may cause noticeably slower catalog lookups under realistic data volumes.
- Steps to Reproduce:
- Run
EXPLAIN ANALYZE SELECT t1.typname, p1.proname FROM pg_type AS t1, pg_proc AS p1 WHERE t1.typinput = p1.oid ORDER BY t1.typname, p1.proname LIMIT 5;. - Run
EXPLAIN ANALYZE SELECT t1.typname, p1.proname FROM pg_type AS t1, pg_proc AS p1 WHERE t1.typinput::oid = p1.oid ORDER BY t1.typname, p1.proname LIMIT 5;. - Compare plans: implicit form uses
HashJoinwithHashLookup, explicit-cast form usesCrossJoinwithFilterdespite equivalent predicate semantics.
- Run
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In
server/types/type.go, the PR changed(*DoltgresType).Equalsto strict serialized-byte equality only and removed the previousidType()shortcut path. That changed-code path can alter when the analyzer treats id-backed types as compatible, which aligns with the observed split: the implicit variant remains on a hash-join shortcut path while the explicit cast variant is left as a filter over cross join. Smallest practical fix: normalize explicit::oidequality predicates into the same canonical join-equality form used by implicit id-backed comparisons (or restore a narrowly scoped id-type compatibility check for join-key recognition) so both query spellings take the same planner path. - Why this is likely a bug: The two predicates are semantically equivalent comparisons on the same join keys, so planner strategy should not diverge purely because one side is explicitly cast to the target type. This inconsistency points to a planning/cast-ordering defect rather than expected query semantics.
Relevant code
server/types/type.go:583-588
// Equals implements the types.ExtendedType interface.
func (t *DoltgresType) Equals(otherType sql.Type) bool {
if otherExtendedType, ok := otherType.(*DoltgresType); ok {
return bytes.Equal(t.Serialize(), otherExtendedType.Serialize())
}
return false
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Equivalent catalog join predicates take inconsistent planner paths**
**What failed:** The optimizer applies different join strategies to semantically equivalent predicates, indicating cast handling occurs before a stable equality-join canonicalization step.
- **Impact:** Queries using this explicit cast path can be planned with a less efficient join strategy, which may cause noticeably slower catalog lookups under realistic data volumes.
- **Steps to reproduce:**
1. Run `EXPLAIN ANALYZE SELECT t1.typname, p1.proname FROM pg_type AS t1, pg_proc AS p1 WHERE t1.typinput = p1.oid ORDER BY t1.typname, p1.proname LIMIT 5;`.
2. Run `EXPLAIN ANALYZE SELECT t1.typname, p1.proname FROM pg_type AS t1, pg_proc AS p1 WHERE t1.typinput::oid = p1.oid ORDER BY t1.typname, p1.proname LIMIT 5;`.
3. Compare plans: implicit form uses `HashJoin` with `HashLookup`, explicit-cast form uses `CrossJoin` with `Filter` despite equivalent predicate semantics.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In `server/types/type.go`, the PR changed `(*DoltgresType).Equals` to strict serialized-byte equality only and removed the previous `idType()` shortcut path. That changed-code path can alter when the analyzer treats id-backed types as compatible, which aligns with the observed split: the implicit variant remains on a hash-join shortcut path while the explicit cast variant is left as a filter over cross join. Smallest practical fix: normalize explicit `::oid` equality predicates into the same canonical join-equality form used by implicit id-backed comparisons (or restore a narrowly scoped id-type compatibility check for join-key recognition) so both query spellings take the same planner path.
- **Why this is likely a bug:** The two predicates are semantically equivalent comparisons on the same join keys, so planner strategy should not diverge purely because one side is explicitly cast to the target type. This inconsistency points to a planning/cast-ordering defect rather than expected query semantics.
**Relevant code:**
`server/types/type.go:583-588`
~~~go
// Equals implements the types.ExtendedType interface.
func (t *DoltgresType) Equals(otherType sql.Type) bool {
if otherExtendedType, ok := otherType.(*DoltgresType); ok {
return bytes.Equal(t.Serialize(), otherExtendedType.Serialize())
}
return false
}
~~~


No description provided.