yujun777 commented on code in PR #68662:
URL: https://github.com/apache/doris/pull/68662#discussion_r4142866239
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertOverwriteTableCommand.java:
##########
@@ -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);
Review 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`).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/RemoteOlapInsertExecutor.java:
##########
@@ -208,6 +208,7 @@ protected void onComplete() throws UserException {
} else {
txnStatus = TransactionStatus.COMMITTED;
}
+ markCommitted();
LOG.info("commit remote txn success, catalog={}, dbId={},
txnId={}, status={}",
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertOverwriteTableCommand.java:
##########
@@ -386,12 +440,87 @@ private static void
failBetweenTheTwoHalvesOfAnOverwrite(TableIf targetTable) th
throw new UserException("debug point: " +
DEBUG_POINT_FAIL_BETWEEN_THE_HALVES_OF_AN_OVERWRITE);
}
+ /**
+ * Cancels this overwrite when the debug point names the table it targets;
see the constants above.
+ * Nothing here decides what a cancelled overwrite means -- the call sites
do, and they differ: the ones
+ * before the rows are durable take the statement back, the one after them
does not.
+ *
+ * <p>One lookup, because a point is consumed by the lookup that reads it:
reading it twice with
+ * {@code execute=1} armed would have the first read spend the allowance
and the point be gone before the
+ * second, which would silently leave the overwrite uncancelled.
+ */
+ private void cancelTheOverwriteAt(String debugPointName, TableIf
targetTable) {
+ if
(!targetTable.getName().equals(DebugPointUtil.getDebugParamOrDefault(
+ debugPointName, "table_name", ""))) {
+ return;
+ }
+ LOG.info("debug point {} cancels the overwrite of {}", debugPointName,
targetTable.getName());
+ cancel();
+ }
+
+ /**
+ * Publishes this overwrite by swapping the temp partitions in, with a
last look at the cancellation flag
+ * taken under the lock the swap contends for.
+ *
+ * <p>{@link #run} reads the flag before the swap is issued, and the swap
then waits for the table's write
+ * lock, so a cancellation that arrives during that wait is the one place
a check before the swap cannot
+ * see. Reading it again here costs nothing and is where the wait happens:
for a cancellation with nothing
+ * committed there is nothing durable to publish, so refusing to swap
costs the statement and leaves the
+ * rows the client asked to keep -- while a swap that went ahead would
replace them with empty partitions.
+ *
+ * <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,
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapInsertExecutor.java:
##########
@@ -240,10 +240,14 @@ protected void onComplete() throws UserException {
ctx.getSessionVariable().getInsertVisibleTimeoutMs(),
txnCommitAttachment,
streamUpdateInfos)) {
txnStatus = TransactionStatus.VISIBLE;
+ markCommitted();
} else {
// Keep the committed status so load accounting and insert result
bookkeeping stay aligned.
txnStatus = TransactionStatus.COMMITTED;
publishTimedOutAfterCommit = true;
+ // 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();
}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
