Repository navigation
fix: preserve restore and clone authorization on main - #29532
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
mergify
Bot
added
the
kind/documentation
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Independent whole-PR review with gpt-6-astra / xhigh, as explicitly requested. Actual model/effort verified from CLI session 01a0fbd9-ad66-7d52-b45c-c3fb93ec491b. Reviewed b75086c6fab613b147bff4e474dddac7e237c637..5a58efcf3fead21eb3f5bc1fed2bcc0be74a5081: all 41 files / 90 hunks, including owners/consumers, competing mutations, tests/results and documentation. No subagents or production edits.
Decision: REQUEST_CHANGES — two verified P2 findings.
-
Ownership preflight still includes external tables that bulk restore deliberately skips. Fresh authenticated public SQL on this exact head: admin creates
app.t, another user creates externalapp.ext, then create snapshot/PITR history. The nearest control database restore succeeds, restores the ordinary table and omits the external table. Delete only the external-table creator and retry: both snapshot and PITR database restore fail withcannot restore app.ext: creator no longer exists. The physical user catalog ID is unchanged, so this is not the accepted cross-generation restriction. Filter ownership requirements to the actual recreation set using the existing skip policy; retain rejection for missing principals of recreated objects and explicit single-external-table restore. -
Required SCA fails on
missedSQLpreallocation. Exact-head job 110694260311 reportsauthenticate.go:7911:2: Consider pre-allocating missedSQL (prealloc)and exits 2; CI Required fails. This is a confirmed delivery failure, not an inferred runtime bug. Preserve ordered errors/cache checks and the zero-collection-allocation first-hit path while satisfying the analyzer.
Design and historical comments
The design is proportionate: reuse existing restore transactions, historical DDL identity context, catalog locks, live scoped-grant capture/rebinding and the existing privilege evaluator. No extra persistent identity state or authorization engine is needed. The user-accepted restriction rejecting ordinary custom-principal partial restores across rebuilt catalog generations is not a finding. Data/result compatibility was reviewed; mixed-version rolling-upgrade compatibility was excluded.
- Original retained-database example in thread 4151723310 is fixed. Its broader actual-recreation-set requirement remains incomplete because of finding 1.
- Authorization-cost thread 4151723313 meets the recorded revised performance budget through ordered exact-predicate miss deduplication. Matched reader p50 improves 1.679 ms -> 1.361 ms (~19%), admin 251 us -> 249 us, with comparable controls. This does not recover the old ~80 us stale-cache path; residual fresh-authorization cost remains a material disclosed limitation, not a production performance claim.
- Same-name replacement ownership transfer and the Apache-header defect are fixed. The current SCA license phase passes; finding 2 is a different failure.
- Request-local ownership and grant snapshots represent different historical/current time domains and are justified. Current principal pins and pessimistic removal transactions, DB-before-relation locking, transactional rollback/replay, subscription error propagation and prepared/cross-CN authorization were traced. No additional confirmed leak, double cleanup, unbounded growth or new material blocker was established in those affected closures.
Verification and limits
Reconciled current source hashes with existing evidence before reuse. Current complete diff SHA256 is 46da1489bbbfc4ffd20398fe74f73564a9a6ca40b16ef6aa1ce3b015ed733255; identity delta is 12d2bf20ba0a8e2f1eb5714c4e60f75a7dafc8c3d511d92b96cbc2807bd9dc31. Reused qualifying frontend/public identity/race, planner/compiler/upgrade, BVT and matched performance evidence; prior model approval itself was not evidence. The current authorization BVT is 125/125, builtin full-then-partial neighbor 213/213, with successful recorded comparisons.
New independent probe: TestAstra29532SkippedExternalPrincipal, external Go overlay, repository CGo wrapper, Go 1.26.4 Darwin/ARM64, -count=1 -timeout=180s; native provenance verified. It reached both restore operations and failed the intended assertions, package exit 1 in 14.287s. Initial long-TMPDIR Unix socket setup failure was diagnosed and excluded; the unchanged probe reran using a short task-owned path. No new exact-base runtime execution is claimed: base source already skips external tables and lacks the new ownership preflight.
Exact-head shared build, Ubuntu UT, coverage and reported multi-CN proxy/pessimistic BVT pass; skipped checks are not counted as executed validation. No CI wait, new production-scale benchmark, broad duplicate suite run or repaired-code validation was performed. Worktree and remote head were rechecked unchanged before submission. Full review and probe artifacts retained in the task record.
| } | ||
| } | ||
| tables, err := getStringColsListFromTS(sourceCtx, bh, fmt.Sprintf( | ||
| "select reldatabase, relname, cast(creator as char), cast(owner as char) from mo_catalog.mo_tables {MO_TS = %d} where %s", ts, tableFilter), source, target, 0, 1, 2, 3) |
There was a problem hiding this comment.
[P2] Exclude skipped external tables from required restore principals
This query collects every historical table in the database, but snapshot.go:1635 and pitr.go:1568 later skip external tables. On this exact head, an authenticated SQL probe restores admin-owned app.t successfully while external app.ext's creator exists; the control also proves ext is omitted. After deleting only that creator, both snapshot and PITR database restore fail with cannot restore app.ext: creator no longer exists, although the mo_user catalog generation is unchanged. Require principals only for the actual recreation set, reusing the existing external-table skip policy, while retaining missing-principal rejection for recreated objects and explicit single-external-table rejection.
There was a problem hiding this comment.
Addressed by1b5cd63 and retained unchanged in current54af0d38. Ownership collection now reads existing relkind and calls shouldSkipRestoreTableInBulk before collecting/pinning principals. Execution's selection is unchanged, so an explicitly selected external table still reaches the existing rejection. Fresh exact-head authenticated ownership-scope test passed: missing external creator and missing external owner are independently isolated under snapshot and PITR; ordinary rows restore exactly and external tables are omitted. Explicit external restore preserves the current replacement's physical ID, sentinel42, and sibling data on rejection. Physical user/role catalog IDs remain unchanged; actual recreated ordinary principals still reject before DROP. The shared existing policy is reused, not a separate restore path.
| // Duplicate levels can generate the same complete predicate. Remember only | ||
| // successful misses in this invocation, retaining ordered cache/error checks | ||
| // and the early wildcard hit without allocating a collection. | ||
| var missedSQL []string |
There was a problem hiding this comment.
[P2] Close the observed required preallocation check
The exact-head SCA job 110694260311 reports Consider pre-allocating missedSQL (prealloc) here and exits 2; CI Required is failed. Keep the intended no-collection-allocation first wildcard hit and ordered error/cache behavior while using a bounded backing store or lazy capacity on the first confirmed miss that passes the configured analyzer. This is a real delivery blocker, distinct from the now-fixed license header. Verify with the narrow static check and existing helper tests; do not weaken authorization freshness or restore the rejected batching path.
There was a problem hiding this comment.
Addressed by1b5cd63 and retained unchanged in current54af0d38: missedSQL uses a stack backing array bounded by privilegeLevelEnd, with a zero-length slice until a confirmed miss. Ordered errors/cache/early wildcard-hit behavior is retained; no linter suppression or batching restoration. Fresh full frontend configured golangci-lint2.6.2 reports0issues (all enabled linters, not diff-filtered); full frontend confirm passes25.788s, and exact-head seven-package vet passes. The previous helper allocation observation remains first-hit3 and all-miss30->29, including SQL/mock allocations; it is not a claim of zero total allocations. Newly triggered54af remote SCA is not yet declared passed.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed commit 93405cdc0eafd1d2ce15a65d75d5afe2d531b36a, including ownership and view selection, snapshot/PITR and grant controls, unhappy paths, tests, and the remaining authorization cost. I found no new blocker for this PR.
The constant-view SELECT authorization bypass reproduced on both this commit and its clean base; it is tracked separately in #29572. CI checks are still running at the time of this review.
This branch was successfully deployed
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #29399 on
main; forward-ports the remaining fixes from #29431 (4.2-dev).What this PR does / why we need it:
Repeated restore can mistake CLONE provenance or a view definition for legacy CHECK metadata, lose ownership/current grants, or preserve revoked authorization in an existing connection. Partial restore also treated a role name as identity: renaming an owner blocked restore, while reusing its old name could transfer DROP authority to a different role.
docs/ai-skills/backup-restore.md.The upstream merge at
54af0d38290c66b61ac78a46aa07a363eb576aaeis preserved, including #29563 ESQL and #29568 FULLTEXT2/HNSW coordinator placement. No force push, workflow change or merge-policy bypass.Validation
Go 1.26.4 / macOS ARM64, verified native provenance. Latest source-selection correction (
93405cdc0ea):TestIssue29399*public tests 31.475 s, and configured incremental golangci-lint 0 issues. The byte-identical correction also passed all six tests under race detection before the merge (69.920 s). Go vet and repository molint completed with exit 0; existing molint recover diagnostics outside changed code are retained. No linter suppression/configuration weakening.Unchanged evidence from the whole-PR review is retained: planner/compiler and V406 maintenance tests; prepared/cache/active-role controls; normal BVT comparisons including metadata (ownership/CLONE 125/125, builtin full-then-partial 213/213, cache 37/37, revoked-role 39/39). The inherited target merge separately passed compiler/index owning packages and two-CN FULLTEXT2/HNSW rebuild integration. Its initial
TestConnMeasuresDelayedBufferedFlushtiming failure remains recorded; isolated repetitions and subsequent full frontend passed without weakening assertions.Performance and limitations
Partial source selection adds bounded name maps, uses existing view planning once and avoids repeated partial source enumeration. It adds no persistent state, catalog format, principal query per object or new lock. Current-dependent runtime guards can conservatively omit a prevalidated candidate; this change does not claim exact prediction of every dynamic FK skip.
Existing matched public microbenchmark: reader p50 median 1.617 → 1.333 ms (~17.6% lower); admin 243.5 → 244.3 µs, with comparable controls. These local measurements do not recover the old stale-cache roughly 80 µs latency or establish production throughput. Authorization code is unchanged by the latest source-selection correction.
The latest correction's design and final overall review use actual gpt-6.1-sol / xhigh, reconciling the whole-PR evidence with the changed paths. The source-selection correction's remote CI remains distinct from the target synchronization below; skipped jobs are not passes.
Prior target synchronization and CI
Prior synchronization head:
b190162ef097987a71e9252a49266caaee810451; targetmain:401d153150d9076f84aaaf2a32cdcf880b478395. Normal merge parents are the preserved live93405cdc0eafd1d2ce15a65d75d5afe2d531b36aand the new target. This closes the observed strict-baseline BEHIND condition without rebasing, force-pushing or changing Ready/base state. Only #29570's two prepared-DECIMAL SQL/result files change relative to the first parent, byte-identically to the target; every other tracked file is unchanged. The existing source selection, ownership, current-grant transaction, authorization implementation and disclosed performance limits are preserved.Fresh monitor-local exact-merge binary build passes. The new upstream prepared-DECIMAL case passes strict JDBC metadata validation 213/213, with named database cleanup zero (macOS ARM64 / Go 1.26.4 / Java 20). Independent read-only mechanical merge review approves and whitespace/worktree checks are clean. The prior
93405cdcfrontend/public/race proof above is external local unchanged-Go evidence, not a fresh whole-suite execution or current-head remote CI pass. No source-branch cache-certificate or internal-executor fix is blindly copied to main.XuPeng-SH approved
93405cdcat 14:21 UTC after whole-PR follow-up; this is an actual submitted review, not the local mechanical approval. Prior-head Connector/J run 37015828472 attempt 1 fails during ONNX Runtime download (HTTP 504), before the server/test. Exactly one failed-job rerun request was accepted. Attempt 2 / 110870725085 downloads the dependency and actually passes pool-reset tests for all four Connector/J versions at 14:07:46 UTC. That success belongs to93405cdc, not this new merge head. No healthy jobs or whole ALL CI run were rerun/cancelled and no build/workflow/tester/business workaround was added.That head's Connector/J 37023553819 actually succeeds at 15:01:50 UTC; ALL CI remains unfinished when it is superseded by the next target merge. This is historical evidence, not a result for the current head.
Prior target synchronization and validation
Prior head:
380bbce20601196b9c9b09ea48089bb666985857; targetmain:5a09e1ffbf40a59f50c55fe89e6f0c23eba143a3. After main merges #29567, strict protection marks the previous head BEHIND again. A normal merge preserves parentsb190162ef097987a71e9252a49266caaee810451and the new target, without conflicts, manual overwrites, rebasing or force push. Its 15-file delta matches the upstream commit exactly, including modes and blob IDs. All existing PR changes from the approved implementation remain intact. Inherited prepared runtime-plan statistics admission does not bypass schema invalidation or text/binary EXECUTE reauthorization; shared restore source selection, ownership preflight and current-grant transactions are unchanged.Fresh monitor-local exact-head proof (macOS ARM64 / Go 1.26.4):
TestIssue29399*tests pass (29.908 s), including the eight optimistic-SI privilege-removal arms, snapshot/PITR ownership and source-selection boundaries, principal pins and invalid-grant rollback. Upstream reordered-join semantics and prepared workspace visibility tests pass (13.031 s), including integer/bigint/string/composite unique controls. The test-only atomic output[no statements]is not production coverage.380bbce206pass. Configured incremental lint across all five upstream changed package roots reports 0 issues. Worktree and whitespace checks are clean. Independent read-only merge/integration review approves; it checks inheritance and interaction, not the general upstream statistics policy.A fresh OPEN/Ready/head guard was awaited and read before the accepted normal push. Latest current-head CI is newly triggered and not complete; no BVT, full coverage, required-context or merge-gate pass is claimed. Prior-head successes are not substituted for current-head results. No workflow, tester or source cache-certificate/internal-executor framework is copied into main. Existing performance limitations and the separate constant-view issue #29572 remain unchanged.
Current target synchronization and validation
Current head:
ae4460f455bafa98a5beb301906adbd0abc522b6; targetmain:9078a8bd87af49ea06f52e09d64e7740122aea61(#29573 vector UnionOne/marshal). Strict-baseline BEHIND is addressed by a normal merge with first parent380bbce20601196b9c9b09ea48089bb666985857, without conflict, manual overwrite, rebasing or force push. The four-file delta is byte/mode-identical to the upstream delta; the original 43-file PR patch remains identical against the new target. Restore source selection, ownership/current-grant transactions, principal-generation restriction, protocol/DAG and authorization implementation are preserved.Independent exact-merge local proof (macOS ARM64 / Go 1.26.4, verified inherited native inputs and absolute CGo paths):
TestIssue29399*plus authenticated fixture configuration pass under race (65.232 s), including all eight optimistic-SI privilege-removal arms. Another 28 objectio/hashbuild marshal, recovery/area and runtime-filter consumer tests pass under race (2.722 s / 2.297 s).[no statements]is not production coverage.A fresh OPEN/Ready/head/body guard is awaited and read before the accepted normal push. The source-only SQL-task principal follow-up is not blindly copied into main's unchanged adapter/authorization path. Existing performance limitations and separate constant-view issue #29572 remain.
Current exact-head ALL CI run 37034311275, attempt 1, completes SUCCESS at 2026-10-02 17:51:19 UTC. Actual Ubuntu UT, arm64 SCA, shared build, UT Coverage, PROXY/PESSIMISTIC multi-CN BVT, Coverage and CI Required all succeed. The CI Required log confirms
fullscope, generation37034311275-1, all five dependency groups successful andcoverage_ready=true/coverage_status=passed. The Coverage log selects UT and both complementary BVT profiles from that same generation and publishes the merged result. Current-head Connector/J succeeds; Iceberg is SKIPPED, not a functional test pass. No failed-job or whole-run rerun/cancellation is performed this round.The PR remains OPEN / Ready / MERGEABLE / BLOCKED / APPROVED. The retained approval was submitted on the earlier
93405cdcimplementation; it is not a new review of this target-merge head. All four discussion threads remain unresolved, with no new human findings. Completed CI Required and test jobs do not establish that every review/protection requirement has been satisfied; no Ready/base/approval/merge or protection policy is changed.