fix(db): Resolve the execution table through search_path in the revision guard - #929
Conversation
|
Warning Review limit reachedNext included review available in 30 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
This change may need patch-release backports. Comment with one of these commands to open a cherry-pick PR:
|
There was a problem hiding this comment.
🟡 Changes recommended
Add coverage for search_path = first_schema, later_schema with execution only in the later schema.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes the PostgreSQL revision guard so execution resolves consistently through search_path.
Changes:
- Uses
to_regclassandpg_attributefor relation/type detection. - Preserves idempotent widening to
bigint.
File summaries
| File | Summary |
|---|---|
make/migrations/postgresql/harbor_next.sql |
Corrects relation resolution; a moderate finding remains that the two-schema regression scenario lacks test coverage. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
be13377 to
e03143b
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="make/migrations/postgresql/harbor_next.sql">
<violation number="1" location="make/migrations/postgresql/harbor_next.sql:164">
P3: The new domain-preserving guard branch is untested. Add cases for a domain over `bigint`, which must remain unchanged, and a domain over `integer`, which must widen without losing the intended behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| AND a.attname = 'revision' | ||
| AND a.attnum > 0 | ||
| AND NOT a.attisdropped | ||
| AND CASE WHEN t.typtype = 'd' THEN t.typbasetype ELSE a.atttypid END |
There was a problem hiding this comment.
P3: The new domain-preserving guard branch is untested. Add cases for a domain over bigint, which must remain unchanged, and a domain over integer, which must widen without losing the intended behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At make/migrations/postgresql/harbor_next.sql, line 164:
<comment>The new domain-preserving guard branch is untested. Add cases for a domain over `bigint`, which must remain unchanged, and a domain over `integer`, which must widen without losing the intended behavior.</comment>
<file context>
@@ -143,14 +143,26 @@ BEGIN
AND a.attnum > 0
AND NOT a.attisdropped
- AND a.atttypid <> 'bigint'::regtype
+ AND CASE WHEN t.typtype = 'd' THEN t.typbasetype ELSE a.atttypid END
+ <> 'bigint'::regtype
) THEN
</file context>
…ion guard The guard added in #924 tested information_schema against current_schema(), which is only the first entry in search_path. On a connection whose search_path puts another schema first, an execution table in a later schema makes the predicate false while the unqualified ALTER below it resolves to that same later table, so the widening is skipped without a word and execution.revision silently stays integer. Resolve the relation with to_regclass and read its type from pg_attribute, so the guard and the ALTER always agree on which table they are talking about. to_regclass returns NULL when no execution relation is reachable, which leaves the guard false as before. Two narrower cases came out of review. to_regclass resolves any relation, so the guard is restricted to tables by relkind: an index named execution earlier in search_path would otherwise match a pg_attribute row and send ALTER TABLE at something it cannot alter, failing the whole file. And the column type is compared after resolving a domain to its base, so a column already typed as a domain over bigint keeps the domain and its CHECK constraints instead of having them stripped by the ALTER. Covered by two tests against a real Postgres: execution in a schema that is not first in search_path is widened and its rows preserved on two consecutive applies, and a non-table relation of that name leaves the apply clean rather than aborting it. Reported by the review bots on #924 and on this PR. Signed-off-by: Prasanth Baskar <prasanth@8gears.com>
e03143b to
8e01e5e
Compare
|
/backport v2.15 |
|
Backport to |
…ion guard (backport #929) The guard backported in #910 tested information_schema against current_schema(), which is only the first entry in search_path. On a connection whose search_path puts another schema first, an execution table in a later schema makes the predicate false while the unqualified ALTER below it resolves to that same later table, so the widening is skipped without a word and execution.revision silently stays integer. Resolve the relation with to_regclass and read its type from pg_attribute, so the guard and the ALTER always agree on which table they are talking about. to_regclass returns NULL when no execution relation is reachable, which leaves the guard false as before. Two narrower cases came with it. to_regclass resolves any relation, so the guard is restricted to tables by relkind: an index named execution earlier in search_path would otherwise match a pg_attribute row and send ALTER TABLE at something it cannot alter, failing the whole file. And the column type is compared after resolving a domain to its base, so a column already typed as a domain over bigint keeps the domain and its CHECK constraints instead of having them stripped by the ALTER. Covered by two tests against a real Postgres: execution in a schema that is not first in search_path is widened and its rows preserved on two consecutive applies, and a non-table relation of that name leaves the apply clean rather than aborting it. Signed-off-by: Prasanth Baskar <prasanth@8gears.com>
|
Preview images for this PR are available in
Verify a preview image: Verify the SBOM attestation: |
Follow-up to #924, which merged before the cubic bot's review comment on it was addressed. The finding is valid, so it is fixed here rather than on the merged branch.
What broke
The guard #924 added tests
information_schemaagainstcurrent_schema(), which is only the first entry insearch_path. If a connection'ssearch_pathputs another schema first andexecutionlives in a later one, the guard reads false while the unqualifiedALTER TABLE executionbelow it resolves to that later table. The widening is skipped silently andexecution.revisionstaysinteger.What changed
Resolve the relation with
to_regclassand read its type frompg_attribute, so the guard and theALTERalways agree on which table they mean.to_regclassreturnsNULLwhen noexecutiontable is reachable, which leaves the guard false exactly as before.How it was verified
Both predicates run against the same two-schema setup,
search_path = first_schema, later_schemawithexecutionin the later one:Applying the block twice in that layout lands on
bigintboth times, so the guard stays idempotent:go test -tags db ./migration/ -run TestAuthoritativeSchemaAgainstPostgreSQLpasses.