test: cover multi-insert prepare and privileges

[Ariznawlll]

Summary

• add SQL PREPARE/EXECUTE coverage for INSERT FIRST parameter binding and prepared-handle reuse
• add privilege coverage for source SELECT, per-target INSERT, and all-or-nothing rejection

Validation

• Multi Insert targeted UT passed
• new prepare and privilege BVT each passed 3 consecutive runs
• full Multi Insert BVT: 561/561 statements passed, including big/skew cases

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[XuPeng-SH]

Deep review at exact head 477b87d250c6b1ea430a54d110b11f9c4bc67e4a found two blocking test-contract gaps.

1. The advertised source-SELECT privilege contract is not actually tested. Every multi-insert execution in multi_insert_privilege.sql runs after SELECT on src has been granted. A production regression that completely skips source privilege validation would still pass both the success case and the denied-second-target case. Please add a negative case where the role has INSERT on every target but lacks/revokes SELECT on the source, assert the statement is rejected, and assert neither target changed. Keep it distinct from the current per-target INSERT/all-or-nothing oracle.

2. The case mutates @@global.enable_privilege_cache from an unknown prior value to off, then hard-codes on at teardown. A BVT group intentionally starting with the global value off will leave this file with it on, contaminating later cases; three repeated runs cannot detect that because each run overwrites the state again. This scenario creates all grants before login and does not appear to require cache invalidation, so remove the global mutation if it is unnecessary. Otherwise capture and restore the actual pre-existing value with cleanup that remains effective on expected-error paths.

The PREPARE parameter-position, NULL three-valued routing, handle reuse, deterministic output, deallocation, and database cleanup portions are sound. Exact-head CI is green and git diff --check passes, but those checks do not prove the missing negative oracle or cross-case global-state isolation.

[aptend]

Deep review at exact head 477b87d found two blocking gaps.

[P1] Add the missing source-SELECT denial oracle — test/distributed/cases/dml/insert/multi_insert_privilege.sql:17 grants SELECT on src before every execution, so the suite never runs multi-insert without that privilege. A regression that stops checking source SELECT would still produce every expected result here. Add a distinct case with INSERT on both targets but no SELECT on src, expect rejection, and verify both targets are unchanged; otherwise the advertised source-privilege contract is not covered.

[P1] Do not overwrite shared global state — test/distributed/cases/dml/insert/multi_insert_privilege.sql:3 forces enable_privilege_cache off and line 45 unconditionally sets it on. If a BVT group enters this file with the global value off, the case changes it to on for all later files; an expected-error abort can also skip the restoration entirely. All grants are created before the test login, so this scenario should work under the normal/default cache path without either mutation. If changing it is truly required, preserve the actual prior value and restore it through cleanup that runs on failure.

The prepared-marker ordering, NULL routing, handle reuse, per-target INSERT denial, all-or-nothing counts, deterministic ordering, and resource cleanup were also checked. Exact-head CI is green and git diff --check passes, but those do not exercise the missing negative oracle or prove cross-case global-state isolation.

[Ariznawlll]

Addressed both review blockers in 2669665ac1:

• Added an independent role/user with INSERT on both targets but no SELECT on the source. The rejected multi-insert is followed by zero-count assertions on both targets, so the test now detects a missing source privilege check and verifies atomicity.
• Removed the global enable_privilege_cache mutation. All grants are completed before each test user's first session, so the case no longer changes suite-global state.

Validation:

• mo-tester -c SQL/result contract check passed.
• go test ./pkg/sql/plan -run 'MultiInsert' -count=1 passed.
• The privilege paths were exercised against a local MO service 3 times: source denial stayed 0/0; target denial stayed 1/0.
• Independent diff review found no remaining issues.

[XuPeng-SH]

Re-reviewed exact head 2669665 after my request-changes review on 477b87d.

Both blockers are closed:

• the independent no-SELECT principal has INSERT on every target, so its rejection specifically proves source SELECT enforcement; the following 0/0 assertions prove all-or-nothing behavior;
• the suite-global enable_privilege_cache mutation was removed, eliminating state contamination and expected-error cleanup risk.

I also rechecked marker ordering across WHEN/VALUES/source predicates, prepared-handle reuse, NULL three-valued routing, per-target INSERT denial, deterministic result ordering, user/role/database cleanup, and the exact-head CI evidence. No remaining blocking correctness, isolation, or test-quality issue found.

stale

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[mergify]

queue

⚠️ Invalid commit message

Details

section not found