diff --git a/src/postgres/src/backend/utils/misc/pg_yb_utils.c b/src/postgres/src/backend/utils/misc/pg_yb_utils.c index 041ef72c07c7..eed0d4999a7c 100644 --- a/src/postgres/src/backend/utils/misc/pg_yb_utils.c +++ b/src/postgres/src/backend/utils/misc/pg_yb_utils.c @@ -946,17 +946,37 @@ YBCCommitTransaction() if (!IsYugaByteEnabled()) return; - HandleYBStatus(YBCPgCommitTransaction()); + HandleYBStatus(YBCPgCommitPlainTransaction()); } void YBCAbortTransaction() { - if (!IsYugaByteEnabled()) + if (!IsYugaByteEnabled() || !YBTransactionsEnabled()) return; - if (YBTransactionsEnabled()) - HandleYBStatus(YBCPgAbortTransaction()); + /* + * If a DDL operation during a DDL txn fails, the txn will be aborted before + * we get here. However if there are failures afterwards (i.e. during + * COMMIT or catalog version increment), then we might get here as part of + * top level error recovery in PostgresMain() with the DDL txn state still + * set in pggate. Clean it up in that case. + */ + YBCStatus status = YBCPgClearSeparateDdlTxnMode(); + + /* + * Aborting a transaction is likely to fail only when there are issues + * communicating with the tserver. Close the backend connection in such + * scenarios to avoid a recursive loop of aborting again and again as part + * of error handling in PostgresMain() because of the error faced during + * abort. + */ + if (unlikely(status)) + elog(FATAL, "Failed to abort DDL transaction: %s", YBCMessageAsCString(status)); + + status = YBCPgAbortPlainTransaction(); + if (unlikely(status)) + elog(FATAL, "Failed to abort DML transaction: %s", YBCMessageAsCString(status)); } void diff --git a/src/yb/yql/pggate/pg_txn_manager.cc b/src/yb/yql/pggate/pg_txn_manager.cc index 7578d6ed10e5..303f2ccd8c01 100644 --- a/src/yb/yql/pggate/pg_txn_manager.cc +++ b/src/yb/yql/pggate/pg_txn_manager.cc @@ -164,7 +164,8 @@ PgTxnManager::PgTxnManager( PgTxnManager::~PgTxnManager() { // Abort the transaction before the transaction manager gets destroyed. - WARN_NOT_OK(AbortTransaction(), "Failed to abort transaction in dtor"); + WARN_NOT_OK(ExitSeparateDdlTxnModeWithAbort(), "Failed to abort DDL transaction in dtor"); + WARN_NOT_OK(AbortPlainTransaction(), "Failed to abort DML transaction in dtor"); } Status PgTxnManager::BeginTransaction(int64_t start_time) { @@ -360,16 +361,21 @@ void PgTxnManager::SetActiveSubTransactionId(SubTransactionId id) { active_sub_transaction_id_ = id; } -Status PgTxnManager::CommitTransaction() { - return FinishTransaction(Commit::kTrue); +Status PgTxnManager::CommitPlainTransaction() { + return FinishPlainTransaction(Commit::kTrue); } -Status PgTxnManager::FinishTransaction(Commit commit) { - // If a DDL operation during a DDL txn fails the txn will be aborted before we get here. - // However if there are failures afterwards (i.e. during COMMIT or catalog version increment), - // then we might get here with a ddl_txn_. Clean it up in that case. - if (IsDdlMode() && !commit) { - RETURN_NOT_OK(ExitSeparateDdlTxnModeWithAbort()); +Status PgTxnManager::AbortPlainTransaction() { + return FinishPlainTransaction(Commit::kFalse); +} + +Status PgTxnManager::FinishPlainTransaction(Commit commit) { + if (PREDICT_FALSE(IsDdlMode())) { + // GH #22353 - A DML txn must be aborted or committed only when there is no active DDL txn + // (ie. after any active DDL txn has itself committed or aborted). Silently ignoring this + // scenario may lead to errors in the future. Convert this to an SCHECK once the GH issue is + // resolved. + LOG(WARNING) << "Received DML txn commit with DDL txn state active"; } if (!txn_in_progress_) { @@ -383,6 +389,10 @@ Status PgTxnManager::FinishTransaction(Commit commit) { return Status::OK(); } + // Aborting a transaction is on a best-effort basis. In the event that the abort RPC to the + // tserver fails, we simply reset the transaction state here and return any error back to the + // caller. In the event that the tserver recovers, it will eventually expire the transaction due + // to inactivity. VLOG_TXN_STATE(2) << (commit ? "Committing" : "Aborting") << " transaction."; Status status = client_->FinishTransaction(commit); VLOG_TXN_STATE(2) << "Transaction " << (commit ? "commit" : "abort") << " status: " << status; @@ -390,10 +400,6 @@ Status PgTxnManager::FinishTransaction(Commit commit) { return status; } -Status PgTxnManager::AbortTransaction() { - return FinishTransaction(Commit::kFalse); -} - void PgTxnManager::ResetTxnAndSession() { txn_in_progress_ = false; isolation_level_ = IsolationLevel::NON_TRANSACTIONAL; @@ -437,9 +443,17 @@ Status PgTxnManager::ExitSeparateDdlTxnMode(const std::optional& if (commit_info && commit_info->is_silent_altering) { ddl_mode->silently_altered_db = commit_info->db_oid; } - RETURN_NOT_OK(client_->FinishTransaction(commit_info ? Commit::kTrue : Commit::kFalse, ddl_mode)); - ddl_mode_.reset(); - return Status::OK(); + + Commit commit = commit_info ? Commit::kTrue : Commit::kFalse; + Status status = client_->FinishTransaction(commit, ddl_mode); + WARN_NOT_OK(status, Format("Failed to $0 DDL transaction", commit ? "commit" : "abort")); + if (PREDICT_TRUE(status.ok() || !commit)) { + // In case of an abort, reset the DDL mode here as we may later re-enter this function and retry + // the abort as part of transaction error recovery if the status is not ok. + ddl_mode_.reset(); + } + + return status; } void PgTxnManager::SetDdlHasSyscatalogChanges() { diff --git a/src/yb/yql/pggate/pg_txn_manager.h b/src/yb/yql/pggate/pg_txn_manager.h index 9d373ae2d416..1fd3895c33b7 100644 --- a/src/yb/yql/pggate/pg_txn_manager.h +++ b/src/yb/yql/pggate/pg_txn_manager.h @@ -61,8 +61,8 @@ class PgTxnManager : public RefCountedThreadSafe { Status RestartReadPoint(); bool IsRestartReadPointRequested(); void SetActiveSubTransactionId(SubTransactionId id); - Status CommitTransaction(); - Status AbortTransaction(); + Status CommitPlainTransaction(); + Status AbortPlainTransaction(); Status SetPgIsolationLevel(int isolation); PgIsolationLevel GetPgIsolationLevel(); Status SetReadOnly(bool read_only); @@ -108,7 +108,7 @@ class PgTxnManager : public RefCountedThreadSafe { std::string TxnStateDebugStr() const; - Status FinishTransaction(Commit commit); + Status FinishPlainTransaction(Commit commit); void IncTxnSerialNo(); diff --git a/src/yb/yql/pggate/pggate.cc b/src/yb/yql/pggate/pggate.cc index bd2572efdf89..1157d859ad53 100644 --- a/src/yb/yql/pggate/pggate.cc +++ b/src/yb/yql/pggate/pggate.cc @@ -2115,16 +2115,16 @@ bool PgApiImpl::IsRestartReadPointRequested() { return pg_txn_manager_->IsRestartReadPointRequested(); } -Status PgApiImpl::CommitTransaction() { +Status PgApiImpl::CommitPlainTransaction() { DCHECK(pg_session_->explicit_row_lock_buffer().IsEmpty()); pg_session_->InvalidateForeignKeyReferenceCache(); RETURN_NOT_OK(pg_session_->FlushBufferedOperations()); - return pg_txn_manager_->CommitTransaction(); + return pg_txn_manager_->CommitPlainTransaction(); } -Status PgApiImpl::AbortTransaction() { +Status PgApiImpl::AbortPlainTransaction() { ClearSessionState(); - return pg_txn_manager_->AbortTransaction(); + return pg_txn_manager_->AbortPlainTransaction(); } Status PgApiImpl::SetTransactionIsolationLevel(int isolation) { diff --git a/src/yb/yql/pggate/pggate.h b/src/yb/yql/pggate/pggate.h index b09bdca40276..092b24d01e28 100644 --- a/src/yb/yql/pggate/pggate.h +++ b/src/yb/yql/pggate/pggate.h @@ -647,8 +647,8 @@ class PgApiImpl { Status ResetTransactionReadPoint(); Status RestartReadPoint(); bool IsRestartReadPointRequested(); - Status CommitTransaction(); - Status AbortTransaction(); + Status CommitPlainTransaction(); + Status AbortPlainTransaction(); Status SetTransactionIsolationLevel(int isolation); Status SetTransactionReadOnly(bool read_only); Status SetTransactionDeferrable(bool deferrable); diff --git a/src/yb/yql/pggate/test/pggate_test.cc b/src/yb/yql/pggate/test/pggate_test.cc index dedbed214514..feb9dd54f141 100644 --- a/src/yb/yql/pggate/test/pggate_test.cc +++ b/src/yb/yql/pggate/test/pggate_test.cc @@ -240,7 +240,7 @@ void PggateTest::BeginTransaction() { } void PggateTest::CommitTransaction() { - CHECK_YBC_STATUS(YBCPgCommitTransaction()); + CHECK_YBC_STATUS(YBCPgCommitPlainTransaction()); } void PggateTest::ExecCreateTableTransaction(YBCPgStatement pg_stmt) { diff --git a/src/yb/yql/pggate/ybc_pggate.cc b/src/yb/yql/pggate/ybc_pggate.cc index 05535becbe78..c21d5e57904e 100644 --- a/src/yb/yql/pggate/ybc_pggate.cc +++ b/src/yb/yql/pggate/ybc_pggate.cc @@ -1689,12 +1689,12 @@ bool YBCIsRestartReadPointRequested() { return pgapi->IsRestartReadPointRequested(); } -YBCStatus YBCPgCommitTransaction() { - return ToYBCStatus(pgapi->CommitTransaction()); +YBCStatus YBCPgCommitPlainTransaction() { + return ToYBCStatus(pgapi->CommitPlainTransaction()); } -YBCStatus YBCPgAbortTransaction() { - return ToYBCStatus(pgapi->AbortTransaction()); +YBCStatus YBCPgAbortPlainTransaction() { + return ToYBCStatus(pgapi->AbortPlainTransaction()); } YBCStatus YBCPgSetTransactionIsolationLevel(int isolation) { diff --git a/src/yb/yql/pggate/ybc_pggate.h b/src/yb/yql/pggate/ybc_pggate.h index 64af08c45965..4281f00e11a4 100644 --- a/src/yb/yql/pggate/ybc_pggate.h +++ b/src/yb/yql/pggate/ybc_pggate.h @@ -659,8 +659,8 @@ YBCStatus YBCPgRestartTransaction(); YBCStatus YBCPgResetTransactionReadPoint(); YBCStatus YBCPgRestartReadPoint(); bool YBCIsRestartReadPointRequested(); -YBCStatus YBCPgCommitTransaction(); -YBCStatus YBCPgAbortTransaction(); +YBCStatus YBCPgCommitPlainTransaction(); +YBCStatus YBCPgAbortPlainTransaction(); YBCStatus YBCPgSetTransactionIsolationLevel(int isolation); YBCStatus YBCPgSetTransactionReadOnly(bool read_only); YBCStatus YBCPgSetTransactionDeferrable(bool deferrable);