korlov42 commented on code in PR #7309: URL: https://github.com/apache/ignite-3/pull/7309#discussion_r2651067940
########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/SqlPlanOutdatedException.java: ########## @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.sql.engine.exec; + +import org.apache.ignite.internal.lang.IgniteInternalException; +import org.apache.ignite.internal.tx.InternalTransaction; +import org.apache.ignite.lang.ErrorGroups.Common; + +/** + * Exception occurs when SQL engine detects that the execution plan is outdated after preparation. + * + * <p>It is used internally to signal the SQL engine to retry the optimization step + * using the transaction {@link InternalTransaction#schemaTimestamp() schema timestamp}. + * This exception should never be passed to the user, and has no special error code. + */ +public class SqlPlanOutdatedException extends IgniteInternalException { Review Comment: I don't think we need to create ControlFlowException as full-fledged IgniteException. Let's extend RuntimeException, cut off thread dump (no one will ever see it) ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/QueryExecutionProgram.java: ########## @@ -75,6 +80,32 @@ private QueryExecutionProgram() { static boolean errorHandler(Query query, Throwable th) { if (canRecover(query, th)) { query.error.set(null); + + if (multiStepPlanOutdated(th)) { + assert query.plan instanceof MultiStepPlan; + assert query.currentPhase() == ExecutionPhase.CURSOR_INITIALIZATION : query.currentPhase(); + + SqlOperationContext context = query.operationContext; + QueryTransactionWrapper txWrapper = query.usedTransaction; + + assert context != null; + assert txWrapper != null; + + context.getAndSetTxOnRetry(txWrapper); Review Comment: currently, error handling for this case is scattered across number of places. I would propose to move all preparation here. Looks like we just need to recreate operationContext with all necessary changes (operation time, transaction to use, if any). ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/KeyValueGetPlan.java: ########## @@ -316,4 +316,12 @@ private abstract static class Performable<RowT> { public int catalogVersion() { return catalogVersion; } + + @Override + public boolean lazyCursorPublication() { + // In order to repeat execution in case of an error where the replica request schema Review Comment: I would rephrase this like ``` Let's postpone cursor publication so we can recover from errors during plan execution, like `InternalSchemaVersionMismatchException` ``` ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlPlanToTxSchemaVersionValidator.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.sql.engine; + +import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture; + +import java.util.concurrent.CompletableFuture; +import org.apache.ignite.internal.catalog.CatalogService; +import org.apache.ignite.internal.hlc.HybridTimestamp; +import org.apache.ignite.internal.schema.SchemaSyncService; +import org.apache.ignite.internal.sql.engine.exec.SqlPlanOutdatedException; +import org.apache.ignite.internal.sql.engine.prepare.MultiStepPlan; +import org.apache.ignite.internal.sql.engine.tx.QueryTransactionWrapper; +import org.apache.ignite.internal.tx.InternalTransaction; +import org.jetbrains.annotations.TestOnly; + +/** + * SQL query execution plan validator. + * + * <p>Performs validation of the catalog version from the {@link MultiStepPlan multi-step plan} relative to the started transaction. + */ +public class SqlPlanToTxSchemaVersionValidator { + @TestOnly + public static final SqlPlanToTxSchemaVersionValidator NOOP = new NoopSqlPlanToTxSchemaVersionValidator(); + + private final SchemaSyncService schemaSyncService; + private final CatalogService catalogService; + + /** + * Compares the catalog version from the plan with the version of the active catalog + * at the {@link InternalTransaction#schemaTimestamp() schema time} of the transaction. + * + * @param plan {@link MultiStepPlan multi-step} execution plan. + * @param tx Query transaction wrapper. + * @return Successfully completed future if the provided transaction is explicit or the catalog versions match. + * Otherwise returns a future completed with an exception {@link SqlPlanOutdatedException}. + */ + public CompletableFuture<Void> validate(MultiStepPlan plan, QueryTransactionWrapper tx) { + if (!tx.implicit()) { + return nullCompletedFuture(); + } + + HybridTimestamp ts = tx.unwrap().schemaTimestamp(); + + return schemaSyncService.waitForMetadataCompleteness(ts) + .thenRun(() -> { + int requiredCatalog = catalogService.activeCatalogVersion(ts.longValue()); Review Comment: I think, such validation won't work well because there will be many false negatives (meaning valid plan is considered invalid resulting in re-planning). Although single DDL operation is long due to delay duration, multiple independent threads may generate hundreds of catalog versions per second. In such environment, implicit transactions will perform bad due to double planning... On the other hand, even single planning will result in severe performance drop. Perhaps, it worth to reconsider plan cache under separate ticket. May I ask you to file one and add TODO to this place referring newly created ticket? -- 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]
