Conversation
…at lost rows An INSERT OVERWRITE commits its rows into temporary partitions and publishes them with a swap afterwards, so a cancellation that lands between the two halves is too late to take anything back: the rows are durable, and everything the read consumed -- the base table stream offsets among it -- was committed with that same transaction. The window was answered by dropping the temporary partitions and returning normally, which published nothing while telling the client the overwrite had succeeded, and left the consumed offsets advanced past rows no target will ever hold (a re-run reads from the advanced offset and silently misses that range). The cancellations that land before the rows are committed were answered the same way, reporting success for an overwrite that never ran. Key changes: - A cancellation that lands before the rows are committed now fails the statement, like the cancellation the inner insert already reports for itself. Nothing durable happened, so the offsets are where they were and a re-run reads the same rows. - A cancellation that lands after the rows are committed completes the swap instead: the rows the read consumed are the ones the table holds, the offset advance and the publication stay aligned, and the success the statement reports is the outcome it is. - The failure path no longer asks the overwrite manager to fail the task of a cancellation that landed before any task was registered. - A debug point (stage=beforeTheInsert|afterTheInsert, scoped by table name) injects the cancellation at either side of the insert. Unit Test: - regression-test/suites/insert_overwrite_p0/test_insert_overwrite_cancel.groovy covers both sides: a cancelled statement leaves the table untouched, and a cancellation inside the window is published rather than dropped. - insert_overwrite_p0 (14 suites, including the new one) and mtmv_p0/ivm/test_ivm_overwrite_failure_between_the_halves pass.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
yujun777
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review outcome
Complete static review of PR #68662 at 85c4ae452e17feccaa9b297fdf2a2a11a69d5b9a. Request changes for the P1 cancellation/data-loss case. Four distinct new inline findings are attached (one P1, two P2, one P3). There were no existing inline P0/P1 findings to reconfirm and no additional user focus points.
Critical checkpoints
- Goal and proof: The explicit-partition path now fails before task registration/insert and completes the swap after a committed nonempty insert. The goal is incomplete for a physical empty-relation input: no transaction commits, yet a cancelled explicit overwrite can replace the table with empty partitions (P1). The new post-insert test can pass without firing its cancellation point (P2).
- Scope and structure: Changes are localized to the overwrite command and one regression suite. The conditional assumption that every returned inner insert committed rows is false for the empty-plan fast path.
PARTITION(*)remains a parallel false-success path (P2). - Concurrency and lifecycle:
StmtExecutor.cancelruns on another thread, sets the outer command's atomic flag and a pending coordinator reason, and may cancel a published coordinator. The inner insert is a separate command. A nonempty insert publishes a coordinator and fences cancellation before commit; an empty insert returns before that handoff. Task registration, temporary partitions, swap, and cleanup were traced. No new locks, lock-order issue, static-initialization dependency, or resource cycle was found. - Transactions, persistence, and writes: Before insert, task failure drops temporary partitions; after a committed nonempty insert, the swap preserves its rows and consumed offsets. The empty-source case violates the before-commit cancellation rule and can erase previously visible rows. The task manager's existing edit-log/replay path and the pre-existing crash/swap window are unchanged; this review did not claim failover validation.
- Parallel paths and compatibility: Auto-detect, local/remote OLAP, and connector paths were examined. The auto-detect empty-source cancellation gap is distinct from the explicit-partition data loss. No new configuration, FE/BE transmitted variable, function symbol, protocol, or storage-format change was introduced; rolling-version handling does not change.
- Tests and expected output: The
.outrows match the fixture, and the suite uses deterministic ordered queries, an expected-error assertion, and debug-point cleanup. It lacks an assertion that the later cancellation fired, and it does not exercise realStmtExecutor.cancel, a table-stream offset, or the empty-source case. No builds or tests were run because the review instructions prohibit them; author-reported test passes were not treated as independent verification. - Observability and performance: Cancellation logs include query IDs; no additional production metric or hot-path performance issue was substantiated. The one-shot debug point is broken because each parameter lookup consumes its execution budget (P3). No other substantiated changed-line issue remained after the final sweep.
| @@ -266,32 +283,37 @@ public void run(ConnectContext ctx, StmtExecutor executor) throws Exception { | |||
| } else { | |||
| // it's overwrite table(as all partitions) or specific partition(s) | |||
| List<String> tempPartitionNames = InsertOverwriteUtil.generateTempPartitionNames(partitionNames); | |||
There was a problem hiding this comment.
[P2] Apply the pre-commit cancel rule to PARTITION(*) as well. This check is only in the explicit-partition branch. For INSERT OVERWRITE t PARTITION(*) SELECT ... LIMIT 0, the inner insert returns through its empty-plan fast path without a transaction or coordinator, so a real KILL leaves the outer cancellation flag set but the auto-detect branch still calls taskGroupSuccess and reports success. Check cancellation in that branch when no insert committed, so this form does not retain the false-success behavior the PR is fixing.
There was a problem hiding this comment.
Applied the same boundary to this branch: insertIntoAutoDetect now hands its context back, and a cancellation there is honoured when the load committed nothing (hasCommittedNothing()), the same way the explicit-partition branch does.
One correction to the premise, from probing the plans on a local cluster: a PARTITION(*) target is partitioned, and an empty query over a partitioned target plans an exchange, so the sink's child is not the PhysicalEmptyRelation and requiresTransaction() is true -- the load does begin and commit an (empty) transaction rather than returning through the no-transaction path. The empty-plan shape reproduces on an unpartitioned target, which is what the new suite case uses (INSERT OVERWRITE TABLE flat_dst SELECT ... WHERE 1 = 0). The branch is kept so both routes take the same decision.
There was a problem hiding this comment.
You are right, and my earlier reply's premise was wrong. I re-probed with the session variable set: with enable_strict_consistency_dml = false, INSERT OVERWRITE TABLE t PARTITION(*) SELECT ... WHERE 1 = 0 plans 0:VEMPTYSET as the OLAP TABLE SINK's direct child -- no exchange -- so that shape does take the insert's no-transaction path and the marker stays false for it.
Fixed on this head: both routes now publish through one place. publishTheOverwrite takes the publication as an action, so the explicit-partition swap and taskGroupSuccess share the same last-chance decision under the target table's write lock (and the same failure for a target dropped under the swap). A cancellation landing there with nothing committed now fails the statement instead of being acknowledged.
For the record, what that residual was in this shape: the group has no registered pairs, so taskGroupSuccess reaches the utility with two empty lists and returns without replacing anything -- no rows were destroyed, the problem was the false success. The suite drives it: SET enable_strict_consistency_dml = false, arm the lock-wait point on a partitioned target, INSERT OVERWRITE TABLE dst PARTITION(*) SELECT ... WHERE 1 = 0 has to fail with cancelled while the swap waited for the table lock and the table has to keep its rows (dst_after_the_cancelled_auto_detect_swap).
| LOG.info("insert overwrite is cancelled before replacePartition, queryId: {}", | ||
| ctx.getQueryIdentifier()); | ||
| insertOverwriteManager.taskFail(taskId); | ||
| return; |
There was a problem hiding this comment.
[P1] Preserve cancellation when the inner insert has no transaction. INSERT OVERWRITE dst SELECT ... LIMIT 0 becomes a PhysicalEmptyRelation; InsertIntoTableCommand.runInternal returns at !requiresTransaction() without committing or publishing a coordinator. If KILL lands after the earlier check (or this after-insert debug point fires), isCancelled is true here, yet the command swaps the empty temporary partitions over dst and reports success. This deletes the prior rows even though cancellation occurred before any insert commit. Fail and roll back this no-transaction case; complete the swap only after a committed insert.
There was a problem hiding this comment.
Fixed. The insert now tells its caller that nothing was committed: InsertIntoTableCommand.runInternal marks the context at the !requiresTransaction() return (InsertCommandContext#setCommittedNothing), the overwrite reads that context back from insertIntoPartitions, and the window check fails the statement -- rolling the empty temporary partitions back through the existing catch -- instead of completing the swap.
Verified end to end on an unpartitioned empty-plan overwrite: with the cancellation injected in the window, INSERT OVERWRITE TABLE dst SELECT ... WHERE 1 = 0 now returns insert overwrite is cancelled after an insert that committed nothing and the target keeps its rows. Before the change the statement succeeded and the table was emptied (FE log cancelled after its rows were committed, completing it, count(*) 1 -> 0). The new suite case pins it.
There was a problem hiding this comment.
Fixed on this head, and you are right that the interval matters: the swap waits for the target table's write lock, and that wait lasts as long as whoever holds it, so a cancellation landing in it is not a microsecond race.
The flag is now read once more under that lock, immediately before the swap, and when a cancellation found nothing committed the overwrite refuses to swap: the temporary partitions are rolled back by the existing catch, the statement fails, and the table keeps its rows. Where the rows are committed the cancellation is still too late and the swap runs, which is the same boundary as everywhere else -- the check only changes the outcome for a cancellation with nothing durable to publish. Only a local table is wrapped (a remote table swaps on the frontend that owns it, where this lock says nothing), and the lock is reentrant, so the util's own acquisition inside it is free.
A debug point (InsertOverwriteTableCommand.cancelWhileTheSwapWaitsForTheTableLock) injects the cancellation at exactly that point, and the suite's dst_after_the_cancelled_locked_swap case asserts the statement fails with cancelled while the swap waited for the table lock and that the table still holds the row it had.
| */ | ||
| private void cancelTheOverwriteAt(String stage, TableIf targetTable) { | ||
| if (!stage.equals(DebugPointUtil.getDebugParamOrDefault( | ||
| DEBUG_POINT_CANCEL_AN_OVERWRITE, "stage", ""))) { |
There was a problem hiding this comment.
[P3] Make one-shot cancellation points usable at both stages. getDebugParamOrDefault consumes the point's execute budget on every lookup. With execute=1 for beforeTheInsert, the stage read uses the one allowance and this table_name read expires the point; for afterTheInsert, the earlier nonmatching before-stage check consumes it. Both silently skip cancel(). Read both parameters from one point instance and avoid consuming a stage's one-shot budget at the other stage (for example, use stage-specific point names).
There was a problem hiding this comment.
Fixed: two point names instead of one, and one lookup per site. cancelBeforeTheInsertOfAnOverwrite and cancelBetweenTheTwoHalvesOfAnOverwrite each read their single table_name parameter from one getDebugPoint call, so no site can spend the other's execute allowance; the stage parameter and the second lookup are gone. The comment on the constants records why a shared name was wrong.
| // partitions, and the swap is what publishes them. The statement reports the success it now is, and the | ||
| // rows it read are the ones the table holds. | ||
| try { | ||
| GetDebugPoint().enableDebugPointForAllFEs(cancelPoint, |
There was a problem hiding this comment.
[P2] Make the post-insert cancellation case observable. If the afterTheInsert call is removed or its stage stops matching, this SQL still succeeds and the final query still returns rows 3 and 4, so the suite passes without exercising the behavior it was added to protect. Assert that this stage actually cancelled the command (and preferably exercise a real StmtExecutor.cancel/stream-offset case) before treating the row result as a regression guard.
There was a problem hiding this comment.
Fixed by making the window's injection observable, in the way the earlier failure-half suite does it. Alongside the case that asserts the rows are published, the same injection point now drives an empty-plan overwrite (... WHERE 1 = 0) where the cancelled statement has to fail: that assertion holds only if the injection reached the window, so deleting the call, renaming the point, or breaking its lookup makes the suite fail. The empty-plan case also pins its own behaviour (the table has to keep its rows, so the swap must not have run) rather than passing on the rows either way.
A real StmtExecutor.cancel is not used for the same reason the failure half is injected: the window is a couple of metadata operations wide, so a KILL would have to land inside it by luck. The stream-offset variant of this scenario is the one that matters for IVM refreshes; it needs the crash half of the window (swap as a committed action of the insert transaction) to be worth testing end to end.
TPC-H: Total hot run time: 27853 ms |
TPC-DS: Total hot run time: 152313 ms |
ClickBench: Total hot run time: 23.82 s |
The window between an overwrite's two halves treated every cancellation that landed in it as too late, and completed the swap. That is right where the insert committed the rows it read, but an insert whose plan folded to an empty relation takes the path that begins no transaction at all: it commits nothing, so completing the swap published an empty table for a statement the client had cancelled, and reported success for it. The auto-detect route left the same boundary unhandled, and the two cancel points shared one debug point and two lookups, which let a point armed with execute=1 be spent by the other site's lookup and silently not fire. Key changes: - The insert marks the context it ran under when it takes the path that begins no transaction, and the overwrite reads that back: a cancellation landing in the window now fails the statement and drops the empty temporary partitions, while one that lands after committed rows still completes the swap. - The auto-detect route takes the same decision. - One debug point per cancel site, each reading its single table_name parameter from one lookup. - The suite covers the empty-plan window as well: the cancelled statement has to fail and the table has to keep its rows, which also pins that the injection reached the window. Unit Test: - regression-test/suites/insert_overwrite_p0/test_insert_overwrite_cancel.groovy - insert_overwrite_p0 (14 suites) and mtmv_p0/ivm/test_ivm_overwrite_failure_between_the_halves pass.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review outcome
Complete static review of PR #68662 at 215bcf07bd59d9981ed5549ee53b61e39083b232. Request changes for one new P1 inline finding: a committed insert can return a visibility-timeout error, bypass the new cancellation decision, and have its temporary partitions dropped. The earlier P1 thread 4140574895 is fixed on this head by the no-transaction context flag and failure path, so there are no still-applicable existing P0/P1 inline findings. The existing P2 test-observability thread is not duplicated. There was no additional user-provided focus.
Critical checkpoints
- Goal and proof: Before-commit cancellation now fails; a normal committed insert completes the partition swap. The goal is incomplete for the committed-but-error visibility-timeout path in the inline finding. The new suite exercises before-insert cancellation, a rowful post-insert swap, and a cancelled empty-plan overwrite, but not that error path.
- Scope and clarity: The changes are localized to the insert context, the inner and outer commands, and one regression suite with its expected output. The new
hasCommittedNothingcondition is appropriate for the no-transaction fast path, but an error response cannot establish that the inner insert was uncommitted. - Concurrency and locks: KILL runs on another FE thread.
StmtExecutor.cancelsets the outer atomic cancellation flag and retains a pending coordinator reason; the execution thread publishes the coordinator and checks its status before commit. The added checks take no new locks or heavy work inside locks. No new lock-order or shared-container issue was found. - Lifecycle: Each inner insert has a fresh context; task registration and cleanup remain paired by the outer catch/finally path. The new early failure skips task cleanup only when no task was registered. No new static initialization or ownership cycle was found.
- Configuration and compatibility: No new configuration, FE/BE transmitted variable, protocol, function symbol, or storage format is added. The existing
insert_visible_timeout_return_mode=errorsetting exposes the P1 path. No rolling-upgrade change is required by these hunks. - Parallel paths and conditions: Explicit/whole-table and
PARTITION(*)routes both use the new context; the latter shares the P1 cause throughtaskGroupFail. Remote OLAP and supported plugin overwrite paths yielded no separate substantiated defect. Separate one-lookup debug points address the earlier one-shot thread. - Tests and expected results: The
.outrows match the seeded rows and ordered queries. Expected errors usetest { sql; exception }, and table setup and debug-point cleanup follow the suite conventions. The rowful case alone can pass without cancellation, a limitation already raised in thread4140574902; a real KILL, stream-offset case, and the visibility-timeout error path are not covered here. No build or test was run under the review instructions; the author's reported runs and output-generation provenance were not independently verified. - Observability and performance: Cancellation/error logs include query IDs; no additional metric or material hot-path cost was found. The production additions are a small context flag and guarded debug-point lookups.
- Transactions, persistence, and writes: The task manager retains its existing edit-log and cleanup lifecycle. Ordinary postcommit cancellation swaps durable rows and their consumed offsets, but the P1 path drops committed temporary partitions while offsets may advance. The pre-existing crash, master-failover, and metadata-error gap between commit and swap is outside this patch and was not treated as newly fixed; this static review does not claim runtime or failover validation.
Two review rounds converged: both full-review passes and the separate risk-focused pass returned NO_NEW_VALUABLE_FINDINGS in round two. The final sweep covered all five changed files and every candidate; only the P1 below remains for a new inline comment.
| throw cancelledBeforeTheRowsWereCommitted("before insertInto", ctx); | ||
| } | ||
| // todo: need to refresh remote target table after add temp partitions | ||
| insertIntoPartitions(ctx, executor, tempPartitionNames, wholeTable); |
There was a problem hiding this comment.
[P1] Complete the swap after a committed visibility-timeout error. With insert_visible_timeout_return_mode=error, OlapInsertExecutor.onComplete can commit rows into the temporary partitions and then setReturnInfo sets the connection state to ERR when publication times out. runInsertCommand throws on that ERR before this new post-insert cancellation decision runs; the outer catch calls taskFail (or taskGroupFail for PARTITION(*)), which drops the committed temporary partitions. A KILL during that postcommit wait still loses the rows and any committed stream offsets the change is meant to preserve. Carry the committed outcome separately from the response state and finish the swap for this case.
There was a problem hiding this comment.
Fixed, on the same boundary the previous commit drew. The insert context now records that its transaction committed (set by each executor where its own commit happens -- OLAP, remote OLAP, external/plugin -- via AbstractInsertExecutor#markCommitted), separately from the response state the insert leaves behind. The overwrite reads it in both places it decides:
- the window's cancellation is honoured only when nothing was committed;
runInsertCommandno longer stops the overwrite on an error state whose rows are committed: the swap publishes them, and the client keeps the error the session asked for (insert_visible_timeout_return_mode=error), which is a statement about visibility rather than about whether the overwrite ran.
Verified with the publish daemon blocked (PublishVersionDaemon.stop_publish, 10s) and insert_visible_timeout_ms=1000: the statement returns the visibility-timeout error, the FE log records insert overwrite continues over an error state whose rows are committed, and the row the overwrite read becomes visible once publish resumes. The suite's new dst_after_the_publish_timeout case asserts exactly that -- with the previous code the table keeps its old rows and the wait fails.
One boundary worth naming, since the response stays the error the session asked for: a client that retries the overwrite after that error gets a partial overwrite, because the re-run reads from the offset the first attempt advanced. That is the same contract insert_visible_timeout_return_mode=error has for a plain INSERT, and making the retry safe belongs to the durable fix (the swap as a committed action of the insert transaction) rather than to this PR.
TPC-H: Total hot run time: 27949 ms |
TPC-DS: Total hot run time: 152296 ms |
ClickBench: Total hot run time: 23.71 s |
…an error The overwrite stopped at any error state the insert left behind, and dropped the temporary partitions it had written. A load whose publication times out after its commit is committed, though, and the session's visibility-timeout mode (insert_visible_timeout_return_mode=error) reports that timeout as an error: the rows existed, the response said otherwise, and the overwrite discarded them together with the stream offsets the read had consumed. The question that decides a cancellation in the same window -- did the insert commit -- answers this one as well. Key changes: - The insert context records that the transaction committed, set by each executor where its own commit happens (OLAP, remote OLAP, external/plugin). That is a different question from the response state the insert leaves behind. - The overwrite reads it wherever it decides: a cancellation landing in the window is honoured only when nothing was committed, and an error state no longer stops the swap when the rows are durable -- the swap publishes them, and the client keeps the error the session asked for. - The suite reaches the publication timeout by blocking the publish daemon, as test_insert_visible_timeout_return_mode does, and asserts that the rows the error reported as committed become visible once publish resumes. Unit Test: - regression-test/suites/insert_overwrite_p0/test_insert_overwrite_cancel.groovy - insert_overwrite_p0 (14 suites), insert_p0/test_insert_visible_timeout_return_mode and mtmv_p0/ivm/test_ivm_overwrite_failure_between_the_halves pass.
|
run buildall |
|
/review |
TPC-H: Total hot run time: 27701 ms |
There was a problem hiding this comment.
Static review complete for PR #68662 at 83bac904ca8555873d587f328c33d9e3eb50f697 after two full review rounds and a final nine-file sweep. One new P2 inline finding concerns the cloud regression case. Existing P1 comment 4140574895 still applies to a narrower late-cancellation window; I confirmed it and have not reposted it. The prior P1 visibility-timeout concern is addressed on this head. There was no additional user-provided review focus.
Goal and data correctness. The request-local committed flag is set after the reachable local OLAP, remote OLAP, and external connector commits. The local publication-timeout path now keeps committed temporary partitions for the swap while retaining the configured error response. A no-transaction empty insert leaves the flag false, and the explicit and PARTITION(*) branches check it. The outstanding existing P1 remains: if KILL arrives after the explicit branch's last no-commit check while partition replacement waits for the table write lock, the empty partitions can still replace the original rows and the statement can report success.
Concurrency, lifecycle, and transaction checkpoints. StmtExecutor.cancel sets the outer command's atomic cancellation flag from another thread; the insert and overwrite run on the statement thread. No new shared lock, lock ordering, static lifetime, FE/BE field, configuration item, wire format, or persisted format was added. Task and task-group failure paths roll back registered temporary partitions before commit; committed rows proceed to replacement. The commit marker is request-local, while transaction and stream-offset persistence remains in the existing transaction managers. The swap remains a separate metadata action, so preexisting crash, swap-error, and uncertain commit-response windows are unchanged; no distinct new issue in those paths was substantiated.
Parallel paths, tests, and operational impact. Executor selection excludes transaction-model overwrite and ordinary group commit; the reviewed local, remote, external, explicit-partition, and auto-detect paths use the applicable marker. The added debug points are separately named and the empty-plan error assertion observes its injection. The .out rows agree with the fixture's ordered queries. The unconditional publish-timeout assertion cannot pass in cloud mode, where commit has no FE publication wait; that is the new P2 inline finding. Existing logs identify the committed cancellation and error-state paths, and the added checks have constant cost. This review was static only: the review contract prohibited builds and tests, and author or CI results were not treated as independent runtime validation.
Existing P0/P1 findings confirmed for this head: #68662 (comment)
| GetDebugPoint().enableDebugPointForAllFEs(stopPublishPoint, [timeout: "10"]) | ||
| sql """SET insert_visible_timeout_ms = 1000""" | ||
| sql """SET insert_visible_timeout_return_mode = 'error'""" | ||
| test { |
There was a problem hiding this comment.
[P2] Skip this publish-timeout case in cloud mode. CloudGlobalTransactionMgr.commitAndPublishTransaction returns true after commit because cloud mode has no FE publish phase, so OlapInsertExecutor reports success even when PublishVersionDaemon.stop_publish is armed. This unconditional test { exception ... } fails when the suite runs in cloud mode. Guard the timeout case and its final publish assertion with !isCloudMode() (as test_insert_visible_timeout_return_mode does); keep the cancellation cases active in cloud mode.
There was a problem hiding this comment.
Guarded. The publication-timeout case is now inside if (!isCloudMode()), with the reason recorded in a comment (cloud's commit has no FE publication wait to time out, since CloudGlobalTransactionMgr commits straight into the meta service), and the final publish assertion moved inside it. The cancellation cases stay unconditional, so cloud still runs those.
TPC-DS: Total hot run time: 152411 ms |
ClickBench: Total hot run time: 23.89 s |
…for the lock The last read of the cancellation flag happens before the swap is issued, and the swap then waits for the target table's write lock -- a wait that lasts as long as whoever holds it. A cancellation that found nothing committed could therefore still be followed by a swap publishing empty temporary partitions over the rows the client asked to keep. Key changes: - The flag is read once more under that lock, where the wait happens, and the swap is refused when a cancellation found nothing committed -- there is nothing durable to publish, so refusing costs the statement and leaves the rows. Only a local table is wrapped: a remote table swaps on the frontend that owns it. - A debug point injects the cancellation there, and the suite asserts the table keeps its rows. - The publication-timeout case is skipped in cloud mode, where the commit has no FE publication wait to time out, as test_insert_visible_timeout_return_mode does. The cancellation cases stay active there. Unit Test: - regression-test/suites/insert_overwrite_p0/test_insert_overwrite_cancel.groovy - insert_overwrite_p0 (14 suites), insert_p0/test_insert_visible_timeout_return_mode and mtmv_p0/ivm/test_ivm_overwrite_failure_between_the_halves pass.
|
run buildall |
The comment on the window's "too late to cancel" branch still described the insert as having published the rows by the time it returns. Since the publication-timeout case is handled there too, the rows can be committed with their publication still pending, which is what the comment now says.
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 27717 ms |
TPC-DS: Total hot run time: 152404 ms |
ClickBench: Total hot run time: 23.94 s |
There was a problem hiding this comment.
Static review complete for head 5eee943f3d313befe4c815a58178ea4f0e7610b6 after two full-review rounds, a separate risk scan in each round, candidate deduplication, and a final sweep of all nine changed files. I found four distinct issues (three P1, one P2), all inline. The earlier P1 comments 4140574895 and 4140980598 were independently rechecked and are fixed for their local empty-plan/lock-wait and received visibility-timeout scenarios on this head; no existing P0/P1 comment still applies as written. The four new comments cover separate remote, cloud, and concurrent-DROP cases. There was no additional user-provided review focus.
Critical checkpoints:
- Goal, scope, and tests: The patch makes precommit local cancellation fail and lets a known committed local insert finish its swap. The injected local regression cases and expected ordered rows cover those returned-result paths. The four inline cases remain outside that coverage. The change is focused on overwrite outcome handling, although the new boolean cannot represent an unknown commit result.
- Concurrency and locking:
isCancelledis atomic. The local final check holds the target table's reentrant write lock, with the existing replacement routine taking that lock again; no new lock-order deadlock was substantiated. The remote FE owns a different lock and receives no cancellation state (P1), and a concurrent local DROP can make the new wrapper skip replacement then report success (P2). The added lock covers only the existing metadata swap work; no separate hot-path locking regression was found. - Lifecycle and error handling: Each overwrite gets a fresh context, read on the same statement thread; there is no static initialization issue. Task registration and failure cleanup are persisted through the existing overwrite manager. Known committed results set the marker, but exhausted remote RPC retries and cloud maybe-committed errors can leave durable rows marked uncommitted, so cleanup can delete them (two P1 findings). A skipped swap after DROP is also incorrectly acknowledged. No new configuration item was added.
- Parallel paths and compatibility: Local, cloud, remote OLAP, connector, empty-plan, explicit-partition, and
PARTITION(*)paths were traced. Transaction-model overwrite is rejected before execution and group commit excludes the inner overwrite. No wire or storage format changed, and no FE-to-BE variable was added. Remote cancellation requires an owner-side decision that the current RPC does not convey. - Transaction, persistence, and data correctness: The existing visible-version and publish mechanisms are used, and the local swap remains under the table lock. The new context flag is ephemeral and does not itself change edit-log replay. The two-phase crash and replacement-failure window is acknowledged in the PR description; this review's new comments address incorrect rollback or success outcomes reachable while the FE stays running. No BE memory, delete-bitmap, or rowset code changed.
- Test results, observability, and performance: The
.outlabels and rows match the suite's ordered queries, and its debug points are distinct and consumed once per site. The lock-point fixture checks the under-lock branch but does not simulate a concurrent wait; no remote RPC-loss, cloud ambiguous-commit, or concurrent-DROP test is present. New logs identify cancellation and committed ERR paths; no extra metric was required by this narrow change. One additional reentrant lock acquisition and debug lookup per local overwrite show no obvious performance issue. This was a static review only: the review instructions prohibited builds and tests, so the PR author's reported passes were not independently reproduced.
| txnStatus = TransactionStatus.COMMITTED; | ||
| } | ||
| markCommitted(); | ||
| LOG.info("commit remote txn success, catalog={}, dbId={}, txnId={}, status={}", |
There was a problem hiding this comment.
[P1] Reconcile a remote commit whose reply is lost before rolling back the overwrite. The owning FE commits before it waits for publication, while masterCallWithRetry can exhaust retries after a lost reply. In that case this marker stays false although the remote transaction is COMMITTED; onFail cannot abort it, and the overwrite treats its ERR state as uncommitted and calls taskFail, dropping the partitions containing those rows. Query the remote transaction outcome or preserve its temporary partitions while the result is unknown. This differs from the existing visibility-timeout thread, where the committed response reached this FE.
There was a problem hiding this comment.
Agreed that the state is possible, and agreed that the caller cannot tell it apart from an aborted commit: after masterCallWithRetry loses the reply, this FE has no way to know whether the owner committed, and onFail's abort cannot take a committed remote transaction back.
I would not fix it inside this PR, for the reason the comment itself states as an alternative: "keeping the partitions while the result is unknown" is not a stable end state either. The temp partitions it would keep are unpublished, they belong to a task the overwrite manager still holds, and the next allTaskFail (a master transfer, a restart) drops them -- so the loss is deferred rather than prevented, with an extra uncertain state in between. What prevents it is the property this PR's scope note names: the publication being a committed action of the insert transaction (or the consumption position moving with the publication), so that "committed" and "published" cannot disagree. That is a change to the transaction and journal path, not to this command.
Both remote findings are worth tracking; I would file them as their own issue with the reproduction (a lost commit reply / a KILL during the owner's swap lock wait) rather than growing this PR.
There was a problem hiding this comment.
Tracked in #68679, filed for exactly these paths (remote lost commit reply, remote swap lock wait, ambiguous cloud commit) together with the crash/swap-failure windows this PR's scope note names. It records why an uncertain commit has no correct local resolution -- dropping the partitions loses durable rows, keeping them leaves unpublished ones the next allTaskFail drops -- and the fix directions (publication as a committed action of the insert transaction, or the consumption position moving into the publication's record). This PR stays on the local cancellation and error-response handling.
| * <p>Only a local table is wrapped: a remote table swaps on the frontend that owns it, where this lock | ||
| * says nothing. | ||
| */ | ||
| private void publishTheOverwrite(TableIf targetTable, List<String> partitionNames, |
There was a problem hiding this comment.
[P1] Carry precommit cancellation through the remote swap. For an unpartitioned remote target, SELECT ... WHERE 1 = 0 can return from the inner insert without a transaction. If KILL arrives after the check at line 332 while the owning FE waits for its table write lock in replacePartitionsImpl, this branch supplies no cancellation state or last check: the empty temp partition replaces existing rows and the statement reports success. The existing local lock-wait fix does not cover this remote path; make the owning FE decide under its swap lock before replacing an uncommitted empty result.
There was a problem hiding this comment.
Real gap, but the decision it asks for is the owning frontend's, so I have not taken it here.
The cancellation lives on this FE (the client's session), and replacePartitions carries no cancellation state, so the local last-chance check added in this PR cannot see a KILL that arrives while the owner waits for its table write lock in replacePartitionsImpl -- the check on this side runs before the RPC, and the wait happens after it. Covering it means either conveying the verdict (cancelled with nothing committed) in the replacement RPC so the owner refuses under its own lock, or moving the whole decision to the owner; both are remote-Doris protocol changes that belong with that feature rather than with the local overwrite outcome handling this PR is about.
The local half of the same window is covered (publishTheOverwrite reads the flag under the lock), and the auto-detect route is not reachable with nothing committed on this side, as discussed in the earlier thread.
There was a problem hiding this comment.
Tracked in #68679, filed for exactly these paths (remote lost commit reply, remote swap lock wait, ambiguous cloud commit) together with the crash/swap-failure windows this PR's scope note names. It records why an uncertain commit has no correct local resolution -- dropping the partitions loses durable rows, keeping them leaves unpublished ones the next allTaskFail drops -- and the fix directions (publication as a committed action of the insert transaction, or the consumption position moving into the publication's record). This PR stays on the local cancellation and error-response handling.
| InsertOverwriteUtil.replacePartition(targetTable, partitionNames, tempPartitionNames, | ||
| isForceDropPartition()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[P2] Fail when the target was dropped before the swap. writeLockIfExist() returns false after a concurrent DROP, so this new early return skips replacePartition, but run still calls taskSuccess and reports the overwrite as successful. The previous direct utility path raised an exception in this case; preserve an error outcome and clean the task rather than acknowledging a swap that never occurred.
There was a problem hiding this comment.
Fixed. The wrapper's writeLockIfExist() early return skipped the swap while run went on to taskSuccess, and that was a change from what the utility did: for a dropped table the utility returned from inside a try whose finally { writeUnlock(); } then unlocked a lock that return never took, so the call raised and the statement failed. publishTheOverwrite now raises deliberately, with a message naming the table, so the outcome is the one a client whose swap never happened is owed -- the catch drops the temp partitions of the dropped table and takes the task back.
One note on the other route, which I left alone rather than widen this PR: the auto-detect swap goes through InsertOverwriteManager.taskGroupSuccess into the same utility, so a target dropped under it fails the statement the same way, with that same unlock-after-early-return shape.
| // Committed, visible later: the rows are durable even though the session's visibility-timeout | ||
| // mode may report the timeout as an error. See InsertCommandContext#setCommitted. | ||
| markCommitted(); | ||
| } |
There was a problem hiding this comment.
[P1] Resolve an ambiguous cloud commit before failing the overwrite. FoundationDB can report a maybe-committed result after applying the transaction; after finite meta-service retries this reaches FE as KV_TXN_COMMIT_ERR. commitAndPublishTransactionWithRetry then throws before either markCommitted() call, leaving this context false. The overwrite sees ERR and taskFail drops its temp partitions even if the cloud commit made their rows and stream offsets durable. Determine the cloud transaction state, or keep the partitions when commit remains uncertain, before permitting rollback. This is separate from the returned visibility timeout already discussed.
There was a problem hiding this comment.
Same family as the remote-commit case above, and it needs the same kind of decision, which is why I have not taken it here.
Two things make it a cloud transaction-semantics question rather than an overwrite one. First, the outcome is not decidable locally: after KV_TXN_COMMIT_ERR the meta service may or may not have applied the commit -- CloudInternalCatalog already treats that code specially elsewhere -- so choosing between "publish" and "drop" needs a state query against the meta service, or a policy for uncertain commits, and it applies to every cloud load that reaches that path, not only to overwrites. Second, dropping the partitions on an uncertain commit is wrong in the loss direction and keeping them is wrong in the stuck direction: an unpublished leftover has no owner that will ever publish it, since the overwrite that would have is gone. The durable answer is the same one this PR's scope note names -- make the publication and the commit one event (or carry the consumption position with the publication) -- and until that exists, an uncertain commit has no correct local resolution.
Happy to file this alongside the remote findings as a separate issue if that is useful.
There was a problem hiding this comment.
Tracked in #68679, filed for exactly these paths (remote lost commit reply, remote swap lock wait, ambiguous cloud commit) together with the crash/swap-failure windows this PR's scope note names. It records why an uncertain commit has no correct local resolution -- dropping the partitions loses durable rows, keeping them leaves unpublished ones the next allTaskFail drops -- and the fix directions (publication as a committed action of the insert transaction, or the consumption position moving into the publication's record). This PR stays on the local cancellation and error-response handling.
The last-chance wrapper took the target table's write lock itself and returned silently when the table no longer existed, after which run() acknowledged the overwrite with a success. Before the wrapper, the utility's own early return for a dropped table ran into a finally that unlocks a lock that return never took, so it raised and the statement failed. That outcome is the one a client whose swap never happened is owed, and it is now taken deliberately, with a message naming the table. Key changes: - publishTheOverwrite raises for a dropped target instead of skipping the swap. The existing catch drops the temp partitions of the dropped table and takes the task back. Unit Test: - regression-test/suites/insert_overwrite_p0/test_insert_overwrite_cancel.groovy. The dropped-target path needs a concurrent DROP and has no injection point of its own, so it is reasoned rather than driven. - test_insert_overwrite_cancel, test_iot_range_nereids, test_iot_auto_detect and mtmv_p0/ivm/test_ivm_basic_mtmv pass.
|
run buildall |
|
/review |
publishTheOverwrite raises when it cannot take the target table's write lock, which is the state a concurrent DROP leaves. The branch is driven directly, because reaching it through a statement needs a DROP concurrent with the swap and the command has no injection point for that; the condition the branch reads is what the mock supplies. Unit Test: - InsertOverwriteDroppedTargetTest#targetDroppedBeforeTheSwapFailsTheOverwrite (Tests run: 1, Failures: 0)
|
The remaining windows this PR does not fix -- the FE stopping between the two halves (crash, master switch), a This PR stays on what it fixed: a cancellation that found nothing committed fails the statement, one that reaches the swap under its lock is still honoured, one over committed rows completes the swap, and an error response over committed rows (the session's visibility-timeout mode) publishes them. |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Review conclusion
Complete static review; changes requested on the three existing P1 threads below. The live base/head still match 0a3595e61870ed302eb8b732f968e48823bc0cc4 / 9039a08b9caaa367c748d43cf682bdfb9f9d8e3c. I reviewed all 10 changed files, the insert and overwrite call chains, the test fixtures and expected output, the existing inline threads, and the local, cloud, remote, auto-detect, and connector paths. The normal full-review passes and separate risk-focused pass all returned NO_NEW_VALUABLE_FINDINGS; the final changed-file sweep found no distinct new issue. No builds or tests were run in this review.
The new committed-state marker and local lock-time cancellation check address the ordinary local cancellation, empty-plan, and returned publication-timeout paths. Three existing P1 comments remain valid on this head and are not repeated inline:
- 4142311115 — lost remote commit reply: the owning FE may commit before
FeServiceClient.masterCallWithRetryexhausts retries.RemoteOlapInsertExecutorthen leaves the marker false, and overwrite cleanup can drop the committed temporary partitions. - 4142311125 — remote swap lock wait: the owning FE waits for its table write lock after this FE checks cancellation. The remote replacement RPC carries no cancellation verdict, so an empty uncommitted result can still replace prior rows after KILL.
- 4142311141 — ambiguous cloud commit: a maybe-committed meta-service result can surface as
KV_TXN_COMMIT_ERRbeforeOlapInsertExecutor.markCommitted(). Cleanup may drop partitions whose rows and stream offsets were committed.
The existing P2 auto-detect cancellation thread 4140574889 already covers the false-success concern for an empty no-transaction PARTITION(*) plan. With enable_strict_consistency_dml=false, its plan can avoid the exchange assumed in the author's reply; a cancellation after the branch's check can still be acknowledged while taskGroupSuccess waits for a lock. That path has no BE-created group tasks or partition replacement, and I have not duplicated the thread.
Required checkpoints
- Goal and proof: The change aims to fail cancellation before durable work and finish publication after durable work. The code and added regression cases cover the ordinary local cases and returned visibility timeout; the three existing P1 paths above keep the full data-correctness goal unmet.
- Scope and clarity: The change is confined to insert outcome propagation, overwrite decisions, and targeted tests. The marker is passed through the existing insert context without a new protocol field.
- Concurrency and locks: Client cancellation sets the outer command's atomic flag and cancels its coordinator. The explicit local path rereads that flag under the target table's reentrant write lock, immediately before replacement. The nested utility lock is balanced. The remote owner has a separate lock and the unresolved gap above. The wrapper adds no new lock order or new journal wait beyond the utility's existing replacement path.
- Lifecycle and cleanup: Before registration, a cancel creates no task; after registration, exceptions enter task cleanup. A dropped local target now raises instead of acknowledging a skipped swap. The remaining uncertain-commit paths can still clean up durable temporary partitions. No static initialization or ownership cycle was added.
- Configuration: No new configuration item was added. Existing visibility-timeout return mode and strict-DML-consistency settings affect the branches described above; no dynamic configuration propagation is introduced.
- Compatibility: No persisted format, function symbol, FE/BE message, or rolling-upgrade contract changed. The remote cancellation gap would require an owner-side decision or protocol change and remains open.
- Parallel paths and conditions: Local, remote, cloud, connector, explicit-partition, and auto-detect paths were traced. Transaction-model overwrite is rejected by
StmtExecutor; group commit is excluded by the overwrite context. Local returned commit outcomes set the marker, but remote and cloud uncertain outcomes remain unresolved. The new conditions have comments, with the auto-detect edge already raised in its existing thread. - Tests and results: The suite checks precommit failure, committed-row swap, empty-plan and lock-check failures, and local publication timeout; the
.outrows match those scenarios. The cloud timeout case is gated while cancellation cases remain enabled. The dropped-target unit test exercises the false lock return. No test here establishes the remote or ambiguous cloud outcomes, and this review did not execute tests or independently validate the author's test-run claims. - Observability: The new path logs committed cancellation, continued publication over an error response, and dropped-target failure with table/query context. It adds no metric; existing transaction and task logs remain the primary tracing route.
- Transactions, persistence, and failover: The marker is transient, as intended for one statement. The existing transaction manager commits and journals data and stream offsets; the later partition swap remains a separate operation. The PR does not make that pair atomic across failover or ambiguous commit, and the existing P1 threads identify two concrete loss paths.
- Data writes and crashes: Local returned publication timeout preserves committed rows through the swap. The remote lost-reply and cloud ambiguous-commit cases can still discard committed rows and advanced stream offsets. A crash between commit and swap remains outside this PR's stated scope and is not claimed as fixed.
- FE/BE variables and performance: No new transmitted variable or significant hot-path allocation was added. The added checks are constant-time; the local wrapper holds the table lock only across the precheck and the replacement work that already required that lock.
- Other issues and focus: No additional user-provided focus was supplied. Every new suspicion was either dismissed with code evidence or matched to an existing inline thread. No new inline finding remains unresolved or needs reposting.
Existing P0/P1 findings confirmed for this head: #68662 (comment), #68662 (comment), #68662 (comment)
…route too An auto-detect overwrite (PARTITION(*)) publishes through the overwrite manager, so it bypassed the check the explicit-partition route got. With enable_strict_consistency_dml off, an empty-plan auto-detect overwrite has no exchange in its plan -- VEMPTYSET is the sink's direct child -- so it takes the insert's no-transaction path, and a cancellation landing while the swap waits for the table lock was answered with a success for a statement that did nothing. Both routes now publish through one place that holds the target table's write lock and reads the flag under it. Key changes: - publishTheOverwrite takes the publication as an action, so the explicit-partition swap and the auto-detect task group share the same last-chance decision, and the same failure for a target that was dropped. - The suite drives the auto-detect shape: with strict DML consistency off, a cancelled PARTITION(*) overwrite of an empty plan fails and the table keeps its rows. Unit Test: - regression-test/suites/insert_overwrite_p0/test_insert_overwrite_cancel.groovy - test_insert_overwrite_cancel, test_iot_auto_detect, test_iot_auto_detect_fail, test_iot_auto_detect_concurrent, test_iot_overwrite_and_create and mtmv_p0/ivm/test_ivm_basic_mtmv pass.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review complete for PR #68662 at b2f346c. Changes requested: two new inline findings (P1 test failure, P2 table-lock regression). Three earlier P1 findings still apply and are referenced by ID without duplicate comments.
Critical checkpoints:
- Goal and proof: The local overwrite path now distinguishes a cancelled insert that committed nothing from one whose rows or stream offsets committed, and retains a committed visibility-timeout response through the swap. The regression suite covers the explicit, auto-detect, empty-plan, lock-wait, and local visibility-timeout cases by static inspection. Its new dropped-target unit test cannot run its intended assertion (new P1).
- Scope and clarity: The context marker and shared publication helper are focused on the stated cancellation boundary. The invalid reflective test call and the overly broad auto-detect lock scope need correction.
- Concurrency and locks: The statement thread reads its insert context after the executor returns; cancellation may set an atomic flag from another thread. The local swap rechecks that flag under the target table's reentrant write lock. The new wrapper also holds that lock through two synchronous edit-log writes per auto-detect task (new P2). The BE task-group RPC takes the group lock before a table lock, but publication does not take the group lock; no additional lock cycle was established. Remote swap locking remains the previously reported P1.
- Lifecycle and cleanup: Normal local/cloud, remote OK, and external commit paths set the marker; the no-transaction empty-plan path leaves it false. Task cleanup follows publication failure after the local table lock is released. A remote lost commit reply and an ambiguous cloud commit can still leave durable rows with a false marker (existing P1s).
- Configuration and compatibility: No new runtime configuration, persisted format, FE/BE field, or cross-version protocol is introduced. The tests set and restore existing session variables; the cloud-only visibility behavior is guarded.
- Parallel paths and conditions: Explicit partitions and PARTITION(*) share the local last-chance cancellation decision. Remote publication intentionally bypasses the local lock, leaving the previously reported remote owner-side wait gap. The committed-state condition and separate one-shot debug points are coherent on the normal paths.
- Tests and results: Ordered expected rows in the new .out file match the six regression snapshots, and expected errors use the test exception form. The dropped-target JUnit call has the wrong arity and will fail before testing its branch. Builds and tests were prohibited by this review contract; these conclusions are static only, not an independent runtime validation of author-reported runs.
- Observability: The added cancellation, committed-error, and task logs carry query or task context; no separate metric requirement was identified.
- Transactions, persistence, and data writes: No new journal state or replay format was added. The normal committed local result proceeds to the swap, while an uncommitted cancellation rolls back temporary partitions. The remote lost-reply, remote lock-wait, and ambiguous cloud-commit paths remain unresolved as earlier P1 threads 4142311115, 4142311125, and 4142311141; they are also tracked in #68679. Crash/swap-failure atomicity remains outside this change's implemented boundary. The new auto-detect wrapper adds journal waits inside the table lock (new P2).
- Performance, memory, and other risks: The per-task journal waits can block readers and writers for large PARTITION(*) overwrites. No new material memory ownership issue or other distinct substantiated finding was found after two complete review rounds and the final changed-file sweep.
User focus: no additional focus was provided. All ten changed files were swept; no candidate remains unresolved. Existing P1 threads 4140574895 and 4140980598 are addressed on this head and are excluded from the blocking-ID list.
Existing P0/P1 findings confirmed for this head: #68662 (comment), #68662 (comment), #68662 (comment)
| Mockito.when(ctx.getQueryIdentifier()).thenReturn("stmt[1, query-id]"); | ||
|
|
||
| UserException thrown = Assertions.assertThrows(UserException.class, | ||
| () -> Deencapsulation.invoke(command, "publishTheOverwrite", droppedTarget, |
There was a problem hiding this comment.
[P1] Call the current publication method in this test. This invocation passes five arguments (target, two name lists, context, connection), but the only publishTheOverwrite overload takes four (target, insert context, connection, publication callback). Deencapsulation requires equal arity and throws IllegalArgumentException before the dropped-target branch; assertThrows(UserException.class) therefore fails whenever this test runs. Pass a publication callback and assert it was not invoked after writeLockIfExist() returns false.
There was a problem hiding this comment.
My slip, and you are right. The previous commit's refactor changed publishTheOverwrite's signature and I did not re-run the unit test afterwards. Reproduced exactly as described: IllegalArgumentException: No compatible method found: publishTheOverwrite(OlapTable, ArrayList, ArrayList, OlapInsertCommandContext, ConnectContext), so assertThrows(UserException.class) failed before reaching the branch.
Fixed the way you suggest: the test invokes the current method with a publication callback and asserts the callback was not reached (assertFalse(published.get())). The callback interface is package-private so a test in the same package can name it. Tests run: 1, Failures: 0, Errors: 0 on this head.
| throw cancelledBeforeTheRowsWereCommitted("after a load that committed nothing", ctx); | ||
| } | ||
| publishTheOverwrite(targetTable, insertCtx, ctx, | ||
| () -> insertOverwriteManager.taskGroupSuccess(groupId, (OlapTable) targetTable, |
There was a problem hiding this comment.
[P2] Release the table write lock before task-group journal writes. The new wrapper holds that lock for the entire taskGroupSuccess call. After replacePartition returns, taskGroupSuccess writes an ADD and a DROP edit-log entry for every group task, and each logEdit waits for journal completion. On a PARTITION(*) overwrite spanning many partitions, readers and writers of the target stay blocked through 2N serial journal waits; previously the utility released the lock after the swap, before this bookkeeping. Keep the last cancellation check and replacement under the lock, then finish task logging after unlocking.
There was a problem hiding this comment.
Confirmed and fixed the way you describe. I checked the durability this path actually uses before answering: BDBEnvironment builds Durability(master_sync_policy, replica_sync_policy, replica_ack_policy) from the defaults SYNC / SYNC / SIMPLE_MAJORITY, so every logEdit in that bookkeeping waits for a replica acknowledgement -- and my wrapper was holding the target's write lock across 2N of them.
taskGroupSuccess is now split into replacePartitionsOfTaskGroup (the replacement) and finishTaskGroup (the per-task journaling and the task cleanup). The overwrite calls the replacement inside publishTheOverwrite -- under the lock, with the last-chance cancellation check -- and the bookkeeping after it returns. The owner-side entry (FrontendServiceImpl -> InsertOverwriteManager.taskGroupSuccess) keeps calling the composed method, and for a remote target the manager's replacement call is the existing RPC (the owner runs the whole sequence behind it) with finishTaskGroup a no-op there. So the lock again covers only the replacement and the last check, not the journal waits.
…lock Two follow-ups from the previous commit. The task-group publication ran all of its bookkeeping under the target table's write lock, which the wrapper holds: that bookkeeping writes an edit-log entry for every task (an ADD and a DROP each), and with the default journal durability (SYNC/SYNC/SIMPLE_MAJORITY) every one of them waits for a replica acknowledgement, so a PARTITION(*) overwrite spanning many partitions kept readers and writers of the target blocked through all of those waits. And the unit test added for the dropped-target branch still called the publication method with its pre-refactor signature, so its reflective call never reached the branch it was written for. Key changes: - taskGroupSuccess is split into the replacement and the bookkeeping. The overwrite calls the replacement under its last-chance lock and the bookkeeping after it; the owner-side entry point keeps calling the composed method, so that path is unchanged. - The dropped-target test calls the current method with a publication callback and asserts the callback was not reached. Unit Test: - InsertOverwriteDroppedTargetTest#targetDroppedBeforeTheSwapFailsTheOverwrite (Tests run: 1, Failures: 0, Errors: 0) - test_insert_overwrite_cancel, test_iot_auto_detect, test_iot_auto_detect_fail, test_iot_auto_detect_concurrent and test_iot_overwrite_and_create pass.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68662 at head 1c28718. I reviewed all 13 changed files, traced the affected FE insert and overwrite paths, completed the risk scan and focused follow-up, and found no new inline issue to add.
- Goal and tests: The local explicit-partition and PARTITION(*) paths now fail cancellation before an insert transaction commits and finish publication after a confirmed commit. The dropped-target unit test and ordered regression cases cover the intended branches by static inspection. I did not run builds or tests under this review contract, so runtime behavior is not independently verified here.
- Scope and parallel paths: The change is focused on insert outcome tracking, overwrite publication, manager bookkeeping, and tests. Local OLAP, remote OLAP, and external/plugin executor paths were traced. The candidate concerning a successful empty transaction was dismissed: it crosses the PR's stated commit boundary and the subsequent swap completes the requested overwrite.
- Concurrency and lifecycle: The cancellation flag is atomic. The local final check and replacement share the table write lock; task-group edit-log bookkeeping follows after unlocking, avoiding serial journal waits under that lock. Task cleanup after a completed replacement only targets temporary partitions. No new static/global lifecycle, lock-order, or memory-ownership change was found.
- Conditions, errors, and data correctness: The context marker is set after acknowledged commits, including a local publication timeout, while the no-transaction path leaves it false. The local error response after a committed visibility timeout preserves publication. Three already-reported P1 paths still apply: a lost remote commit reply (4142311115), cancellation during the remote owner's swap lock wait (4142311125), and an ambiguous cloud commit (4142311141). Those existing inline threads are the blocking disposition; I am not duplicating them. The broader commit/swap crash window is outside this local fix and is already tracked in the PR discussion.
- Persistence, compatibility, and configuration: The task-group split preserves the existing replacement and task journal operations. No new FE/BE field, wire format, persistent format, or configuration item is introduced. The change does not alter visible-version or delete-bitmap handling.
- Coverage, observability, and performance: The added unit case targets a dropped table; regression cases use scoped debug points, error assertions, ordered expected results, and a cloud guard for the FE publication timeout. New cancellation and continuation logs include query identifiers. The narrower table-lock span addresses the apparent bookkeeping latency risk; no further hot-path cost was substantiated.
User focus: no additional focus was supplied. Review execution is complete on this head, with the three existing P1 findings still blocking.
Existing P0/P1 findings confirmed for this head: #68662 (comment), #68662 (comment), #68662 (comment)
TPC-H: Total hot run time: 27286 ms |
TPC-DS: Total hot run time: 152392 ms |
ClickBench: Total hot run time: 23.91 s |
FE Regression Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Related PR: #68390
Problem Summary:
An
INSERT OVERWRITEis two-phase: the rows are committed into temporary partitions first, and a later swap publishes them. A cancellation that lands between the two halves cannot take anything back -- the rows are durable, and everything the read consumed, the base table stream offsets among it, was committed with that same transaction (InsertIntoTableCommandhangs the offsets on the transaction,DatabaseTransactionMgr.updateCatalogAfterCommittedapplies them on commit and replays them). The command answered that window by dropping the temporary partitions and returning normally:INSERT OVERWRITE t SELECT * FROM stream(...), and also of an IVM partition refresh, which resets the offsets of exactly the partitions it replaces.The cancellations that land before the rows are committed were answered the same way: success for a statement that did nothing.
Fix: decide by whether the rows are already committed.
(it begins no transaction), so the temporary partitions hold no rows and there is no offset to keep -- nothing
durable happened, so the statement fails, like the cancellation the inner insert already reports for itself, and a
re-run reads the same rows. The auto-detect route takes the same decision.
A debug point (
stage=beforeTheInsert|afterTheInsert, scoped by table name) injects the cancellation at either side, in the same shape as the existingfailBetweenTheTwoHalvesOfAnOverwrite.Scope: this closes the cancellation, which is the one entry into the window that reports success and therefore cannot be recovered by any later refresh. #68390's per-partition rebuild requirement, raised before a refresh reads, covers the failure and master-switch halves of the same window -- but a cancellation defeats it, precisely because the statement says it succeeded and the refresh records its epochs as met. The crash window and a
replacePartitionthat throws are unchanged and still need the swap to become a committed action of the insert transaction.Release note
KILL/cancellation of anINSERT OVERWRITEthat lands after its rows were committed now completes the overwrite instead of silently reporting a success that published nothing. A cancellation before that point -- including one that lands where the insert committed nothing at all, such as an overwrite whose plan folded to an empty relation -- now reports an error and leaves the target untouched, instead of reporting success for a statement that did nothing or emptying the table for a statement that was cancelled.Check List (For Author)
insert_overwrite_p0/test_insert_overwrite_cancelcovers both sides;insert_overwrite_p0in full (14 suites) andmtmv_p0/ivm/test_ivm_overwrite_failure_between_the_halvespass locally. The new suite cannot fail on the pre-change code: the injection point it needs is introduced by this change, the same way [feature](ivm) Track the refresh baseline per MV partition, not by a rebuild barrier #68390 added the point its suite uses.INSERT OVERWRITEreports failure when it committed nothing, and success only after completing the swap, instead of reporting success in both cases.