From 059694b0b4f3a6f92f852b0a7be6358d708ee5e0 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Thu, 1 Aug 2019 10:49:45 +0300 Subject: [PATCH 01/24] Improve query restarts logic so that it handles SELECT ... WITH LOCK (with single record), inserts and updates correctly --- src/dsql/dsql.cpp | 114 ++++++++++++++++++++++++++++++++++++---------- src/dsql/dsql.h | 8 +++- src/jrd/jrd.cpp | 72 ++--------------------------- src/jrd/vio.cpp | 3 +- 4 files changed, 105 insertions(+), 92 deletions(-) diff --git a/src/dsql/dsql.cpp b/src/dsql/dsql.cpp index dcee054350..fc215988be 100644 --- a/src/dsql/dsql.cpp +++ b/src/dsql/dsql.cpp @@ -286,7 +286,10 @@ bool DsqlDmlRequest::fetch(thread_db* tdbb, UCHAR* msgBuffer) tdbb->checkCancelState(true); UCHAR* dsqlMsgBuffer = req_msg_buffers[message->msg_buffer_number]; - JRD_receive(tdbb, req_request, message->msg_number, message->msg_length, dsqlMsgBuffer); + if (prefetchedFirstRow) + prefetchedFirstRow = false; + else + JRD_receive(tdbb, req_request, message->msg_number, message->msg_length, dsqlMsgBuffer); const dsql_par* const eof = statement->getEof(); const USHORT* eofPtr = eof ? (USHORT*) (dsqlMsgBuffer + (IPTR) eof->par_desc.dsc_address) : NULL; @@ -677,33 +680,14 @@ void DsqlDmlRequest::dsqlPass(thread_db* tdbb, DsqlCompilerScratch* scratch, boo *destroyScratchPool = true; } -// Execute a dynamic SQL statement. -void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, +// Execute a dynamic SQL statement +void DsqlDmlRequest::doExecute(thread_db* tdbb, jrd_tra** traHandle, Firebird::IMessageMetadata* inMetadata, const UCHAR* inMsg, Firebird::IMessageMetadata* outMetadata, UCHAR* outMsg, bool singleton) { - if (!req_request) - { - ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-504) << - Arg::Gds(isc_unprepared_stmt)); - } - - // If there is no data required, just start the request - + prefetchedFirstRow = false; const dsql_msg* message = statement->getSendMsg(); - if (message) - mapInOut(tdbb, false, message, inMetadata, NULL, inMsg); - - // we need to mapInOut() before tracing of execution start to let trace - // manager know statement parameters values - TraceDSQLExecute trace(req_dbb->dbb_attachment, this); - - // Setup and start timeout timer - const bool have_cursor = reqTypeWithCursor(statement->getType()) && !singleton; - - setupTimer(tdbb); - thread_db::TimerGuard timerGuard(tdbb, req_timer, !have_cursor); if (!message) JRD_start(tdbb, req_request, req_transaction); @@ -805,6 +789,17 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, status_exception::raise(&localStatus); } } + else + { + // Prefetch first row of a query + if (reqTypeWithCursor(statement->getType())) { + dsql_msg* message = (dsql_msg*) statement->getReceiveMsg(); + + UCHAR* dsqlMsgBuffer = req_msg_buffers[message->msg_buffer_number]; + JRD_receive(tdbb, req_request, message->msg_number, message->msg_length, dsqlMsgBuffer); + prefetchedFirstRow = true; + } + } switch (statement->getType()) { @@ -826,6 +821,79 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, } break; } +} + +// Execute a dynamic SQL statement with tracing, restart and timeout handler +void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, + Firebird::IMessageMetadata* inMetadata, const UCHAR* inMsg, + Firebird::IMessageMetadata* outMetadata, UCHAR* outMsg, + bool singleton) +{ + if (!req_request) + { + ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-504) << + Arg::Gds(isc_unprepared_stmt)); + } + + // If there is no data required, just start the request + + const dsql_msg* message = statement->getSendMsg(); + if (message) + mapInOut(tdbb, false, message, inMetadata, NULL, inMsg); + + // we need to mapInOut() before tracing of execution start to let trace + // manager know statement parameters values + TraceDSQLExecute trace(req_dbb->dbb_attachment, this); + + // Setup and start timeout timer + const bool have_cursor = reqTypeWithCursor(statement->getType()) && !singleton; + + setupTimer(tdbb); + thread_db::TimerGuard timerGuard(tdbb, req_timer, !have_cursor); + + int numTries = 0; + TraNumber prev_concurrent_tx = 0; + while (true) + { + try + { + doExecute(tdbb, traHandle, inMetadata, inMsg, outMetadata, outMsg, singleton); + break; + } + catch (const status_exception &ex) + { + const ISC_STATUS* v = ex.value(); + if (// Update conflict error + v[0] == isc_arg_gds && + v[1] == isc_deadlock && + v[2] == isc_arg_gds && + v[3] == isc_update_conflict && + // Read committed transaction with snapshots + (req_transaction->tra_flags & TRA_read_committed) && + (req_transaction->tra_flags & TRA_read_consistency) && + // Snapshot has been assigned to the request - + // it was top-level request + !TRA_get_prior_request(tdbb)) + { + // It makes no sense to repeat statement if we stumble on the same Tx again and again + if (v[4] == isc_arg_gds && + v[5] == isc_concurrent_transaction && + v[6] == isc_arg_number) + { + if (prev_concurrent_tx && prev_concurrent_tx == v[7]) + throw; + + prev_concurrent_tx = v[7]; + } + if (++numTries < 10) + { + fb_utils::init_status(tdbb->tdbb_status_vector); + continue; + } + } + throw; + } + } trace.finish(have_cursor, ITracePlugin::RESULT_SUCCESS); } diff --git a/src/dsql/dsql.h b/src/dsql/dsql.h index e1fcccc593..cf3f004a74 100644 --- a/src/dsql/dsql.h +++ b/src/dsql/dsql.h @@ -645,7 +645,8 @@ public: explicit DsqlDmlRequest(MemoryPool& pool, StmtNode* aNode) : dsql_req(pool), node(aNode), - needDelayedFormat(false) + needDelayedFormat(false), + prefetchedFirstRow(false) { } @@ -664,9 +665,14 @@ public: virtual void setDelayedFormat(thread_db* tdbb, Firebird::IMessageMetadata* metadata); private: + void doExecute(thread_db* tdbb, jrd_tra** traHandle, + Firebird::IMessageMetadata* inMetadata, const UCHAR* inMsg, + Firebird::IMessageMetadata* outMetadata, UCHAR* outMsg, + bool singleton); NestConst node; Firebird::RefPtr delayedFormat; bool needDelayedFormat; + bool prefetchedFirstRow; }; class DsqlDdlRequest : public dsql_req diff --git a/src/jrd/jrd.cpp b/src/jrd/jrd.cpp index 08fb2f2330..cab5a86ff3 100644 --- a/src/jrd/jrd.cpp +++ b/src/jrd/jrd.cpp @@ -8880,39 +8880,8 @@ void JRD_start(Jrd::thread_db* tdbb, jrd_req* request, jrd_tra* transaction) * Get a record from the host program. * **************************************/ - - // Repeat execution to handle update conflicts, if any - int numTries = 0; - while (true) - { - try - { - EXE_unwind(tdbb, request); - EXE_start(tdbb, request, transaction); - break; - } - catch (const status_exception &ex) - { - const ISC_STATUS* v = ex.value(); - if (// Update conflict error - v[0] == isc_arg_gds && - v[1] == isc_update_conflict && - // Read committed transaction with snapshots - (transaction->tra_flags & TRA_read_committed) && - (transaction->tra_flags & TRA_read_consistency) && - // Snapshot has been assigned to the request - - // it was top-level request - !TRA_get_prior_request(tdbb)) - { - if (++numTries < 10) - { - fb_utils::init_status(tdbb->tdbb_status_vector); - continue; - } - } - throw; - } - } + EXE_unwind(tdbb, request); + EXE_start(tdbb, request, transaction); check_autocommit(tdbb, request); @@ -9001,40 +8970,9 @@ void JRD_start_and_send(thread_db* tdbb, jrd_req* request, jrd_tra* transaction, * Get a record from the host program. * **************************************/ - - // Repeat execution to handle update conflicts, if any - int numTries = 0; - while (true) - { - try - { - EXE_unwind(tdbb, request); - EXE_start(tdbb, request, transaction); - EXE_send(tdbb, request, msg_type, msg_length, msg); - break; - } - catch (const status_exception &ex) - { - const ISC_STATUS* v = ex.value(); - if (// Update conflict error - v[0] == isc_arg_gds && - v[1] == isc_update_conflict && - // Read committed transaction with snapshots - (transaction->tra_flags & TRA_read_committed) && - (transaction->tra_flags & TRA_read_consistency) && - // Snapshot has been assigned to the request - - // it was top-level request - !TRA_get_prior_request(tdbb)) - { - if (++numTries < 10) - { - fb_utils::init_status(tdbb->tdbb_status_vector); - continue; - } - } - throw; - } - } + EXE_unwind(tdbb, request); + EXE_start(tdbb, request, transaction); + EXE_send(tdbb, request, msg_type, msg_length, msg); check_autocommit(tdbb, request); diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index aa628a91f5..6b5f83b7c7 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -5791,7 +5791,8 @@ static int prepare_update( thread_db* tdbb, { tdbb->bumpRelStats(RuntimeStatistics::RECORD_CONFLICTS, relation->rel_id); - ERR_post(Arg::Gds(isc_update_conflict) << + ERR_post(Arg::Gds(isc_deadlock) << + Arg::Gds(isc_update_conflict) << Arg::Gds(isc_concurrent_transaction) << Arg::Num(update_conflict_trans)); } From c6247f8a266eadbc59448f5740ac563baa00f3e4 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Fri, 2 Aug 2019 18:14:04 +0300 Subject: [PATCH 02/24] Go back to Vlad's logic to avoid unnecessary restarts --- src/dsql/dsql.cpp | 14 +------------- src/jrd/vio.cpp | 3 +-- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/src/dsql/dsql.cpp b/src/dsql/dsql.cpp index fc215988be..503d8b58d6 100644 --- a/src/dsql/dsql.cpp +++ b/src/dsql/dsql.cpp @@ -865,9 +865,7 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, const ISC_STATUS* v = ex.value(); if (// Update conflict error v[0] == isc_arg_gds && - v[1] == isc_deadlock && - v[2] == isc_arg_gds && - v[3] == isc_update_conflict && + v[1] == isc_update_conflict && // Read committed transaction with snapshots (req_transaction->tra_flags & TRA_read_committed) && (req_transaction->tra_flags & TRA_read_consistency) && @@ -875,16 +873,6 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, // it was top-level request !TRA_get_prior_request(tdbb)) { - // It makes no sense to repeat statement if we stumble on the same Tx again and again - if (v[4] == isc_arg_gds && - v[5] == isc_concurrent_transaction && - v[6] == isc_arg_number) - { - if (prev_concurrent_tx && prev_concurrent_tx == v[7]) - throw; - - prev_concurrent_tx = v[7]; - } if (++numTries < 10) { fb_utils::init_status(tdbb->tdbb_status_vector); diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index 6b5f83b7c7..aa628a91f5 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -5791,8 +5791,7 @@ static int prepare_update( thread_db* tdbb, { tdbb->bumpRelStats(RuntimeStatistics::RECORD_CONFLICTS, relation->rel_id); - ERR_post(Arg::Gds(isc_deadlock) << - Arg::Gds(isc_update_conflict) << + ERR_post(Arg::Gds(isc_update_conflict) << Arg::Gds(isc_concurrent_transaction) << Arg::Num(update_conflict_trans)); } From 87e720b918168f394eac4e6b5ce47de2c700d265 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Fri, 9 Aug 2019 19:28:52 +0300 Subject: [PATCH 03/24] Preserve write locks during query restarts due to update conflicts --- src/dsql/StmtNodes.cpp | 12 +++++++++-- src/dsql/dsql.cpp | 40 +++++++++++++---------------------- src/jrd/Savepoint.cpp | 48 +++++++++++++++++++++++++++++++++++++----- src/jrd/Savepoint.h | 4 ++-- src/jrd/req.h | 2 ++ src/jrd/tra.cpp | 11 ++++++---- src/jrd/tra.h | 2 +- src/jrd/vio.cpp | 44 ++++++++++++++++++++++++++------------ src/jrd/vio_proto.h | 4 ++-- 9 files changed, 113 insertions(+), 54 deletions(-) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index 198d7ed649..d62bb1ce00 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -2643,7 +2643,11 @@ const StmtNode* EraseNode::erase(thread_db* tdbb, jrd_req* request, WhichTrigger VirtualTable::erase(tdbb, rpb); else if (!relation->rel_view_rse) { - VIO_erase(tdbb, rpb, transaction); + while (!VIO_erase(tdbb, rpb, transaction)) + { + if (!VIO_refetch_record(tdbb, rpb, transaction, true, true)) + return parentStmt; + } REPL_erase(tdbb, rpb, transaction); } @@ -6452,7 +6456,11 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg VirtualTable::modify(tdbb, orgRpb, newRpb); else if (!relation->rel_view_rse) { - VIO_modify(tdbb, orgRpb, newRpb, transaction); + while (!VIO_modify(tdbb, orgRpb, newRpb, transaction)) + { + if (!VIO_refetch_record(tdbb, orgRpb, transaction, true, true)) + return parentStmt; + } IDX_modify(tdbb, orgRpb, newRpb, transaction); REPL_modify(tdbb, orgRpb, newRpb, transaction); } diff --git a/src/dsql/dsql.cpp b/src/dsql/dsql.cpp index 503d8b58d6..504caca528 100644 --- a/src/dsql/dsql.cpp +++ b/src/dsql/dsql.cpp @@ -851,36 +851,26 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, setupTimer(tdbb); thread_db::TimerGuard timerGuard(tdbb, req_timer, !have_cursor); - int numTries = 0; - TraNumber prev_concurrent_tx = 0; - while (true) { - try + AutoSavePoint savePoint(tdbb, req_transaction); + int numTries = 0; + while (true) { doExecute(tdbb, traHandle, inMetadata, inMsg, outMetadata, outMsg, singleton); - break; - } - catch (const status_exception &ex) - { - const ISC_STATUS* v = ex.value(); - if (// Update conflict error - v[0] == isc_arg_gds && - v[1] == isc_update_conflict && - // Read committed transaction with snapshots - (req_transaction->tra_flags & TRA_read_committed) && - (req_transaction->tra_flags & TRA_read_consistency) && - // Snapshot has been assigned to the request - - // it was top-level request - !TRA_get_prior_request(tdbb)) - { - if (++numTries < 10) - { - fb_utils::init_status(tdbb->tdbb_status_vector); - continue; - } + if (!(req_request->req_flags & req_update_conflict)) + break; + req_request->req_flags &= ~req_update_conflict; + if (numTries >= 10) { + gds__log("Update conflict: unable to get a stable set of rows in the source tables"); + ERR_post(Arg::Gds(isc_deadlock) << + Arg::Gds(isc_update_conflict) << + Arg::Gds(isc_concurrent_transaction) << Arg::Num(req_request->req_conflict_txn)); } - throw; + req_transaction->rollbackSavepoint(tdbb, true); + req_transaction->startSavepoint(tdbb); + numTries++; } + savePoint.release(); // everything is ok } trace.finish(have_cursor, ITracePlugin::RESULT_SUCCESS); diff --git a/src/jrd/Savepoint.cpp b/src/jrd/Savepoint.cpp index 3cd31a5f51..edd53479fa 100644 --- a/src/jrd/Savepoint.cpp +++ b/src/jrd/Savepoint.cpp @@ -234,7 +234,7 @@ void VerbAction::mergeTo(thread_db* tdbb, jrd_tra* transaction, VerbAction* next release(transaction); } -void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction) +void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks) { // Undo changes recorded for this verb action. // After that, clear the verb action and prepare it for later reuse. @@ -258,7 +258,7 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction) if (!DPM_get(tdbb, &rpb, LCK_read)) BUGCHECK(186); // msg 186 record disappeared - if (have_undo && !(rpb.rpb_flags & rpb_deleted)) + if ((have_undo || preserveLocks) && !(rpb.rpb_flags & rpb_deleted)) VIO_data(tdbb, &rpb, transaction->tra_pool); else CCH_RELEASE(tdbb, &rpb.getWindow(tdbb)); @@ -267,7 +267,45 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction) BUGCHECK(185); // msg 185 wrong record version if (!have_undo) - VIO_backout(tdbb, &rpb, transaction); + { + if (preserveLocks) { + // Fetch previous record version and update in place current version with it + record_param temp = rpb; + temp.rpb_page = rpb.rpb_b_page; + temp.rpb_line = rpb.rpb_b_line; + temp.rpb_record = NULL; + + if (temp.rpb_flags & rpb_delta) + fb_assert(temp.rpb_prior != NULL); + else + fb_assert(temp.rpb_prior == NULL); + + if (!DPM_fetch(tdbb, &temp, LCK_read)) + BUGCHECK(291); // msg 291 cannot find record back version + + if (!(temp.rpb_flags & rpb_chained) || (temp.rpb_flags & (rpb_blob | rpb_fragment))) + ERR_bugcheck_msg("invalid back version"); + + VIO_data(tdbb, &temp, tdbb->getDefaultPool()); + + Record* const save_record = rpb.rpb_record; + if (rpb.rpb_flags & rpb_deleted) + rpb.rpb_record = NULL; + Record* const dead_record = rpb.rpb_record; + + VIO_update_in_place(tdbb, transaction, &rpb, &temp); + + if (dead_record) + { + rpb.rpb_record = NULL; // VIO_garbage_collect_idx will play with this record dirty tricks + VIO_garbage_collect_idx(tdbb, transaction, &rpb, dead_record); + } + rpb.rpb_record = save_record; + + delete temp.rpb_record; + } else + VIO_backout(tdbb, &rpb, transaction); + } else { AutoUndoRecord record(vct_undo->current().setupRecord(transaction)); @@ -378,7 +416,7 @@ void Savepoint::cleanupTempData() } } -Savepoint* Savepoint::rollback(thread_db* tdbb, Savepoint* prior) +Savepoint* Savepoint::rollback(thread_db* tdbb, Savepoint* prior, bool preserveLocks) { // Undo changes made in this savepoint. // Perform index and BLOB cleanup if needed. @@ -399,7 +437,7 @@ Savepoint* Savepoint::rollback(thread_db* tdbb, Savepoint* prior) { VerbAction* const action = m_actions; - action->undo(tdbb, m_transaction); + action->undo(tdbb, m_transaction, preserveLocks); m_actions = action->vct_next; action->vct_next = m_freeActions; diff --git a/src/jrd/Savepoint.h b/src/jrd/Savepoint.h index b70632701c..d6ff330258 100644 --- a/src/jrd/Savepoint.h +++ b/src/jrd/Savepoint.h @@ -94,7 +94,7 @@ namespace Jrd UndoItemTree* vct_undo; // Data for undo records void mergeTo(thread_db* tdbb, jrd_tra* transaction, VerbAction* nextAction); - void undo(thread_db* tdbb, jrd_tra* transaction); + void undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks); void garbageCollectIdxLite(thread_db* tdbb, jrd_tra* transaction, SINT64 recordNumber, VerbAction* nextAction, Record* goingRecord); @@ -231,7 +231,7 @@ namespace Jrd void cleanupTempData(); - Savepoint* rollback(thread_db* tdbb, Savepoint* prior = NULL); + Savepoint* rollback(thread_db* tdbb, Savepoint* prior = NULL, bool preserveLocks = false); Savepoint* rollforward(thread_db* tdbb, Savepoint* prior = NULL); static void destroy(Savepoint*& savepoint) diff --git a/src/jrd/req.h b/src/jrd/req.h index d88ca0b593..2403116118 100644 --- a/src/jrd/req.h +++ b/src/jrd/req.h @@ -275,6 +275,7 @@ public: SINT64 req_fetch_rowcount; // Total number of rows returned by this request jrd_req* req_proc_caller; // Procedure's caller request const ValueListNode* req_proc_inputs; // and its node with input parameters + TraNumber req_conflict_txn; // Transaction number for update conflict in read consistency mode ULONG req_src_line; ULONG req_src_column; @@ -397,6 +398,7 @@ const ULONG req_continue_loop = 0x100L; // PSQL continue statement const ULONG req_proc_fetch = 0x200L; // Fetch from procedure in progress const ULONG req_same_tx_upd = 0x400L; // record was updated by same transaction const ULONG req_reserved = 0x800L; // Request reserved for client +const ULONG req_update_conflict = 0x1000L; // We need to restart request due to update conflict // Index lock block diff --git a/src/jrd/tra.cpp b/src/jrd/tra.cpp index 34e8ac8415..c2a142d344 100644 --- a/src/jrd/tra.cpp +++ b/src/jrd/tra.cpp @@ -1616,8 +1616,11 @@ int TRA_snapshot_state(thread_db* tdbb, const jrd_tra* trans, TraNumber number, // GC thread accesses data directly without any request if (jrd_req* current_request = tdbb->getRequest()) { - // There is no request snapshot when we build expression index - if (jrd_req* snapshot_request = current_request->req_snapshot.m_owner) + // Notes: + // 1) There is no request snapshot when we build expression index + // 2) Disable read committed snapshot after we encountered update conflict + jrd_req* snapshot_request = current_request->req_snapshot.m_owner; + if (snapshot_request && !(snapshot_request->req_flags & req_update_conflict)) { if (stateCn > snapshot_request->req_snapshot.m_number) return tra_active; @@ -3847,7 +3850,7 @@ Savepoint* jrd_tra::startSavepoint(bool root) return savepoint; } -void jrd_tra::rollbackSavepoint(thread_db* tdbb) +void jrd_tra::rollbackSavepoint(thread_db* tdbb, bool preserveLocks) /************************************** * * r o l l b a c k S a v e p o i n t @@ -3864,7 +3867,7 @@ void jrd_tra::rollbackSavepoint(thread_db* tdbb) REPL_save_cleanup(tdbb, this, tra_save_point, true); Jrd::ContextPoolHolder context(tdbb, tra_pool); - tra_save_point = tra_save_point->rollback(tdbb); + tra_save_point = tra_save_point->rollback(tdbb, NULL, preserveLocks); } } diff --git a/src/jrd/tra.h b/src/jrd/tra.h index b12841a56a..03f424acdc 100644 --- a/src/jrd/tra.h +++ b/src/jrd/tra.h @@ -386,7 +386,7 @@ public: Record* findNextUndo(VerbAction* before_this, jrd_rel* relation, SINT64 number); void listStayingUndo(jrd_rel* relation, SINT64 number, RecordStack &staying); Savepoint* startSavepoint(bool root = false); - void rollbackSavepoint(thread_db* tdbb); + void rollbackSavepoint(thread_db* tdbb, bool preserveLocks = false); void rollbackToSavepoint(thread_db* tdbb, SavNumber number); void rollforwardSavepoint(thread_db* tdbb); DbCreatorsList* getDbCreatorsList(); diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index aa628a91f5..a8099b131a 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -1441,7 +1441,7 @@ void VIO_data(thread_db* tdbb, record_param* rpb, MemoryPool* pool) } -void VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) +bool VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) { /************************************** * @@ -1497,7 +1497,7 @@ void VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) // hvlad: what if record was created\modified by user tx also, // i.e. if there is backversion ??? VIO_backout(tdbb, rpb, transaction); - return; + return true; } transaction->tra_flags |= TRA_write; @@ -1891,7 +1891,7 @@ void VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) if (transaction->tra_save_point && transaction->tra_save_point->isChanging()) verb_post(tdbb, transaction, rpb, rpb->rpb_undo); - return; + return true; } const bool backVersion = (rpb->rpb_b_page != 0); @@ -1910,12 +1910,20 @@ void VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) { // Update stub didn't find one page -- do a long, hard update PageStack stack; - if (prepare_update(tdbb, transaction, tid_fetch, rpb, &temp, 0, stack, false)) + int prepare_result = prepare_update(tdbb, transaction, tid_fetch, rpb, &temp, 0, stack, false); + if (prepare_result && + (!(transaction->tra_flags & TRA_read_consistency) || prepare_result == PREPARE_LOCKERR)) { ERR_post(Arg::Gds(isc_deadlock) << Arg::Gds(isc_update_conflict) << Arg::Gds(isc_concurrent_transaction) << Arg::Num(rpb->rpb_transaction_nr)); } + if (prepare_result) { + jrd_req* top_request = request->req_snapshot.m_owner; + top_request->req_flags |= req_update_conflict; + top_request->req_conflict_txn = rpb->rpb_transaction_nr; + return false; + } // Old record was restored and re-fetched for write. Now replace it. @@ -1974,6 +1982,7 @@ void VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) { notify_garbage_collector(tdbb, rpb, transaction->tra_number); } + return true; } @@ -2769,7 +2778,7 @@ void VIO_init(thread_db* tdbb) } } -void VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, jrd_tra* transaction) +bool VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, jrd_tra* transaction) { /************************************** * @@ -2835,7 +2844,7 @@ void VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, j { VIO_update_in_place(tdbb, transaction, org_rpb, new_rpb); tdbb->bumpRelStats(RuntimeStatistics::RECORD_UPDATES, relation->rel_id); - return; + return true; } check_gbak_cheating_insupd(tdbb, relation, "UPDATE"); @@ -3198,19 +3207,27 @@ void VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, j } tdbb->bumpRelStats(RuntimeStatistics::RECORD_UPDATES, relation->rel_id); - return; + return true; } const bool backVersion = (org_rpb->rpb_b_page != 0); record_param temp; PageStack stack; - if (prepare_update(tdbb, transaction, org_rpb->rpb_transaction_nr, org_rpb, &temp, new_rpb, - stack, false)) + int prepare_result = prepare_update(tdbb, transaction, org_rpb->rpb_transaction_nr, org_rpb, + &temp, new_rpb, stack, false); + if (prepare_result && + (!(transaction->tra_flags & TRA_read_consistency) || prepare_result == PREPARE_LOCKERR)) { ERR_post(Arg::Gds(isc_deadlock) << Arg::Gds(isc_update_conflict) << Arg::Gds(isc_concurrent_transaction) << Arg::Num(org_rpb->rpb_transaction_nr)); } + if (prepare_result) { + jrd_req* top_request = tdbb->getRequest()->req_snapshot.m_owner; + top_request->req_flags |= req_update_conflict; + top_request->req_conflict_txn = org_rpb->rpb_transaction_nr; + return false; + } IDX_modify_flag_uk_modified(tdbb, org_rpb, new_rpb, transaction); @@ -3259,6 +3276,7 @@ void VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, j { notify_garbage_collector(tdbb, org_rpb, transaction->tra_number); } + return true; } @@ -5662,7 +5680,7 @@ static int prepare_update( thread_db* tdbb, delete_record(tdbb, temp, 0, NULL); - if (writelock) + if (writelock || (transaction->tra_flags & TRA_read_consistency)) { tdbb->bumpRelStats(RuntimeStatistics::RECORD_CONFLICTS, relation->rel_id); return PREPARE_DELETE; @@ -5785,15 +5803,15 @@ static int prepare_update( thread_db* tdbb, switch (state) { case tra_committed: - // We need to loop waiting in read committed with no read consistency transactions only - if (!(transaction->tra_flags & TRA_read_committed) || - (transaction->tra_flags & TRA_read_consistency)) + // For SNAPSHOT mode transactions raise error early + if (!(transaction->tra_flags & TRA_read_committed)) { tdbb->bumpRelStats(RuntimeStatistics::RECORD_CONFLICTS, relation->rel_id); ERR_post(Arg::Gds(isc_update_conflict) << Arg::Gds(isc_concurrent_transaction) << Arg::Num(update_conflict_trans)); } + return PREPARE_CONFLICT; case tra_limbo: if (!(transaction->tra_flags & TRA_ignore_limbo)) diff --git a/src/jrd/vio_proto.h b/src/jrd/vio_proto.h index a833c18799..e37eaa229f 100644 --- a/src/jrd/vio_proto.h +++ b/src/jrd/vio_proto.h @@ -42,7 +42,7 @@ bool VIO_chase_record_version(Jrd::thread_db*, Jrd::record_param*, Jrd::jrd_tra*, MemoryPool*, bool, bool); void VIO_copy_record(Jrd::thread_db*, Jrd::record_param*, Jrd::record_param*); void VIO_data(Jrd::thread_db*, Jrd::record_param*, MemoryPool*); -void VIO_erase(Jrd::thread_db*, Jrd::record_param*, Jrd::jrd_tra*); +bool VIO_erase(Jrd::thread_db*, Jrd::record_param*, Jrd::jrd_tra*); void VIO_fini(Jrd::thread_db*); bool VIO_garbage_collect(Jrd::thread_db*, Jrd::record_param*, Jrd::jrd_tra*); Jrd::Record* VIO_gc_record(Jrd::thread_db*, Jrd::jrd_rel*); @@ -51,7 +51,7 @@ bool VIO_get_current(Jrd::thread_db*, Jrd::record_param*, Jrd::jrd_tra*, MemoryPool*, bool, bool&); void VIO_init(Jrd::thread_db*); bool VIO_writelock(Jrd::thread_db*, Jrd::record_param*, Jrd::jrd_tra*); -void VIO_modify(Jrd::thread_db*, Jrd::record_param*, Jrd::record_param*, Jrd::jrd_tra*); +bool VIO_modify(Jrd::thread_db*, Jrd::record_param*, Jrd::record_param*, Jrd::jrd_tra*); bool VIO_next_record(Jrd::thread_db*, Jrd::record_param*, Jrd::jrd_tra*, MemoryPool*, bool); Jrd::Record* VIO_record(Jrd::thread_db*, Jrd::record_param*, const Jrd::Format*, MemoryPool*); bool VIO_refetch_record(Jrd::thread_db*, Jrd::record_param*, Jrd::jrd_tra*, bool, bool); From 230e6df4b12be1bef87798efb6165b189c6081bd Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Mon, 12 Aug 2019 10:56:17 +0300 Subject: [PATCH 04/24] Go back to using isc_deadlock error code - for compatibility with existing software --- src/jrd/vio.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index a8099b131a..5aa6b83b0b 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -5808,7 +5808,8 @@ static int prepare_update( thread_db* tdbb, { tdbb->bumpRelStats(RuntimeStatistics::RECORD_CONFLICTS, relation->rel_id); - ERR_post(Arg::Gds(isc_update_conflict) << + ERR_post(Arg::Gds(isc_deadlock) << + Arg::Gds(isc_update_conflict) << Arg::Gds(isc_concurrent_transaction) << Arg::Num(update_conflict_trans)); } return PREPARE_CONFLICT; From a521efbc3a392bdb876b37e0a539fe008b3e9296 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Mon, 12 Aug 2019 11:01:34 +0300 Subject: [PATCH 05/24] Use DSQL error code --- src/dsql/dsql.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/dsql/dsql.cpp b/src/dsql/dsql.cpp index 504caca528..5e6741c1a0 100644 --- a/src/dsql/dsql.cpp +++ b/src/dsql/dsql.cpp @@ -862,9 +862,10 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, req_request->req_flags &= ~req_update_conflict; if (numTries >= 10) { gds__log("Update conflict: unable to get a stable set of rows in the source tables"); - ERR_post(Arg::Gds(isc_deadlock) << - Arg::Gds(isc_update_conflict) << - Arg::Gds(isc_concurrent_transaction) << Arg::Num(req_request->req_conflict_txn)); + ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-913) << + Arg::Gds(isc_deadlock) << + Arg::Gds(isc_update_conflict) << + Arg::Gds(isc_concurrent_transaction) << Arg::Num(req_request->req_conflict_txn)); } req_transaction->rollbackSavepoint(tdbb, true); req_transaction->startSavepoint(tdbb); From 64822593fc0ff9d37e6c23899d37e15a3009c3c9 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Mon, 12 Aug 2019 12:51:02 +0300 Subject: [PATCH 06/24] Fix error found during TPC-C test --- src/jrd/Savepoint.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/jrd/Savepoint.cpp b/src/jrd/Savepoint.cpp index edd53479fa..02aecc1b88 100644 --- a/src/jrd/Savepoint.cpp +++ b/src/jrd/Savepoint.cpp @@ -268,7 +268,7 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks) if (!have_undo) { - if (preserveLocks) { + if (preserveLocks && rpb.rpb_b_page) { // Fetch previous record version and update in place current version with it record_param temp = rpb; temp.rpb_page = rpb.rpb_b_page; From a17e42f706f963f51a3eaeeeaf4b0b4c09366da6 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Thu, 15 Aug 2019 17:08:24 +0300 Subject: [PATCH 07/24] Fix restarts of "WITH LOCK" requests --- src/jrd/vio.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index 5aa6b83b0b..e6fef4a78d 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -4080,6 +4080,11 @@ bool VIO_writelock(thread_db* tdbb, record_param* org_rpb, jrd_tra* transaction) { case PREPARE_CONFLICT: case PREPARE_DELETE: + if ((transaction->tra_flags & TRA_read_consistency)) { + jrd_req* top_request = tdbb->getRequest()->req_snapshot.m_owner; + top_request->req_flags |= req_update_conflict; + top_request->req_conflict_txn = org_rpb->rpb_transaction_nr; + } org_rpb->rpb_runtime_flags |= RPB_refetch; return false; case PREPARE_LOCKERR: From c5cffe5cd5c07012a7bb305ec9ca262594cbd542 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Thu, 15 Aug 2019 17:10:52 +0300 Subject: [PATCH 08/24] Fix broken handling of user savepoints --- src/dsql/dsql.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/dsql/dsql.cpp b/src/dsql/dsql.cpp index 5e6741c1a0..362ebba4ea 100644 --- a/src/dsql/dsql.cpp +++ b/src/dsql/dsql.cpp @@ -851,6 +851,8 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, setupTimer(tdbb); thread_db::TimerGuard timerGuard(tdbb, req_timer, !have_cursor); + if (req_transaction && (req_transaction->tra_flags & TRA_read_consistency) && + statement->getType() != DsqlCompiledStatement::TYPE_SAVEPOINT) { AutoSavePoint savePoint(tdbb, req_transaction); int numTries = 0; @@ -872,6 +874,8 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, numTries++; } savePoint.release(); // everything is ok + } else { + doExecute(tdbb, traHandle, inMetadata, inMsg, outMetadata, outMsg, singleton); } trace.finish(have_cursor, ITracePlugin::RESULT_SUCCESS); From ac16bb032a8a59f795eed2440261bcac76e7ac7e Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Thu, 15 Aug 2019 18:36:16 +0300 Subject: [PATCH 09/24] Add comments --- src/dsql/StmtNodes.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index d62bb1ce00..6174fec591 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -2643,8 +2643,16 @@ const StmtNode* EraseNode::erase(thread_db* tdbb, jrd_req* request, WhichTrigger VirtualTable::erase(tdbb, rpb); else if (!relation->rel_view_rse) { + // VIO_erase returns false if there is an update conflict in Read Consistency + // transaction. Before returning false it disables statement-level snapshot + // (via setting req_update_conflict flag) so re-fetch should see new data. + // Deleting new version blindly is generally unsafe, but is ok in this situation + // because all changes made by this request will certainly be undone and request + // will be restarted. while (!VIO_erase(tdbb, rpb, transaction)) { + // VIO_refetch_record returns false if record has been deleted by someone else. + // Gently ignore this situation and proceed further. if (!VIO_refetch_record(tdbb, rpb, transaction, true, true)) return parentStmt; } @@ -6456,8 +6464,16 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg VirtualTable::modify(tdbb, orgRpb, newRpb); else if (!relation->rel_view_rse) { + // VIO_modify returns false if there is an update conflict in Read Consistency + // transaction. Before returning false it disables statement-level snapshot + // (via setting req_update_conflict flag) so re-fetch should see new data. + // Updating new version blindly is generally unsafe, but is ok in this situation + // because all changes made by this request will certainly be undone and request + // will be restarted. while (!VIO_modify(tdbb, orgRpb, newRpb, transaction)) { + // VIO_refetch_record returns false if record has been deleted by someone else. + // Gently ignore this situation and proceed further. if (!VIO_refetch_record(tdbb, orgRpb, transaction, true, true)) return parentStmt; } From accb0e9f8a5caa28b0bfc3e9e40af2438ce0043d Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Fri, 16 Aug 2019 12:11:53 +0300 Subject: [PATCH 10/24] Use snapshot mode for PSQL blocks executed in autonomous transactions --- .../README.autonomous_transactions.txt | 2 +- src/dsql/StmtNodes.cpp | 18 +----------------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/doc/sql.extensions/README.autonomous_transactions.txt b/doc/sql.extensions/README.autonomous_transactions.txt index 53be08aecd..5cbb3d3dc1 100644 --- a/doc/sql.extensions/README.autonomous_transactions.txt +++ b/doc/sql.extensions/README.autonomous_transactions.txt @@ -10,7 +10,7 @@ need to raise an exception but do not want the database changes to be rolled-bac If exceptions are raised inside the body of an autonomous transaction block, the changes are rolled-back. If the block runs till the end, the transaction is committed. -The new transaction is initiated with the same isolation level of the existing one. +The new transaction is initiated with snapshot isolation level and lock timeout of the existing one. Should be used with caution to avoid deadlocks. Author: diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index 6174fec591..26f25e497f 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -3964,7 +3964,7 @@ const StmtNode* InAutonomousTransactionNode::execute(thread_db* tdbb, jrd_req* r jrd_tra* const org_transaction = request->req_transaction; fb_assert(tdbb->getTransaction() == org_transaction); - jrd_tra* const transaction = TRA_start(tdbb, org_transaction->tra_flags, + jrd_tra* const transaction = TRA_start(tdbb, 0, org_transaction->tra_lock_timeout, org_transaction); @@ -3989,12 +3989,6 @@ const StmtNode* InAutonomousTransactionNode::execute(thread_db* tdbb, jrd_req* r const Savepoint* const savepoint = transaction->startSavepoint(); impure->savNumber = savepoint->getNumber(); - if ((transaction->tra_flags & TRA_read_committed) && - (transaction->tra_flags & TRA_read_consistency)) - { - TRA_setup_request_snapshot(tdbb, request, true); - } - return action; } @@ -4006,16 +4000,6 @@ const StmtNode* InAutonomousTransactionNode::execute(thread_db* tdbb, jrd_req* r fb_assert(transaction->tra_number == impure->traNumber); - if (request->req_operation == jrd_req::req_return || - request->req_operation == jrd_req::req_unwind) - { - if ((transaction->tra_flags & TRA_read_committed) && - (transaction->tra_flags & TRA_read_consistency)) - { - TRA_release_request_snapshot(tdbb, request); - } - } - switch (request->req_operation) { case jrd_req::req_return: From 89cb61b9efe7e43e4a80d9526a0c60a10f61537e Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Fri, 16 Aug 2019 13:19:05 +0300 Subject: [PATCH 11/24] Remove function parameter that is not used anymore --- src/jrd/tra.cpp | 4 ++-- src/jrd/tra_proto.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/jrd/tra.cpp b/src/jrd/tra.cpp index c2a142d344..aebab96c64 100644 --- a/src/jrd/tra.cpp +++ b/src/jrd/tra.cpp @@ -145,7 +145,7 @@ jrd_req* TRA_get_prior_request(thread_db* tdbb) return org_request; } -void TRA_setup_request_snapshot(Jrd::thread_db* tdbb, Jrd::jrd_req* request, bool no_prior) +void TRA_setup_request_snapshot(Jrd::thread_db* tdbb, Jrd::jrd_req* request) { // This function is called whenever request is started in a transaction. // Setup context to preserve read consistency in READ COMMITTED transactions. @@ -160,7 +160,7 @@ void TRA_setup_request_snapshot(Jrd::thread_db* tdbb, Jrd::jrd_req* request, boo return; // See if there is any request right above us in the call stack - jrd_req* org_request = no_prior ? nullptr : TRA_get_prior_request(tdbb); + jrd_req* org_request = TRA_get_prior_request(tdbb); if (org_request && org_request->req_transaction == transaction) { diff --git a/src/jrd/tra_proto.h b/src/jrd/tra_proto.h index 1ac8f9bf73..f424db449d 100644 --- a/src/jrd/tra_proto.h +++ b/src/jrd/tra_proto.h @@ -63,7 +63,7 @@ void TRA_update_counters(Jrd::thread_db*, Jrd::Database*); int TRA_wait(Jrd::thread_db* tdbb, Jrd::jrd_tra* trans, TraNumber number, Jrd::jrd_tra::wait_t wait); void TRA_attach_request(Jrd::jrd_tra* transaction, Jrd::jrd_req* request); void TRA_detach_request(Jrd::jrd_req* request); -void TRA_setup_request_snapshot(Jrd::thread_db*, Jrd::jrd_req* request, bool no_prior = false); +void TRA_setup_request_snapshot(Jrd::thread_db*, Jrd::jrd_req* request); void TRA_release_request_snapshot(Jrd::thread_db*, Jrd::jrd_req* request); Jrd::jrd_req* TRA_get_prior_request(Jrd::thread_db*); From f7a77ac90f08610e7a18485dce899cdcc272ffbd Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Fri, 16 Aug 2019 15:05:09 +0300 Subject: [PATCH 12/24] Simulate legacy behavior of autonomous transactions in PSQL when ReadConsistency is disabled --- src/dsql/StmtNodes.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index 26f25e497f..7b2e7a476a 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -3964,7 +3964,14 @@ const StmtNode* InAutonomousTransactionNode::execute(thread_db* tdbb, jrd_req* r jrd_tra* const org_transaction = request->req_transaction; fb_assert(tdbb->getTransaction() == org_transaction); - jrd_tra* const transaction = TRA_start(tdbb, 0, + // Use SNAPSHOT isolation mode in autonomous transactions by default + ULONG transaction_flags = 0; + + // Simulate legacy behavior if ReadConsistency is disabled + if (!dbb->dbb_config->getReadConsistency()) + transaction_flags = org_transaction->tra_flags; + + jrd_tra* const transaction = TRA_start(tdbb, transaction_flags, org_transaction->tra_lock_timeout, org_transaction); From 0206dd99a975ff635ab283af8e9e466eb946f464 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Mon, 19 Aug 2019 10:41:02 +0300 Subject: [PATCH 13/24] Use SNAPSHOT transaction mode in autonomous transaction blocks when ReadConsistency is disabled, but isc_tpb_read_consistency was passed by ther user --- src/dsql/StmtNodes.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index 7b2e7a476a..d5deb32ef6 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -3967,9 +3967,12 @@ const StmtNode* InAutonomousTransactionNode::execute(thread_db* tdbb, jrd_req* r // Use SNAPSHOT isolation mode in autonomous transactions by default ULONG transaction_flags = 0; - // Simulate legacy behavior if ReadConsistency is disabled - if (!dbb->dbb_config->getReadConsistency()) + // Simulate legacy behavior if Read Consistency is not used + if (!dbb->dbb_config->getReadConsistency() && + !(org_transaction->tra_flags & TRA_read_consistency)) + { transaction_flags = org_transaction->tra_flags; + } jrd_tra* const transaction = TRA_start(tdbb, transaction_flags, org_transaction->tra_lock_timeout, From 18e6c2fc12dff667abef221f1f928ab1ea905b2e Mon Sep 17 00:00:00 2001 From: hvlad Date: Fri, 30 Aug 2019 15:03:11 +0300 Subject: [PATCH 14/24] Style --- src/dsql/dsql.cpp | 6 ++++-- src/jrd/Savepoint.cpp | 3 ++- src/jrd/vio.cpp | 9 ++++++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/dsql/dsql.cpp b/src/dsql/dsql.cpp index 362ebba4ea..61ad594c8a 100644 --- a/src/dsql/dsql.cpp +++ b/src/dsql/dsql.cpp @@ -792,7 +792,8 @@ void DsqlDmlRequest::doExecute(thread_db* tdbb, jrd_tra** traHandle, else { // Prefetch first row of a query - if (reqTypeWithCursor(statement->getType())) { + if (reqTypeWithCursor(statement->getType())) + { dsql_msg* message = (dsql_msg*) statement->getReceiveMsg(); UCHAR* dsqlMsgBuffer = req_msg_buffers[message->msg_buffer_number]; @@ -862,7 +863,8 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, if (!(req_request->req_flags & req_update_conflict)) break; req_request->req_flags &= ~req_update_conflict; - if (numTries >= 10) { + if (numTries >= 10) + { gds__log("Update conflict: unable to get a stable set of rows in the source tables"); ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-913) << Arg::Gds(isc_deadlock) << diff --git a/src/jrd/Savepoint.cpp b/src/jrd/Savepoint.cpp index 02aecc1b88..1bb90eb997 100644 --- a/src/jrd/Savepoint.cpp +++ b/src/jrd/Savepoint.cpp @@ -268,7 +268,8 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks) if (!have_undo) { - if (preserveLocks && rpb.rpb_b_page) { + if (preserveLocks && rpb.rpb_b_page) + { // Fetch previous record version and update in place current version with it record_param temp = rpb; temp.rpb_page = rpb.rpb_b_page; diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index e6fef4a78d..dc562f5590 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -1918,7 +1918,8 @@ bool VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) Arg::Gds(isc_update_conflict) << Arg::Gds(isc_concurrent_transaction) << Arg::Num(rpb->rpb_transaction_nr)); } - if (prepare_result) { + if (prepare_result) + { jrd_req* top_request = request->req_snapshot.m_owner; top_request->req_flags |= req_update_conflict; top_request->req_conflict_txn = rpb->rpb_transaction_nr; @@ -3222,7 +3223,8 @@ bool VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, j Arg::Gds(isc_update_conflict) << Arg::Gds(isc_concurrent_transaction) << Arg::Num(org_rpb->rpb_transaction_nr)); } - if (prepare_result) { + if (prepare_result) + { jrd_req* top_request = tdbb->getRequest()->req_snapshot.m_owner; top_request->req_flags |= req_update_conflict; top_request->req_conflict_txn = org_rpb->rpb_transaction_nr; @@ -4080,7 +4082,8 @@ bool VIO_writelock(thread_db* tdbb, record_param* org_rpb, jrd_tra* transaction) { case PREPARE_CONFLICT: case PREPARE_DELETE: - if ((transaction->tra_flags & TRA_read_consistency)) { + if ((transaction->tra_flags & TRA_read_consistency)) + { jrd_req* top_request = tdbb->getRequest()->req_snapshot.m_owner; top_request->req_flags |= req_update_conflict; top_request->req_conflict_txn = org_rpb->rpb_transaction_nr; From d9ce719ef7af469f249dafb180cc1a0185a14ff4 Mon Sep 17 00:00:00 2001 From: "nikolay.samofatov" Date: Thu, 31 Oct 2019 17:21:09 +0300 Subject: [PATCH 15/24] Fix conflict error during refetch found by Vlad --- src/dsql/StmtNodes.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index d5deb32ef6..3a4d16a165 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -2655,6 +2655,8 @@ const StmtNode* EraseNode::erase(thread_db* tdbb, jrd_req* request, WhichTrigger // Gently ignore this situation and proceed further. if (!VIO_refetch_record(tdbb, rpb, transaction, true, true)) return parentStmt; + + rpb->rpb_runtime_flags &= ~RPB_refetch; } REPL_erase(tdbb, rpb, transaction); } @@ -6470,6 +6472,8 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg // Gently ignore this situation and proceed further. if (!VIO_refetch_record(tdbb, orgRpb, transaction, true, true)) return parentStmt; + + orgRpb->rpb_runtime_flags &= ~RPB_refetch; } IDX_modify(tdbb, orgRpb, newRpb, transaction); REPL_modify(tdbb, orgRpb, newRpb, transaction); From 447974bd84662c1818343bffd7c37fc79181dde5 Mon Sep 17 00:00:00 2001 From: hvlad Date: Thu, 13 Feb 2020 01:47:14 +0200 Subject: [PATCH 16/24] Reworked request restart logic --- .../README.autonomous_transactions.txt | 4 +- src/dsql/BlrDebugWriter.cpp | 49 ++-- src/dsql/BlrDebugWriter.h | 4 + src/dsql/Nodes.h | 6 + src/dsql/StmtNodes.cpp | 263 +++++++++++++----- src/dsql/StmtNodes.h | 26 +- src/dsql/dsql.cpp | 50 +++- src/include/firebird/impl/consts_pub.h | 1 + src/jrd/DebugInterface.cpp | 26 +- src/jrd/DebugInterface.h | 14 +- src/jrd/RecordSourceNodes.cpp | 2 + src/jrd/Savepoint.cpp | 17 +- src/jrd/Savepoint.h | 2 +- src/jrd/exe.cpp | 2 +- src/jrd/req.h | 2 + src/jrd/tra.cpp | 3 + src/jrd/tra.h | 1 + src/jrd/vio.cpp | 93 +++++-- 18 files changed, 426 insertions(+), 139 deletions(-) diff --git a/doc/sql.extensions/README.autonomous_transactions.txt b/doc/sql.extensions/README.autonomous_transactions.txt index 5cbb3d3dc1..872db3f2db 100644 --- a/doc/sql.extensions/README.autonomous_transactions.txt +++ b/doc/sql.extensions/README.autonomous_transactions.txt @@ -10,7 +10,9 @@ need to raise an exception but do not want the database changes to be rolled-bac If exceptions are raised inside the body of an autonomous transaction block, the changes are rolled-back. If the block runs till the end, the transaction is committed. -The new transaction is initiated with snapshot isolation level and lock timeout of the existing one. +The new transaction is initiated with the same isolation level and lock timeout of the existing one. +The only exception is that if existing transaction run in Read Committed Read Consistency mode, then +new autonomous transaction will run in Concurrency mode. Should be used with caution to avoid deadlocks. Author: diff --git a/src/dsql/BlrDebugWriter.cpp b/src/dsql/BlrDebugWriter.cpp index 4757e16cd1..f20040fb92 100644 --- a/src/dsql/BlrDebugWriter.cpp +++ b/src/dsql/BlrDebugWriter.cpp @@ -58,21 +58,9 @@ void BlrDebugWriter::putDebugSrcInfo(ULONG line, ULONG col) { debugData.add(fb_dbg_map_src2blr); - debugData.add(line); - debugData.add(line >> 8); - debugData.add(line >> 16); - debugData.add(line >> 24); - - debugData.add(col); - debugData.add(col >> 8); - debugData.add(col >> 16); - debugData.add(col >> 24); - - const ULONG offset = (getBlrData().getCount() - getBaseOffset()); - debugData.add(offset); - debugData.add(offset >> 8); - debugData.add(offset >> 16); - debugData.add(offset >> 24); + putValue(line); + putValue(col); + putBlrOffset(); } void BlrDebugWriter::putDebugVariable(USHORT number, const MetaName& name) @@ -132,10 +120,7 @@ void BlrDebugWriter::putDebugSubFunction(DeclareSubFuncNode* subFuncNode) HalfStaticArray& subDebugData = subFuncNode->blockScratch->debugData; const ULONG count = ULONG(subDebugData.getCount()); - debugData.add(UCHAR(count)); - debugData.add(UCHAR(count >> 8)); - debugData.add(UCHAR(count >> 16)); - debugData.add(UCHAR(count >> 24)); + putValue(count); debugData.add(subDebugData.begin(), count); } @@ -152,11 +137,29 @@ void BlrDebugWriter::putDebugSubProcedure(DeclareSubProcNode* subProcNode) HalfStaticArray& subDebugData = subProcNode->blockScratch->debugData; const ULONG count = ULONG(subDebugData.getCount()); - debugData.add(UCHAR(count)); - debugData.add(UCHAR(count >> 8)); - debugData.add(UCHAR(count >> 16)); - debugData.add(UCHAR(count >> 24)); + putValue(count); debugData.add(subDebugData.begin(), count); } +void BlrDebugWriter::putDebugMarkers(ULONG marks) +{ + debugData.add(fb_dbg_map_markers); + putValue(marks); + putBlrOffset(); +} + +void BlrDebugWriter::putValue(ULONG val) +{ + debugData.add(val); + debugData.add(val >> 8); + debugData.add(val >> 16); + debugData.add(val >> 24); +} + +void BlrDebugWriter::putBlrOffset() +{ + const ULONG offset = (getBlrData().getCount() - getBaseOffset()); + putValue(offset); +} + } // namespace Jrd diff --git a/src/dsql/BlrDebugWriter.h b/src/dsql/BlrDebugWriter.h index 22ff42c67d..41980861ba 100644 --- a/src/dsql/BlrDebugWriter.h +++ b/src/dsql/BlrDebugWriter.h @@ -50,12 +50,16 @@ public: void putDebugCursor(USHORT, const Firebird::MetaName&); void putDebugSubFunction(DeclareSubFuncNode* subFuncNode); void putDebugSubProcedure(DeclareSubProcNode* subProcNode); + void putDebugMarkers(ULONG marks); DebugData& getDebugData() { return debugData; } virtual void raiseError(const Firebird::Arg::StatusVector& vector); private: + void putValue(ULONG val); + void putBlrOffset(); + DebugData debugData; }; diff --git a/src/dsql/Nodes.h b/src/dsql/Nodes.h index 255953724b..22185f31cf 100644 --- a/src/dsql/Nodes.h +++ b/src/dsql/Nodes.h @@ -1400,6 +1400,12 @@ public: POST_TRIG = 2 }; + // Marks used by EraseNode, ModifyNode and StoreNode + static const unsigned MARK_POSITIONED = 0x01; // Erase|Modify node is positioned at explicit cursor + static const unsigned MARK_MERGE = 0x02; // node is part of MERGE statement + // Marks used by ForNode + static const unsigned MARK_FOR_UPDATE = 0x04; // implicit cursor used in UPDATE\DELETE\MERGE statement + struct ExeState { ExeState(thread_db* tdbb, jrd_req* request, jrd_tra* transaction) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index 3a4d16a165..fcd01e0310 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -93,6 +93,7 @@ static RelationSourceNode* pass1Update(thread_db* tdbb, CompilerScratch* csb, jr const TrigVector* trigger, StreamType stream, StreamType updateStream, SecurityClass::flags_t priv, jrd_rel* view, StreamType viewStream, StreamType viewUpdateStream); static void pass1Validations(thread_db* tdbb, CompilerScratch* csb, Array& validations); +static ForNode* pass2FindForNode(StmtNode* node, StreamType stream); static void postTriggerAccess(CompilerScratch* csb, jrd_rel* ownerRelation, ExternalAccess::exa_act operation, jrd_rel* view); static void preModifyEraseTriggers(thread_db* tdbb, TrigVector** trigs, @@ -658,9 +659,12 @@ const StmtNode* BlockNode::execute(thread_db* tdbb, jrd_req* request, ExeState* return parentStmt; } + // Skip PSQL exception handlers when request restart is in progress + const bool skipHandlers = (transaction->tra_flags & TRA_ex_restart); + const StmtNode* temp = parentStmt; - if (handlers && handlers->statements.hasData()) + if (handlers && handlers->statements.hasData() && !skipHandlers) { // First of all rollback failed work if (!(transaction->tra_flags & TRA_system)) @@ -2267,6 +2271,7 @@ static RegisterNode regEraseNode(blr_erase); DmlNode* EraseNode::parse(thread_db* /*tdbb*/, MemoryPool& pool, CompilerScratch* csb, const UCHAR /*blrOp*/) { + const ULONG blrOffset = csb->csb_blr_reader.getOffset(); const USHORT n = csb->csb_blr_reader.getByte(); if (n >= csb->csb_rpt.getCount() || !(csb->csb_rpt[n].csb_flags & csb_used)) @@ -2275,6 +2280,10 @@ DmlNode* EraseNode::parse(thread_db* /*tdbb*/, MemoryPool& pool, CompilerScratch EraseNode* node = FB_NEW_POOL(pool) EraseNode(pool); node->stream = csb->csb_rpt[n].csb_stream; + ULONG marks; + if (csb->csb_dbg_info && csb->csb_dbg_info->blrToMarks.get(blrOffset, marks)) + node->marks |= marks; + return node; } @@ -2289,6 +2298,7 @@ StmtNode* EraseNode::dsqlPass(DsqlCompilerScratch* dsqlScratch) if (dsqlCursorName.hasData() && dsqlScratch->isPsql()) { node->dsqlContext = dsqlPassCursorContext(dsqlScratch, dsqlCursorName, relation); + node->marks |= StmtNode::MARK_POSITIONED; // Process old context values. dsqlScratch->context->push(node->dsqlContext); @@ -2312,7 +2322,10 @@ StmtNode* EraseNode::dsqlPass(DsqlCompilerScratch* dsqlScratch) RseNode* rse; if (dsqlCursorName.hasData()) + { rse = dsqlPassCursorReference(dsqlScratch, dsqlCursorName, relation); + node->marks |= StmtNode::MARK_POSITIONED; + } else { rse = FB_NEW_POOL(dsqlScratch->getPool()) RseNode(dsqlScratch->getPool()); @@ -2366,6 +2379,7 @@ string EraseNode::internalPrint(NodePrinter& printer) const NODE_PRINT(printer, statement); NODE_PRINT(printer, subStatement); NODE_PRINT(printer, stream); + NODE_PRINT(printer, marks); return "EraseNode"; } @@ -2376,42 +2390,24 @@ void EraseNode::genBlr(DsqlCompilerScratch* dsqlScratch) const dsql_ctx* context; if (dsqlContext) - { context = dsqlContext; - - if (statement) - { - dsqlScratch->appendUChar(blr_begin); - statement->genBlr(dsqlScratch); - dsqlScratch->appendUChar(blr_erase); - GEN_stuff_context(dsqlScratch, context); - dsqlScratch->appendUChar(blr_end); - } - else - { - dsqlScratch->appendUChar(blr_erase); - GEN_stuff_context(dsqlScratch, context); - } - } else - { context = dsqlRelation->dsqlContext; - if (statement) - { - dsqlScratch->appendUChar(blr_begin); - statement->genBlr(dsqlScratch); - dsqlScratch->appendUChar(blr_erase); - GEN_stuff_context(dsqlScratch, context); - dsqlScratch->appendUChar(blr_end); - } - else - { - dsqlScratch->appendUChar(blr_erase); - GEN_stuff_context(dsqlScratch, context); - } + if (statement) + { + dsqlScratch->appendUChar(blr_begin); + statement->genBlr(dsqlScratch); } + dsqlScratch->appendUChar(blr_erase); + if (marks) + dsqlScratch->putDebugMarkers(marks); + GEN_stuff_context(dsqlScratch, context); + + if (statement) + dsqlScratch->appendUChar(blr_end); + if (message) dsqlScratch->appendUChar(blr_end); } @@ -2531,6 +2527,9 @@ EraseNode* EraseNode::pass2(thread_db* tdbb, CompilerScratch* csb) doPass2(tdbb, csb, statement.getAddress(), this); doPass2(tdbb, csb, subStatement.getAddress(), this); + if (!(marks & StmtNode::MARK_POSITIONED)) + forNode = pass2FindForNode(parentStmt, stream); + impureOffset = CMP_impure(csb, sizeof(SLONG)); csb->csb_rpt[stream].csb_flags |= csb_update; @@ -2543,6 +2542,7 @@ const StmtNode* EraseNode::execute(thread_db* tdbb, jrd_req* request, ExeState* if (request->req_operation == jrd_req::req_unwind) retNode = parentStmt; + else if (request->req_operation == jrd_req::req_return && subStatement) { if (!exeState->topNode) @@ -2620,6 +2620,12 @@ const StmtNode* EraseNode::erase(thread_db* tdbb, jrd_req* request, WhichTrigger request->req_operation = jrd_req::req_return; RLCK_reserve_relation(tdbb, transaction, relation, true); + if (forNode && forNode->isWriteLockMode(request)) + { + VIO_writelock(tdbb, rpb, transaction); + return parentStmt; + } + // If the stream was sorted, the various fields in the rpb are probably junk. // Just to make sure that everything is cool, refetch and release the record. @@ -2649,14 +2655,23 @@ const StmtNode* EraseNode::erase(thread_db* tdbb, jrd_req* request, WhichTrigger // Deleting new version blindly is generally unsafe, but is ok in this situation // because all changes made by this request will certainly be undone and request // will be restarted. - while (!VIO_erase(tdbb, rpb, transaction)) - { - // VIO_refetch_record returns false if record has been deleted by someone else. - // Gently ignore this situation and proceed further. - if (!VIO_refetch_record(tdbb, rpb, transaction, true, true)) - return parentStmt; - rpb->rpb_runtime_flags &= ~RPB_refetch; + if (forNode) + rpb->rpb_runtime_flags |= RPB_restart_ready; + else + rpb->rpb_runtime_flags &= ~RPB_restart_ready; + + if (!VIO_erase(tdbb, rpb, transaction)) + { + fb_assert(forNode != nullptr); + + forNode->setWriteLockMode(request); + + // VIO_writelock returns false if record has been deleted by someone else. + // Gently ignore this situation and proceed further. + rpb->rpb_runtime_flags |= RPB_refetch; + VIO_writelock(tdbb, rpb, transaction); + return parentStmt; } REPL_erase(tdbb, rpb, transaction); } @@ -3966,15 +3981,12 @@ const StmtNode* InAutonomousTransactionNode::execute(thread_db* tdbb, jrd_req* r jrd_tra* const org_transaction = request->req_transaction; fb_assert(tdbb->getTransaction() == org_transaction); - // Use SNAPSHOT isolation mode in autonomous transactions by default - ULONG transaction_flags = 0; - // Simulate legacy behavior if Read Consistency is not used - if (!dbb->dbb_config->getReadConsistency() && - !(org_transaction->tra_flags & TRA_read_consistency)) - { - transaction_flags = org_transaction->tra_flags; - } + ULONG transaction_flags = org_transaction->tra_flags; + + // Replace Read Consistency by Concurrecy isolation mode + if (transaction_flags & TRA_read_consistency) + transaction_flags &= ~(TRA_read_committed | TRA_read_consistency); jrd_tra* const transaction = TRA_start(tdbb, transaction_flags, org_transaction->tra_lock_timeout, @@ -4315,8 +4327,6 @@ void ExecBlockNode::genBlr(DsqlCompilerScratch* dsqlScratch) { thread_db* tdbb = JRD_get_thread_data(); - dsqlScratch->beginDebug(); - // Sub routine needs a different approach from EXECUTE BLOCK. // EXECUTE BLOCK needs "ports", which creates DSQL messages using the client charset. // Sub routine doesn't need ports and should generate BLR as declared in its metadata. @@ -4451,8 +4461,6 @@ void ExecBlockNode::genBlr(DsqlCompilerScratch* dsqlScratch) dsqlScratch->appendUChar(blr_end); dsqlScratch->genReturn(true); dsqlScratch->appendUChar(blr_end); - - dsqlScratch->endDebug(); } // Revert parameters order for EXECUTE BLOCK statement @@ -4804,6 +4812,11 @@ DmlNode* ForNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* csb, { ForNode* node = FB_NEW_POOL(pool) ForNode(pool); + const ULONG blrOffset = csb->csb_blr_reader.getOffset(); + ULONG marks; + if (csb->csb_dbg_info && csb->csb_dbg_info->blrToMarks.get(blrOffset, marks)) + node->forUpdate = (marks & StmtNode::MARK_FOR_UPDATE) != 0; + if (csb->csb_blr_reader.peekByte() == (UCHAR) blr_stall) node->stall = PAR_parse_stmt(tdbb, csb); @@ -4898,6 +4911,8 @@ string ForNode::internalPrint(NodePrinter& printer) const NODE_PRINT(printer, statement); NODE_PRINT(printer, cursor); NODE_PRINT(printer, parBlrBeginCnt); + NODE_PRINT(printer, forUpdate); + NODE_PRINT(printer, withLock); return "ForNode"; } @@ -4916,6 +4931,9 @@ void ForNode::genBlr(DsqlCompilerScratch* dsqlScratch) dsqlScratch->appendUChar(blr_for); + if (forUpdate) + dsqlScratch->putDebugMarkers(StmtNode::MARK_FOR_UPDATE); + if (!statement || dsqlForceSingular) dsqlScratch->appendUChar(blr_singular); @@ -4981,7 +4999,10 @@ StmtNode* ForNode::pass2(thread_db* tdbb, CompilerScratch* csb) // as implicit cursors are always positioned in a valid record, and the name is // only used to raise isc_cursor_not_positioned. - impureOffset = CMP_impure(csb, sizeof(SavNumber)); + if (rse->flags & RseNode::FLAG_WRITELOCK) + withLock = true; + + impureOffset = CMP_impure(csb, sizeof(Impure)); return this; } @@ -4989,17 +5010,21 @@ StmtNode* ForNode::pass2(thread_db* tdbb, CompilerScratch* csb) const StmtNode* ForNode::execute(thread_db* tdbb, jrd_req* request, ExeState* /*exeState*/) const { jrd_tra* transaction = request->req_transaction; + Impure* impure = request->getImpure(impureOffset); switch (request->req_operation) { case jrd_req::req_evaluate: - *request->getImpure(impureOffset) = 0; + // initialize impure values + impure->savepoint = 0; + impure->writeLockMode = false; + if (!(transaction->tra_flags & TRA_system) && transaction->tra_save_point && transaction->tra_save_point->hasChanges()) { const Savepoint* const savepoint = transaction->startSavepoint(); - *request->getImpure(impureOffset) = savepoint->getNumber(); + impure->savepoint = savepoint->getNumber(); } cursor->open(tdbb); request->req_records_affected.clear(); @@ -5011,11 +5036,42 @@ const StmtNode* ForNode::execute(thread_db* tdbb, jrd_req* request, ExeState* /* // fall into case jrd_req::req_sync: - if (cursor->fetchNext(tdbb)) { - request->req_operation = jrd_req::req_evaluate; - return statement; + const bool fetched = cursor->fetchNext(tdbb); + if (withLock) + { + const jrd_req* top_request = request->req_snapshot.m_owner; + if ((top_request) && (top_request->req_flags & req_update_conflict)) + impure->writeLockMode = true; + } + + if (fetched) + { + if (impure->writeLockMode && withLock) + { + // Skip statement execution and fetch (and try to lock) next record. + request->req_operation = jrd_req::req_sync; + return this; + } + + request->req_operation = jrd_req::req_evaluate; + return statement; + } } + + if (impure->writeLockMode) + { + const jrd_req* top_request = request->req_snapshot.m_owner; + fb_assert(top_request); + fb_assert(top_request->req_flags & req_update_conflict); + + transaction->tra_flags |= TRA_ex_restart; + + ERR_post(Arg::Gds(isc_deadlock) << + Arg::Gds(isc_update_conflict) << + Arg::Gds(isc_concurrent_transaction) << Arg::Num(top_request->req_conflict_txn)); + } + request->req_operation = jrd_req::req_return; // fall into @@ -5036,7 +5092,7 @@ const StmtNode* ForNode::execute(thread_db* tdbb, jrd_req* request, ExeState* /* default: { - const SavNumber savNumber = *request->getImpure(impureOffset); + const SavNumber savNumber = impure->savepoint; if (savNumber) { @@ -5060,6 +5116,21 @@ const StmtNode* ForNode::execute(thread_db* tdbb, jrd_req* request, ExeState* /* } +bool ForNode::isWriteLockMode(jrd_req* request) const +{ + const Impure* impure = request->getImpure(impureOffset); + return impure->writeLockMode; +} + + +void ForNode::setWriteLockMode(jrd_req* request) const +{ + Impure* impure = request->getImpure(impureOffset); + fb_assert(!impure->writeLockMode); + + impure->writeLockMode = true; +} + //-------------------- @@ -5451,6 +5522,8 @@ StmtNode* MergeNode::dsqlPass(DsqlCompilerScratch* dsqlScratch) if (returning) forNode->dsqlForceSingular = true; + forNode->forUpdate = true; + // Get the already processed relations. RseNode* processedRse = nodeAs(forNode->rse->dsqlStreams->items[0]); source = processedRse->dsqlStreams->items[0]; @@ -5488,6 +5561,7 @@ StmtNode* MergeNode::dsqlPass(DsqlCompilerScratch* dsqlScratch) // Build the MODIFY node. ModifyNode* modify = FB_NEW_POOL(pool) ModifyNode(pool); + modify->marks |= StmtNode::MARK_MERGE; thisIf->trueAction = modify; dsql_ctx* const oldContext = dsqlGetContext(target); @@ -5576,6 +5650,7 @@ StmtNode* MergeNode::dsqlPass(DsqlCompilerScratch* dsqlScratch) { // Build the DELETE node. EraseNode* erase = FB_NEW_POOL(pool) EraseNode(pool); + erase->marks |= StmtNode::MARK_MERGE; thisIf->trueAction = erase; dsql_ctx* context = dsqlGetContext(target); @@ -5640,6 +5715,7 @@ StmtNode* MergeNode::dsqlPass(DsqlCompilerScratch* dsqlScratch) // Build the INSERT node. StoreNode* store = FB_NEW_POOL(pool) StoreNode(pool); + // TODO: store->marks |= StmtNode::MARK_MERGE; store->dsqlRelation = relation; store->dsqlFields = notMatched->fields; store->dsqlValues = notMatched->values; @@ -5904,6 +5980,7 @@ static RegisterNode regModifyNode2(blr_modify2); DmlNode* ModifyNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* csb, const UCHAR blrOp) { // Parse the original and new contexts. + const ULONG blrOffset = csb->csb_blr_reader.getOffset(); USHORT context = (unsigned int) csb->csb_blr_reader.getByte(); @@ -5940,6 +6017,10 @@ DmlNode* ModifyNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* c if (blrOp == blr_modify2) node->statement2 = PAR_parse_stmt(tdbb, csb); + ULONG marks; + if (csb->csb_dbg_info && csb->csb_dbg_info->blrToMarks.get(blrOffset, marks)) + node->marks |= marks; + return node; } @@ -5974,6 +6055,7 @@ StmtNode* ModifyNode::internalDsqlPass(DsqlCompilerScratch* dsqlScratch, bool up if (dsqlCursorName.hasData() && dsqlScratch->isPsql()) { node->dsqlContext = dsqlPassCursorContext(dsqlScratch, dsqlCursorName, relation); + node->marks |= StmtNode::MARK_POSITIONED; // Process old context values. dsqlScratch->context->push(node->dsqlContext); @@ -6054,6 +6136,7 @@ StmtNode* ModifyNode::internalDsqlPass(DsqlCompilerScratch* dsqlScratch, bool up { rse = dsqlPassCursorReference(dsqlScratch, dsqlCursorName, relation); old_context = rse->dsqlStreams->items[0]->dsqlContext; + node->marks |= StmtNode::MARK_POSITIONED; } else { @@ -6154,6 +6237,7 @@ string ModifyNode::internalPrint(NodePrinter& printer) const NODE_PRINT(printer, mapView); NODE_PRINT(printer, orgStream); NODE_PRINT(printer, newStream); + NODE_PRINT(printer, marks); return "ModifyNode"; } @@ -6165,6 +6249,8 @@ void ModifyNode::genBlr(DsqlCompilerScratch* dsqlScratch) const dsql_msg* message = dsqlGenDmlHeader(dsqlScratch, rse); dsqlScratch->appendUChar(statement2 ? blr_modify2 : blr_modify); + if (marks) + dsqlScratch->putDebugMarkers(marks); const dsql_ctx* context; @@ -6358,6 +6444,9 @@ ModifyNode* ModifyNode::pass2(thread_db* tdbb, CompilerScratch* csb) SBM_SET(tdbb->getDefaultPool(), &csb->csb_rpt[orgStream].csb_fields, id); } + if (!(marks & StmtNode::MARK_POSITIONED)) + forNode = pass2FindForNode(parentStmt, orgStream); + impureOffset = CMP_impure(csb, sizeof(impure_state)); return this; @@ -6427,7 +6516,12 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg { case jrd_req::req_evaluate: request->req_records_affected.bumpModified(false); - break; + + if (impure->sta_state == 0 && forNode && forNode->isWriteLockMode(request)) + request->req_operation = jrd_req::req_return; + // fall thru + else + break; case jrd_req::req_return: if (impure->sta_state == 1) @@ -6442,6 +6536,12 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg if (impure->sta_state == 0) { + if (forNode && forNode->isWriteLockMode(request)) + { + VIO_writelock(tdbb, orgRpb, transaction); + return parentStmt; + } + // CVC: This call made here to clear the record in each NULL field and // varchar field whose tail may contain garbage. cleanupRpb(tdbb, newRpb); @@ -6461,19 +6561,26 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg else if (!relation->rel_view_rse) { // VIO_modify returns false if there is an update conflict in Read Consistency - // transaction. Before returning false it disables statement-level snapshot + // transaction and our code is ready to work in write lock mode (see flag set below). + // Before returning false it disables statement-level snapshot // (via setting req_update_conflict flag) so re-fetch should see new data. - // Updating new version blindly is generally unsafe, but is ok in this situation - // because all changes made by this request will certainly be undone and request - // will be restarted. - while (!VIO_modify(tdbb, orgRpb, newRpb, transaction)) - { - // VIO_refetch_record returns false if record has been deleted by someone else. - // Gently ignore this situation and proceed further. - if (!VIO_refetch_record(tdbb, orgRpb, transaction, true, true)) - return parentStmt; - orgRpb->rpb_runtime_flags &= ~RPB_refetch; + if (forNode) + orgRpb->rpb_runtime_flags |= RPB_restart_ready; + else + orgRpb->rpb_runtime_flags &= ~RPB_restart_ready; + + if (!VIO_modify(tdbb, orgRpb, newRpb, transaction)) + { + fb_assert(forNode != nullptr); + + forNode->setWriteLockMode(request); + + // VIO_writelock returns false if record has been deleted by someone else. + // Gently ignore this situation and proceed further. + orgRpb->rpb_runtime_flags |= RPB_refetch; + VIO_writelock(tdbb, orgRpb, transaction); + return parentStmt; } IDX_modify(tdbb, orgRpb, newRpb, transaction); REPL_modify(tdbb, orgRpb, newRpb, transaction); @@ -8812,6 +8919,7 @@ static const dsql_msg* dsqlGenDmlHeader(DsqlCompilerScratch* dsqlScratch, RseNod if (dsqlRse) { dsqlScratch->appendUChar(blr_for); + dsqlScratch->putDebugMarkers(StmtNode::MARK_FOR_UPDATE); GEN_expr(dsqlScratch, dsqlRse); } @@ -9696,6 +9804,23 @@ static void pass1Validations(thread_db* tdbb, CompilerScratch* csb, Array(node)) + node = node->parentStmt; + + ForNode* forNode = nodeAs(node); + if (forNode && forNode->rse->containsStream(stream)) + { + //fb_assert(forNode->forUpdate == true); + if (forNode->forUpdate) + return forNode; + } + + return nullptr; +}; + // Inherit access to triggers to be fired. // // When we detect that a trigger could be fired by a request, diff --git a/src/dsql/StmtNodes.h b/src/dsql/StmtNodes.h index cdf6ad91f4..d530d6f316 100644 --- a/src/dsql/StmtNodes.h +++ b/src/dsql/StmtNodes.h @@ -35,6 +35,7 @@ namespace Jrd { class CompoundStmtNode; class ExecBlockNode; +class ForNode; class PlanNode; class RelationSourceNode; class SelectNode; @@ -564,7 +565,8 @@ public: dsqlContext(NULL), statement(NULL), subStatement(NULL), - stream(0) + stream(0), + marks(0) { } @@ -595,6 +597,8 @@ public: NestConst statement; NestConst subStatement; StreamType stream; + NestConst forNode; // parent implicit cursor, if present + unsigned marks; // see StmtNode::IUD_MARK_xxx }; @@ -913,7 +917,9 @@ public: rse(NULL), statement(NULL), cursor(NULL), - parBlrBeginCnt(0) + parBlrBeginCnt(0), + forUpdate(false), + withLock(false) { } @@ -927,7 +933,16 @@ public: virtual StmtNode* pass2(thread_db* tdbb, CompilerScratch* csb); virtual const StmtNode* execute(thread_db* tdbb, jrd_req* request, ExeState* exeState) const; + bool isWriteLockMode(jrd_req* request) const; + void setWriteLockMode(jrd_req* request) const; + public: + struct Impure + { + SavNumber savepoint; + bool writeLockMode; // true - driven statement (UPDATE\DELETE\SELECT WITH LOCK) works in "write lock" mode, false - normal mode + }; + NestConst dsqlSelect; NestConst dsqlInto; DeclareCursorNode* dsqlCursor; @@ -939,6 +954,8 @@ public: NestConst statement; NestConst cursor; int parBlrBeginCnt; + bool forUpdate; // part of UPDATE\DELETE\MERGE statement + bool withLock; // part of SELECT ... WITH LOCK statement }; @@ -1153,7 +1170,8 @@ public: validations(pool), mapView(NULL), orgStream(0), - newStream(0) + newStream(0), + marks(0) { } @@ -1190,6 +1208,8 @@ public: NestConst mapView; StreamType orgStream; StreamType newStream; + NestConst forNode; // parent implicit cursor, if present + unsigned marks; // see StmtNode::IUD_MARK_xxx }; diff --git a/src/dsql/dsql.cpp b/src/dsql/dsql.cpp index 61ad594c8a..cefe64ae1b 100644 --- a/src/dsql/dsql.cpp +++ b/src/dsql/dsql.cpp @@ -603,8 +603,12 @@ void DsqlDmlRequest::dsqlPass(thread_db* tdbb, DsqlCompilerScratch* scratch, boo else scratch->getStatement()->setBlrVersion(4); + scratch->beginDebug(); + GEN_request(scratch, node); + scratch->endDebug(); + // Create the messages buffers for (FB_SIZE_T i = 0; i < scratch->ports.getCount(); ++i) { @@ -856,13 +860,47 @@ void DsqlDmlRequest::execute(thread_db* tdbb, jrd_tra** traHandle, statement->getType() != DsqlCompiledStatement::TYPE_SAVEPOINT) { AutoSavePoint savePoint(tdbb, req_transaction); + req_request->req_flags &= ~req_update_conflict; int numTries = 0; while (true) { - doExecute(tdbb, traHandle, inMetadata, inMsg, outMetadata, outMsg, singleton); + AutoSetRestoreFlag restartReady(&req_request->req_flags, req_restart_ready, true); + try + { + doExecute(tdbb, traHandle, inMetadata, inMsg, outMetadata, outMsg, singleton); + } + catch (const status_exception&) + { + if (!(req_transaction->tra_flags & TRA_ex_restart)) + { + req_request->req_flags &= ~req_update_conflict; + throw; + } + } + if (!(req_request->req_flags & req_update_conflict)) + { + fb_assert((req_transaction->tra_flags & TRA_ex_restart) == 0); + req_transaction->tra_flags &= ~TRA_ex_restart; + +#ifdef DEV_BUILD + if (numTries > 0) + { + string s; + s.printf("restarts = %d", numTries); + + ERRD_post_warning(Arg::Warning(isc_random) << Arg::Str(s)); + } +#endif break; + } + + fb_assert((req_transaction->tra_flags & TRA_ex_restart) != 0); + req_request->req_flags &= ~req_update_conflict; + req_transaction->tra_flags &= ~TRA_ex_restart; + fb_utils::init_status(tdbb->tdbb_status_vector); + if (numTries >= 10) { gds__log("Update conflict: unable to get a stable set of rows in the source tables"); @@ -2076,7 +2114,15 @@ static void sql_info(thread_db* tdbb, { const bool detailed = (item == isc_info_sql_explain_plan); string plan = OPT_get_plan(tdbb, request->req_request, detailed); - +#ifdef DEV_BUILD + if (!detailed) + { + NodePrinter printer; + request->req_request->getStatement()->topNode->print(printer); + plan += "\n--------\n"; + plan += printer.getText(); + } +#endif if (plan.hasData()) { // 1-byte item + 2-byte length + isc_info_end/isc_info_truncated == 4 diff --git a/src/include/firebird/impl/consts_pub.h b/src/include/firebird/impl/consts_pub.h index 6252c49775..b341dd49e5 100644 --- a/src/include/firebird/impl/consts_pub.h +++ b/src/include/firebird/impl/consts_pub.h @@ -732,6 +732,7 @@ #define fb_dbg_subproc 5 #define fb_dbg_subfunc 6 #define fb_dbg_map_curname 7 +#define fb_dbg_map_markers 8 // sub code for fb_dbg_map_argument #define fb_dbg_arg_input 0 diff --git a/src/jrd/DebugInterface.cpp b/src/jrd/DebugInterface.cpp index 14a6e50193..d0e528e8b1 100644 --- a/src/jrd/DebugInterface.cpp +++ b/src/jrd/DebugInterface.cpp @@ -10,10 +10,10 @@ * See the License for the specific language governing rights * and limitations under the License. * - * The Original Code was created by Vlad Horsun + * The Original Code was created by Vlad Khorsun * for the Firebird Open Source RDBMS project. * - * Copyright (c) 2006 Vlad Horsun + * Copyright (c) 2006 Vlad Khorsun * and all contributors signed below. * * All Rights Reserved. @@ -225,6 +225,28 @@ void DBG_parse_debug_info(ULONG length, const UCHAR* data, DbgInfo& dbgInfo) break; } + case fb_dbg_map_markers: + { + if (data + 8 >= end) + { + bad_format = true; + break; + } + + ULONG marks = *data++; + marks |= *data++ << 8; + marks |= *data++ << 16; + marks |= *data++ << 24; + + ULONG offset = *data++; + offset |= *data++ << 8; + offset |= *data++ << 16; + offset |= *data++ << 24; + + dbgInfo.blrToMarks.put(offset, marks); + } + break; + case fb_dbg_end: if (data != end) bad_format = true; diff --git a/src/jrd/DebugInterface.h b/src/jrd/DebugInterface.h index f0295c4c56..b7f3836e5c 100644 --- a/src/jrd/DebugInterface.h +++ b/src/jrd/DebugInterface.h @@ -10,10 +10,10 @@ * See the License for the specific language governing rights * and limitations under the License. * - * The Original Code was created by Vlad Horsun + * The Original Code was created by Vlad Khorsun * for the Firebird Open Source RDBMS project. * - * Copyright (c) 2006 Vlad Horsun + * Copyright (c) 2006 Vlad Khorsun * and all contributors signed below. * * All Rights Reserved. @@ -33,7 +33,8 @@ // Also, it introduces some new tags. const UCHAR DBG_INFO_VERSION_1 = UCHAR(1); const UCHAR DBG_INFO_VERSION_2 = UCHAR(2); -const UCHAR CURRENT_DBG_INFO_VERSION = DBG_INFO_VERSION_2; +const UCHAR DBG_INFO_VERSION_3 = UCHAR(3); // blr offsets of "update" cursors and driven sub-statements +const UCHAR CURRENT_DBG_INFO_VERSION = DBG_INFO_VERSION_3; namespace Firebird { @@ -55,6 +56,7 @@ typedef Firebird::SortedArray< MapBlrToSrcItem> MapBlrToSrc; typedef GenericMap > > MapVarIndexToName; +typedef GenericMap > > MapBlrToMarks; struct ArgumentInfo { @@ -93,7 +95,8 @@ struct DbgInfo : public PermanentStorage argInfoToName(p), curIndexToName(p), subFuncs(p), - subProcs(p) + subProcs(p), + blrToMarks(p) { } @@ -126,6 +129,8 @@ struct DbgInfo : public PermanentStorage subProcs.clear(); } + + blrToMarks.clear(); } MapBlrToSrc blrToSrc; // mapping between blr offsets and source text position @@ -134,6 +139,7 @@ struct DbgInfo : public PermanentStorage MapVarIndexToName curIndexToName; // mapping between cursor index and name GenericMap > > subFuncs; // sub functions GenericMap > > subProcs; // sub procedures + MapBlrToMarks blrToMarks; // blr offsets of marked verbs }; } // namespace Firebird diff --git a/src/jrd/RecordSourceNodes.cpp b/src/jrd/RecordSourceNodes.cpp index de981c8c38..b245799eef 100644 --- a/src/jrd/RecordSourceNodes.cpp +++ b/src/jrd/RecordSourceNodes.cpp @@ -550,6 +550,8 @@ string RelationSourceNode::internalPrint(NodePrinter& printer) const NODE_PRINT(printer, dsqlName); NODE_PRINT(printer, alias); NODE_PRINT(printer, context); + if (relation) + printer.print("rel_name", relation->rel_name); return "RelationSourceNode"; } diff --git a/src/jrd/Savepoint.cpp b/src/jrd/Savepoint.cpp index 1bb90eb997..41de5ab799 100644 --- a/src/jrd/Savepoint.cpp +++ b/src/jrd/Savepoint.cpp @@ -234,7 +234,7 @@ void VerbAction::mergeTo(thread_db* tdbb, jrd_tra* transaction, VerbAction* next release(transaction); } -void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks) +void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, VerbAction* preserveAction) { // Undo changes recorded for this verb action. // After that, clear the verb action and prepare it for later reuse. @@ -258,7 +258,7 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks) if (!DPM_get(tdbb, &rpb, LCK_read)) BUGCHECK(186); // msg 186 record disappeared - if ((have_undo || preserveLocks) && !(rpb.rpb_flags & rpb_deleted)) + if ((have_undo || preserveAction) && !(rpb.rpb_flags & rpb_deleted)) VIO_data(tdbb, &rpb, transaction->tra_pool); else CCH_RELEASE(tdbb, &rpb.getWindow(tdbb)); @@ -268,7 +268,7 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks) if (!have_undo) { - if (preserveLocks && rpb.rpb_b_page) + if (preserveAction && rpb.rpb_b_page) { // Fetch previous record version and update in place current version with it record_param temp = rpb; @@ -304,7 +304,10 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks) rpb.rpb_record = save_record; delete temp.rpb_record; - } else + + RBM_SET(transaction->tra_pool, &preserveAction->vct_records, rpb.rpb_number.getValue()); + } + else VIO_backout(tdbb, &rpb, transaction); } else @@ -437,8 +440,12 @@ Savepoint* Savepoint::rollback(thread_db* tdbb, Savepoint* prior, bool preserveL while (m_actions) { VerbAction* const action = m_actions; + VerbAction* preserveAction = nullptr; - action->undo(tdbb, m_transaction, preserveLocks); + if (preserveLocks && m_next) + preserveAction = m_next->getAction(action->vct_relation); + + action->undo(tdbb, m_transaction, preserveAction); m_actions = action->vct_next; action->vct_next = m_freeActions; diff --git a/src/jrd/Savepoint.h b/src/jrd/Savepoint.h index d6ff330258..e264ff2ba9 100644 --- a/src/jrd/Savepoint.h +++ b/src/jrd/Savepoint.h @@ -94,7 +94,7 @@ namespace Jrd UndoItemTree* vct_undo; // Data for undo records void mergeTo(thread_db* tdbb, jrd_tra* transaction, VerbAction* nextAction); - void undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks); + void undo(thread_db* tdbb, jrd_tra* transaction, VerbAction* preserveAction); void garbageCollectIdxLite(thread_db* tdbb, jrd_tra* transaction, SINT64 recordNumber, VerbAction* nextAction, Record* goingRecord); diff --git a/src/jrd/exe.cpp b/src/jrd/exe.cpp index 8f899e24b5..3013b3bc12 100644 --- a/src/jrd/exe.cpp +++ b/src/jrd/exe.cpp @@ -875,7 +875,7 @@ void EXE_start(thread_db* tdbb, jrd_req* request, jrd_tra* transaction) TRA_post_resources(tdbb, transaction, statement->resources); TRA_attach_request(transaction, request); - request->req_flags &= req_in_use; + request->req_flags &= req_in_use | req_restart_ready; request->req_flags |= req_active; request->req_flags &= ~req_reserved; diff --git a/src/jrd/req.h b/src/jrd/req.h index 2403116118..3884c7261f 100644 --- a/src/jrd/req.h +++ b/src/jrd/req.h @@ -132,6 +132,7 @@ const USHORT RPB_refetch = 0x01; // re-fetch is required const USHORT RPB_undo_data = 0x02; // data got from undo log const USHORT RPB_undo_read = 0x04; // read was performed using the undo log const USHORT RPB_undo_deleted = 0x08; // read was performed using the undo log, primary version is deleted +const USHORT RPB_restart_ready = 0x10; // update conflict could be handled by statement restart const USHORT RPB_UNDO_FLAGS = (RPB_undo_data | RPB_undo_read | RPB_undo_deleted); @@ -399,6 +400,7 @@ const ULONG req_proc_fetch = 0x200L; // Fetch from procedure in progress const ULONG req_same_tx_upd = 0x400L; // record was updated by same transaction const ULONG req_reserved = 0x800L; // Request reserved for client const ULONG req_update_conflict = 0x1000L; // We need to restart request due to update conflict +const ULONG req_restart_ready = 0x2000L; // Request is ready to restat in case of update conflict // Index lock block diff --git a/src/jrd/tra.cpp b/src/jrd/tra.cpp index aebab96c64..52cd44d9bb 100644 --- a/src/jrd/tra.cpp +++ b/src/jrd/tra.cpp @@ -3866,6 +3866,9 @@ void jrd_tra::rollbackSavepoint(thread_db* tdbb, bool preserveLocks) { REPL_save_cleanup(tdbb, this, tra_save_point, true); + if (tra_flags & TRA_ex_restart) + preserveLocks = true; + Jrd::ContextPoolHolder context(tdbb, tra_pool); tra_save_point = tra_save_point->rollback(tdbb, NULL, preserveLocks); } diff --git a/src/jrd/tra.h b/src/jrd/tra.h index 03f424acdc..fe7af3778f 100644 --- a/src/jrd/tra.h +++ b/src/jrd/tra.h @@ -425,6 +425,7 @@ const ULONG TRA_no_auto_undo = 0x8000L; // don't start a savepoint in TRA_start const ULONG TRA_precommitted = 0x10000L; // transaction committed at startup const ULONG TRA_own_interface = 0x20000L; // tra_interface was created for internal needs const ULONG TRA_read_consistency = 0x40000L; // ensure read consistency in this transaction +const ULONG TRA_ex_restart = 0x80000L; // Exception was raised to restart request // flags derived from TPB, see also transaction_options() at tra.cpp const ULONG TRA_OPTIONS_MASK = (TRA_degree3 | TRA_readonly | TRA_ignore_limbo | TRA_read_committed | diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index dc562f5590..e1283736e2 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -1441,6 +1441,57 @@ void VIO_data(thread_db* tdbb, record_param* rpb, MemoryPool* pool) } +static bool check_prepare_result(int prepare_result, jrd_tra* transaction, jrd_req* request, record_param* rpb) +{ +/************************************** + * + * c h e c k _ p r e p a r e _ r e s u l t + * + ************************************** + * + * Functional description + * Called by VIO_modify and VIO_erase. Raise update conflict error if not in + * read consistency transaction or lock error happens or if request is already + * in update conflict mode. In latter case set TRA_ex_restart flag to correctly + * handle request restart. + * + **************************************/ + if (prepare_result == PREPARE_OK) + return true; + + jrd_req* top_request = request->req_snapshot.m_owner; + + const bool restart_ready = top_request && + (top_request->req_flags & req_restart_ready) && + (rpb->rpb_runtime_flags & RPB_restart_ready); + + // Second update conflict when request is already in update conflict mode + // means we have some (indirect) UPDATE\DELETE in WHERE clause of primary + // cursor. In this case all we can do is restart whole request immediately. + const bool secondary = top_request && + (top_request->req_flags & req_update_conflict) && + (prepare_result != PREPARE_LOCKERR); + + if (!(transaction->tra_flags & TRA_read_consistency) || prepare_result == PREPARE_LOCKERR || + secondary || !restart_ready) + { + if (secondary) + transaction->tra_flags |= TRA_ex_restart; + + ERR_post(Arg::Gds(isc_deadlock) << + Arg::Gds(isc_update_conflict) << + Arg::Gds(isc_concurrent_transaction) << Arg::Num(rpb->rpb_transaction_nr)); + } + + if (top_request) + { + top_request->req_flags |= req_update_conflict; + top_request->req_conflict_txn = rpb->rpb_transaction_nr; + } + return false; +} + + bool VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) { /************************************** @@ -1911,20 +1962,8 @@ bool VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction) // Update stub didn't find one page -- do a long, hard update PageStack stack; int prepare_result = prepare_update(tdbb, transaction, tid_fetch, rpb, &temp, 0, stack, false); - if (prepare_result && - (!(transaction->tra_flags & TRA_read_consistency) || prepare_result == PREPARE_LOCKERR)) - { - ERR_post(Arg::Gds(isc_deadlock) << - Arg::Gds(isc_update_conflict) << - Arg::Gds(isc_concurrent_transaction) << Arg::Num(rpb->rpb_transaction_nr)); - } - if (prepare_result) - { - jrd_req* top_request = request->req_snapshot.m_owner; - top_request->req_flags |= req_update_conflict; - top_request->req_conflict_txn = rpb->rpb_transaction_nr; + if (!check_prepare_result(prepare_result, transaction, request, rpb)) return false; - } // Old record was restored and re-fetched for write. Now replace it. @@ -3216,20 +3255,8 @@ bool VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, j PageStack stack; int prepare_result = prepare_update(tdbb, transaction, org_rpb->rpb_transaction_nr, org_rpb, &temp, new_rpb, stack, false); - if (prepare_result && - (!(transaction->tra_flags & TRA_read_consistency) || prepare_result == PREPARE_LOCKERR)) - { - ERR_post(Arg::Gds(isc_deadlock) << - Arg::Gds(isc_update_conflict) << - Arg::Gds(isc_concurrent_transaction) << Arg::Num(org_rpb->rpb_transaction_nr)); - } - if (prepare_result) - { - jrd_req* top_request = tdbb->getRequest()->req_snapshot.m_owner; - top_request->req_flags |= req_update_conflict; - top_request->req_conflict_txn = org_rpb->rpb_transaction_nr; + if (!check_prepare_result(prepare_result, transaction, tdbb->getRequest(), org_rpb)) return false; - } IDX_modify_flag_uk_modified(tdbb, org_rpb, new_rpb, transaction); @@ -4085,8 +4112,18 @@ bool VIO_writelock(thread_db* tdbb, record_param* org_rpb, jrd_tra* transaction) if ((transaction->tra_flags & TRA_read_consistency)) { jrd_req* top_request = tdbb->getRequest()->req_snapshot.m_owner; - top_request->req_flags |= req_update_conflict; - top_request->req_conflict_txn = org_rpb->rpb_transaction_nr; + if (top_request && !(top_request->req_flags & req_update_conflict)) + { + if (!(top_request->req_flags & req_restart_ready)) + { + ERR_post(Arg::Gds(isc_deadlock) << + Arg::Gds(isc_update_conflict) << + Arg::Gds(isc_concurrent_transaction) << Arg::Num(org_rpb->rpb_transaction_nr)); + } + + top_request->req_flags |= req_update_conflict; + top_request->req_conflict_txn = org_rpb->rpb_transaction_nr; + } } org_rpb->rpb_runtime_flags |= RPB_refetch; return false; From 83da5677f2c0defedb3d818a710fb9e4af00f959 Mon Sep 17 00:00:00 2001 From: hvlad Date: Thu, 5 Mar 2020 12:57:04 +0200 Subject: [PATCH 17/24] Fixed regression found by Nikolay --- src/dsql/StmtNodes.cpp | 29 +++++++++++++++++++---------- src/jrd/Savepoint.cpp | 2 +- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index e7ed74d022..f51096c923 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -86,6 +86,7 @@ static void dsqlSetParameterName(DsqlCompilerScratch*, ExprNode*, const ValueExp static void dsqlSetParametersName(DsqlCompilerScratch*, CompoundStmtNode*, const RecordSourceNode*); static void cleanupRpb(thread_db* tdbb, record_param* rpb); +static void forceWriteLock(thread_db* tdbb, record_param* rpb, jrd_tra* transaction); static void makeValidation(thread_db* tdbb, CompilerScratch* csb, StreamType stream, Array& validations); static StmtNode* pass1ExpandView(thread_db* tdbb, CompilerScratch* csb, StreamType orgStream, @@ -2623,7 +2624,7 @@ const StmtNode* EraseNode::erase(thread_db* tdbb, jrd_req* request, WhichTrigger if (forNode && forNode->isWriteLockMode(request)) { - VIO_writelock(tdbb, rpb, transaction); + forceWriteLock(tdbb, rpb, transaction); return parentStmt; } @@ -2668,10 +2669,7 @@ const StmtNode* EraseNode::erase(thread_db* tdbb, jrd_req* request, WhichTrigger forNode->setWriteLockMode(request); - // VIO_writelock returns false if record has been deleted by someone else. - // Gently ignore this situation and proceed further. - rpb->rpb_runtime_flags |= RPB_refetch; - VIO_writelock(tdbb, rpb, transaction); + forceWriteLock(tdbb, rpb, transaction); return parentStmt; } REPL_erase(tdbb, rpb, transaction); @@ -6546,7 +6544,7 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg { if (forNode && forNode->isWriteLockMode(request)) { - VIO_writelock(tdbb, orgRpb, transaction); + forceWriteLock(tdbb, orgRpb, transaction); return parentStmt; } @@ -6584,10 +6582,7 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg forNode->setWriteLockMode(request); - // VIO_writelock returns false if record has been deleted by someone else. - // Gently ignore this situation and proceed further. - orgRpb->rpb_runtime_flags |= RPB_refetch; - VIO_writelock(tdbb, orgRpb, transaction); + forceWriteLock(tdbb, orgRpb, transaction); return parentStmt; } IDX_modify(tdbb, orgRpb, newRpb, transaction); @@ -9621,6 +9616,20 @@ static void cleanupRpb(thread_db* tdbb, record_param* rpb) } } +// Try to set write lock on record until success or record exists +static void forceWriteLock(thread_db * tdbb, record_param * rpb, jrd_tra * transaction) +{ + while (VIO_refetch_record(tdbb, rpb, transaction, true, true)) + { + rpb->rpb_runtime_flags &= ~RPB_refetch; + + // VIO_writelock returns false if record has been deleted or modified + // by someone else. + if (VIO_writelock(tdbb, rpb, transaction)) + break; + } +} + // Build a validation list for a relation, if appropriate. static void makeValidation(thread_db* tdbb, CompilerScratch* csb, StreamType stream, Array& validations) diff --git a/src/jrd/Savepoint.cpp b/src/jrd/Savepoint.cpp index 41de5ab799..0e50912d90 100644 --- a/src/jrd/Savepoint.cpp +++ b/src/jrd/Savepoint.cpp @@ -443,7 +443,7 @@ Savepoint* Savepoint::rollback(thread_db* tdbb, Savepoint* prior, bool preserveL VerbAction* preserveAction = nullptr; if (preserveLocks && m_next) - preserveAction = m_next->getAction(action->vct_relation); + preserveAction = m_next->createAction(action->vct_relation); action->undo(tdbb, m_transaction, preserveAction); From c82dcc915e7ec7265e1b41039cfa99a874c320a1 Mon Sep 17 00:00:00 2001 From: hvlad Date: Fri, 6 Mar 2020 16:13:32 +0200 Subject: [PATCH 18/24] Let restart request immediately when update conflict happens at positioned UPDATE\DELETE --- src/dsql/StmtNodes.cpp | 58 ++++++++++++++++++------------------------ src/jrd/req.h | 1 - src/jrd/vio.cpp | 3 +-- 3 files changed, 26 insertions(+), 36 deletions(-) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index f51096c923..629daf99da 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -102,6 +102,7 @@ static void preModifyEraseTriggers(thread_db* tdbb, TrigVector** trigs, StmtNode::WhichTrigger whichTrig, record_param* rpb, record_param* rec, TriggerAction op); static void preprocessAssignments(thread_db* tdbb, CompilerScratch* csb, StreamType stream, CompoundStmtNode* compoundNode, const Nullable* insertOverride); +static void restartRequest(const jrd_req* request, jrd_tra* transaction); static void validateExpressions(thread_db* tdbb, const Array& validations); } // namespace Jrd @@ -2654,22 +2655,15 @@ const StmtNode* EraseNode::erase(thread_db* tdbb, jrd_req* request, WhichTrigger // VIO_erase returns false if there is an update conflict in Read Consistency // transaction. Before returning false it disables statement-level snapshot // (via setting req_update_conflict flag) so re-fetch should see new data. - // Deleting new version blindly is generally unsafe, but is ok in this situation - // because all changes made by this request will certainly be undone and request - // will be restarted. - - if (forNode) - rpb->rpb_runtime_flags |= RPB_restart_ready; - else - rpb->rpb_runtime_flags &= ~RPB_restart_ready; if (!VIO_erase(tdbb, rpb, transaction)) { - fb_assert(forNode != nullptr); + forceWriteLock(tdbb, rpb, transaction); + + if (!forNode) + restartRequest(request, transaction); forNode->setWriteLockMode(request); - - forceWriteLock(tdbb, rpb, transaction); return parentStmt; } REPL_erase(tdbb, rpb, transaction); @@ -5066,17 +5060,7 @@ const StmtNode* ForNode::execute(thread_db* tdbb, jrd_req* request, ExeState* /* } if (impure->writeLockMode) - { - const jrd_req* top_request = request->req_snapshot.m_owner; - fb_assert(top_request); - fb_assert(top_request->req_flags & req_update_conflict); - - transaction->tra_flags |= TRA_ex_restart; - - ERR_post(Arg::Gds(isc_deadlock) << - Arg::Gds(isc_update_conflict) << - Arg::Gds(isc_concurrent_transaction) << Arg::Num(top_request->req_conflict_txn)); - } + restartRequest(request, transaction); request->req_operation = jrd_req::req_return; // fall into @@ -6567,22 +6551,17 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg else if (!relation->rel_view_rse) { // VIO_modify returns false if there is an update conflict in Read Consistency - // transaction and our code is ready to work in write lock mode (see flag set below). - // Before returning false it disables statement-level snapshot + // transaction. Before returning false it disables statement-level snapshot // (via setting req_update_conflict flag) so re-fetch should see new data. - if (forNode) - orgRpb->rpb_runtime_flags |= RPB_restart_ready; - else - orgRpb->rpb_runtime_flags &= ~RPB_restart_ready; - if (!VIO_modify(tdbb, orgRpb, newRpb, transaction)) { - fb_assert(forNode != nullptr); - - forNode->setWriteLockMode(request); - forceWriteLock(tdbb, orgRpb, transaction); + + if (!forNode) + restartRequest(request, transaction); + + forNode->setWriteLockMode(request); return parentStmt; } IDX_modify(tdbb, orgRpb, newRpb, transaction); @@ -10008,6 +9987,19 @@ static void preprocessAssignments(thread_db* tdbb, CompilerScratch* csb, } } +static void restartRequest(const jrd_req* request, jrd_tra* transaction) +{ + const jrd_req* top_request = request->req_snapshot.m_owner; + fb_assert(top_request); + fb_assert(top_request->req_flags & req_update_conflict); + + transaction->tra_flags |= TRA_ex_restart; + + ERR_post(Arg::Gds(isc_deadlock) << + Arg::Gds(isc_update_conflict) << + Arg::Gds(isc_concurrent_transaction) << Arg::Num(top_request->req_conflict_txn)); +} + // Execute a list of validation expressions. static void validateExpressions(thread_db* tdbb, const Array& validations) { diff --git a/src/jrd/req.h b/src/jrd/req.h index 3884c7261f..d4c86d04b0 100644 --- a/src/jrd/req.h +++ b/src/jrd/req.h @@ -132,7 +132,6 @@ const USHORT RPB_refetch = 0x01; // re-fetch is required const USHORT RPB_undo_data = 0x02; // data got from undo log const USHORT RPB_undo_read = 0x04; // read was performed using the undo log const USHORT RPB_undo_deleted = 0x08; // read was performed using the undo log, primary version is deleted -const USHORT RPB_restart_ready = 0x10; // update conflict could be handled by statement restart const USHORT RPB_UNDO_FLAGS = (RPB_undo_data | RPB_undo_read | RPB_undo_deleted); diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index e2da05c5e1..ed69f62d38 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -1465,8 +1465,7 @@ static bool check_prepare_result(int prepare_result, jrd_tra* transaction, jrd_r jrd_req* top_request = request->req_snapshot.m_owner; const bool restart_ready = top_request && - (top_request->req_flags & req_restart_ready) && - (rpb->rpb_runtime_flags & RPB_restart_ready); + (top_request->req_flags & req_restart_ready); // Second update conflict when request is already in update conflict mode // means we have some (indirect) UPDATE\DELETE in WHERE clause of primary From 16b39a8a9605e1d7d8e47f9de20be6ec132fbabd Mon Sep 17 00:00:00 2001 From: hvlad Date: Fri, 6 Mar 2020 16:24:15 +0200 Subject: [PATCH 19/24] Correct usage of Arg::Int64, thanks to Dmitry --- src/dsql/StmtNodes.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index 629daf99da..61de239c7c 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -9997,7 +9997,7 @@ static void restartRequest(const jrd_req* request, jrd_tra* transaction) ERR_post(Arg::Gds(isc_deadlock) << Arg::Gds(isc_update_conflict) << - Arg::Gds(isc_concurrent_transaction) << Arg::Num(top_request->req_conflict_txn)); + Arg::Gds(isc_concurrent_transaction) << Arg::Int64(top_request->req_conflict_txn)); } // Execute a list of validation expressions. From 7a97a8b050285aea434af7ebf68a26915c095b97 Mon Sep 17 00:00:00 2001 From: hvlad Date: Wed, 11 Mar 2020 12:37:24 +0200 Subject: [PATCH 20/24] Introduced generic way to add some specific markers for primary BLR verbs using new blr_marks code. Debug info markers is replaced by blr_marks. Debug info format and version is changed back. --- src/dsql/BlrDebugWriter.cpp | 7 ----- src/dsql/BlrDebugWriter.h | 1 - src/dsql/DsqlCompilerScratch.cpp | 21 +++++++++++++++ src/dsql/DsqlCompilerScratch.h | 1 + src/dsql/StmtNodes.cpp | 36 ++++++++++++-------------- src/include/firebird/impl/blr.h | 2 ++ src/include/firebird/impl/consts_pub.h | 1 - src/jrd/DebugInterface.cpp | 22 ---------------- src/jrd/DebugInterface.h | 10 ++----- src/jrd/blp.h | 1 + src/jrd/par.cpp | 21 +++++++++++++++ src/jrd/par_proto.h | 1 + 12 files changed, 65 insertions(+), 59 deletions(-) diff --git a/src/dsql/BlrDebugWriter.cpp b/src/dsql/BlrDebugWriter.cpp index f20040fb92..fdb810c440 100644 --- a/src/dsql/BlrDebugWriter.cpp +++ b/src/dsql/BlrDebugWriter.cpp @@ -141,13 +141,6 @@ void BlrDebugWriter::putDebugSubProcedure(DeclareSubProcNode* subProcNode) debugData.add(subDebugData.begin(), count); } -void BlrDebugWriter::putDebugMarkers(ULONG marks) -{ - debugData.add(fb_dbg_map_markers); - putValue(marks); - putBlrOffset(); -} - void BlrDebugWriter::putValue(ULONG val) { debugData.add(val); diff --git a/src/dsql/BlrDebugWriter.h b/src/dsql/BlrDebugWriter.h index 41980861ba..ec81544e50 100644 --- a/src/dsql/BlrDebugWriter.h +++ b/src/dsql/BlrDebugWriter.h @@ -50,7 +50,6 @@ public: void putDebugCursor(USHORT, const Firebird::MetaName&); void putDebugSubFunction(DeclareSubFuncNode* subFuncNode); void putDebugSubProcedure(DeclareSubProcNode* subProcNode); - void putDebugMarkers(ULONG marks); DebugData& getDebugData() { return debugData; } diff --git a/src/dsql/DsqlCompilerScratch.cpp b/src/dsql/DsqlCompilerScratch.cpp index aa5bd71736..a84970b0b8 100644 --- a/src/dsql/DsqlCompilerScratch.cpp +++ b/src/dsql/DsqlCompilerScratch.cpp @@ -59,6 +59,27 @@ void DsqlCompilerScratch::dumpContextStack(const DsqlContextStack* stack) #endif +void DsqlCompilerScratch::putBlrMarkers(ULONG marks) +{ + appendUChar(blr_marks); + if (marks <= MAX_UCHAR) + { + appendUChar(1); + appendUChar(marks); + } + else if (marks <= MAX_USHORT) + { + appendUChar(2); + appendUShort(marks); + } + else + { + appendUChar(4); + appendULong(marks); + } +} + + // Write out field data type. // Taking special care to declare international text. void DsqlCompilerScratch::putDtype(const TypeClause* field, bool useSubType) diff --git a/src/dsql/DsqlCompilerScratch.h b/src/dsql/DsqlCompilerScratch.h index e54cea0ff1..21b93d731a 100644 --- a/src/dsql/DsqlCompilerScratch.h +++ b/src/dsql/DsqlCompilerScratch.h @@ -169,6 +169,7 @@ public: return statement; } + void putBlrMarkers(ULONG marks); void putDtype(const TypeClause* field, bool useSubType); void putType(const TypeClause* type, bool useSubType); void putLocalVariables(CompoundStmtNode* parameters, USHORT locals); diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index 61de239c7c..5a60da1189 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -2274,7 +2274,6 @@ static RegisterNode regEraseNode(blr_erase); DmlNode* EraseNode::parse(thread_db* /*tdbb*/, MemoryPool& pool, CompilerScratch* csb, const UCHAR /*blrOp*/) { - const ULONG blrOffset = csb->csb_blr_reader.getOffset(); const USHORT n = csb->csb_blr_reader.getByte(); if (n >= csb->csb_rpt.getCount() || !(csb->csb_rpt[n].csb_flags & csb_used)) @@ -2283,9 +2282,8 @@ DmlNode* EraseNode::parse(thread_db* /*tdbb*/, MemoryPool& pool, CompilerScratch EraseNode* node = FB_NEW_POOL(pool) EraseNode(pool); node->stream = csb->csb_rpt[n].csb_stream; - ULONG marks; - if (csb->csb_dbg_info && csb->csb_dbg_info->blrToMarks.get(blrOffset, marks)) - node->marks |= marks; + if (csb->csb_blr_reader.peekByte() == blr_marks) + node->marks |= PAR_marks(csb); return node; } @@ -2404,10 +2402,11 @@ void EraseNode::genBlr(DsqlCompilerScratch* dsqlScratch) } dsqlScratch->appendUChar(blr_erase); - if (marks) - dsqlScratch->putDebugMarkers(marks); GEN_stuff_context(dsqlScratch, context); + if (marks) + dsqlScratch->putBlrMarkers(marks); + if (statement) dsqlScratch->appendUChar(blr_end); @@ -4812,10 +4811,8 @@ DmlNode* ForNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* csb, { ForNode* node = FB_NEW_POOL(pool) ForNode(pool); - const ULONG blrOffset = csb->csb_blr_reader.getOffset(); - ULONG marks; - if (csb->csb_dbg_info && csb->csb_dbg_info->blrToMarks.get(blrOffset, marks)) - node->forUpdate = (marks & StmtNode::MARK_FOR_UPDATE) != 0; + if (csb->csb_blr_reader.peekByte() == blr_marks) + node->forUpdate = (PAR_marks(csb) & StmtNode::MARK_FOR_UPDATE) != 0; if (csb->csb_blr_reader.peekByte() == (UCHAR) blr_stall) node->stall = PAR_parse_stmt(tdbb, csb); @@ -4932,7 +4929,7 @@ void ForNode::genBlr(DsqlCompilerScratch* dsqlScratch) dsqlScratch->appendUChar(blr_for); if (forUpdate) - dsqlScratch->putDebugMarkers(StmtNode::MARK_FOR_UPDATE); + dsqlScratch->putBlrMarkers(StmtNode::MARK_FOR_UPDATE); if (!statement || dsqlForceSingular) dsqlScratch->appendUChar(blr_singular); @@ -5970,8 +5967,6 @@ static RegisterNode regModifyNode2(blr_modify2); DmlNode* ModifyNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* csb, const UCHAR blrOp) { // Parse the original and new contexts. - const ULONG blrOffset = csb->csb_blr_reader.getOffset(); - USHORT context = (unsigned int) csb->csb_blr_reader.getByte(); if (context >= csb->csb_rpt.getCount() || !(csb->csb_rpt[context].csb_flags & csb_used)) @@ -6000,6 +5995,9 @@ DmlNode* ModifyNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* c node->orgStream = orgStream; node->newStream = newStream; + if (csb->csb_blr_reader.peekByte() == blr_marks) + node->marks |= PAR_marks(csb); + AutoSetRestore autoCurrentDMLNode(&csb->csb_currentDMLNode, node); node->statement = PAR_parse_stmt(tdbb, csb); @@ -6007,10 +6005,6 @@ DmlNode* ModifyNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* c if (blrOp == blr_modify2) node->statement2 = PAR_parse_stmt(tdbb, csb); - ULONG marks; - if (csb->csb_dbg_info && csb->csb_dbg_info->blrToMarks.get(blrOffset, marks)) - node->marks |= marks; - return node; } @@ -6239,8 +6233,6 @@ void ModifyNode::genBlr(DsqlCompilerScratch* dsqlScratch) const dsql_msg* message = dsqlGenDmlHeader(dsqlScratch, rse); dsqlScratch->appendUChar(statement2 ? blr_modify2 : blr_modify); - if (marks) - dsqlScratch->putDebugMarkers(marks); const dsql_ctx* context; @@ -6255,6 +6247,10 @@ void ModifyNode::genBlr(DsqlCompilerScratch* dsqlScratch) GEN_stuff_context(dsqlScratch, context); context = dsqlRelation->dsqlContext; GEN_stuff_context(dsqlScratch, context); + + if (marks) + dsqlScratch->putBlrMarkers(marks); + statement->genBlr(dsqlScratch); if (statement2) @@ -8913,7 +8909,7 @@ static const dsql_msg* dsqlGenDmlHeader(DsqlCompilerScratch* dsqlScratch, RseNod if (dsqlRse) { dsqlScratch->appendUChar(blr_for); - dsqlScratch->putDebugMarkers(StmtNode::MARK_FOR_UPDATE); + dsqlScratch->putBlrMarkers(StmtNode::MARK_FOR_UPDATE); GEN_expr(dsqlScratch, dsqlRse); } diff --git a/src/include/firebird/impl/blr.h b/src/include/firebird/impl/blr.h index 70a41ac159..b75354c8c6 100644 --- a/src/include/firebird/impl/blr.h +++ b/src/include/firebird/impl/blr.h @@ -441,4 +441,6 @@ #define blr_at_local (unsigned char) 0 #define blr_at_zone (unsigned char) 1 +#define blr_marks (unsigned char) 217 // mark some blr code with specific flags + #endif // FIREBIRD_IMPL_BLR_H diff --git a/src/include/firebird/impl/consts_pub.h b/src/include/firebird/impl/consts_pub.h index 74be2b75bb..9ba14d70f9 100644 --- a/src/include/firebird/impl/consts_pub.h +++ b/src/include/firebird/impl/consts_pub.h @@ -740,7 +740,6 @@ #define fb_dbg_subproc 5 #define fb_dbg_subfunc 6 #define fb_dbg_map_curname 7 -#define fb_dbg_map_markers 8 // sub code for fb_dbg_map_argument #define fb_dbg_arg_input 0 diff --git a/src/jrd/DebugInterface.cpp b/src/jrd/DebugInterface.cpp index d0e528e8b1..79de9e8147 100644 --- a/src/jrd/DebugInterface.cpp +++ b/src/jrd/DebugInterface.cpp @@ -225,28 +225,6 @@ void DBG_parse_debug_info(ULONG length, const UCHAR* data, DbgInfo& dbgInfo) break; } - case fb_dbg_map_markers: - { - if (data + 8 >= end) - { - bad_format = true; - break; - } - - ULONG marks = *data++; - marks |= *data++ << 8; - marks |= *data++ << 16; - marks |= *data++ << 24; - - ULONG offset = *data++; - offset |= *data++ << 8; - offset |= *data++ << 16; - offset |= *data++ << 24; - - dbgInfo.blrToMarks.put(offset, marks); - } - break; - case fb_dbg_end: if (data != end) bad_format = true; diff --git a/src/jrd/DebugInterface.h b/src/jrd/DebugInterface.h index b7f3836e5c..f7cd7253f9 100644 --- a/src/jrd/DebugInterface.h +++ b/src/jrd/DebugInterface.h @@ -33,8 +33,7 @@ // Also, it introduces some new tags. const UCHAR DBG_INFO_VERSION_1 = UCHAR(1); const UCHAR DBG_INFO_VERSION_2 = UCHAR(2); -const UCHAR DBG_INFO_VERSION_3 = UCHAR(3); // blr offsets of "update" cursors and driven sub-statements -const UCHAR CURRENT_DBG_INFO_VERSION = DBG_INFO_VERSION_3; +const UCHAR CURRENT_DBG_INFO_VERSION = DBG_INFO_VERSION_2; namespace Firebird { @@ -56,7 +55,6 @@ typedef Firebird::SortedArray< MapBlrToSrcItem> MapBlrToSrc; typedef GenericMap > > MapVarIndexToName; -typedef GenericMap > > MapBlrToMarks; struct ArgumentInfo { @@ -95,8 +93,7 @@ struct DbgInfo : public PermanentStorage argInfoToName(p), curIndexToName(p), subFuncs(p), - subProcs(p), - blrToMarks(p) + subProcs(p) { } @@ -129,8 +126,6 @@ struct DbgInfo : public PermanentStorage subProcs.clear(); } - - blrToMarks.clear(); } MapBlrToSrc blrToSrc; // mapping between blr offsets and source text position @@ -139,7 +134,6 @@ struct DbgInfo : public PermanentStorage MapVarIndexToName curIndexToName; // mapping between cursor index and name GenericMap > > subFuncs; // sub functions GenericMap > > subProcs; // sub procedures - MapBlrToMarks blrToMarks; // blr offsets of marked verbs }; } // namespace Firebird diff --git a/src/jrd/blp.h b/src/jrd/blp.h index da41ec60aa..a6e41a55b0 100644 --- a/src/jrd/blp.h +++ b/src/jrd/blp.h @@ -247,5 +247,6 @@ static const struct {"local_timestamp", byte_line}, {"local_time", byte_line}, {"at", verb_byte_verb}, + {"marks", byte_literal}, {0, 0} }; diff --git a/src/jrd/par.cpp b/src/jrd/par.cpp index 951cdeec40..99b1027ae6 100644 --- a/src/jrd/par.cpp +++ b/src/jrd/par.cpp @@ -683,6 +683,26 @@ CompoundStmtNode* PAR_make_list(thread_db* tdbb, StmtNodeStack& stack) } +ULONG PAR_marks(Jrd::CompilerScratch* csb) +{ + if (csb->csb_blr_reader.getByte() != blr_marks) + PAR_syntax_error(csb, "blr_marks"); + + switch (csb->csb_blr_reader.getByte()) + { + case 1: + return csb->csb_blr_reader.getByte(); + + case 2: + return csb->csb_blr_reader.getWord(); + + case 4: + return csb->csb_blr_reader.getLong(); + } + PAR_syntax_error(csb, "valid length for blr_marks value (1, 2, or 4)"); + return 0; +} + CompilerScratch* PAR_parse(thread_db* tdbb, const UCHAR* blr, ULONG blr_length, bool internal_flag, ULONG dbginfo_length, const UCHAR* dbginfo) { @@ -1627,6 +1647,7 @@ void PAR_syntax_error(CompilerScratch* csb, const TEXT* string) csb->csb_blr_reader.seekBackward(1); + // BLR syntax error: expected @1 at offset @2, encountered @3 PAR_error(csb, Arg::Gds(isc_syntaxerr) << Arg::Str(string) << Arg::Num(csb->csb_blr_reader.getOffset()) << Arg::Num(csb->csb_blr_reader.peekByte())); diff --git a/src/jrd/par_proto.h b/src/jrd/par_proto.h index 487f8dfe8b..13a5c8594c 100644 --- a/src/jrd/par_proto.h +++ b/src/jrd/par_proto.h @@ -61,6 +61,7 @@ SSHORT PAR_find_proc_field(const Jrd::jrd_prc*, const Firebird::MetaName&); Jrd::ValueExprNode* PAR_gen_field(Jrd::thread_db* tdbb, StreamType stream, USHORT id, bool byId = false); Jrd::ValueExprNode* PAR_make_field(Jrd::thread_db*, Jrd::CompilerScratch*, USHORT, const Firebird::MetaName&); Jrd::CompoundStmtNode* PAR_make_list(Jrd::thread_db*, Jrd::StmtNodeStack&); +ULONG PAR_marks(Jrd::CompilerScratch*); Jrd::CompilerScratch* PAR_parse(Jrd::thread_db*, const UCHAR* blr, ULONG blr_length, bool internal_flag, ULONG = 0, const UCHAR* = NULL); From e7b97e72904bce5a15b0338e018f0b6b86d58543 Mon Sep 17 00:00:00 2001 From: hvlad Date: Sun, 22 Mar 2020 14:04:15 +0200 Subject: [PATCH 21/24] Restore how DebugInfo was generated. Misc. --- src/dsql/StmtNodes.cpp | 7 ++++++- src/dsql/dsql.cpp | 4 ---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/dsql/StmtNodes.cpp b/src/dsql/StmtNodes.cpp index 5a60da1189..f3dd9008d1 100644 --- a/src/dsql/StmtNodes.cpp +++ b/src/dsql/StmtNodes.cpp @@ -4323,6 +4323,8 @@ void ExecBlockNode::genBlr(DsqlCompilerScratch* dsqlScratch) { thread_db* tdbb = JRD_get_thread_data(); + dsqlScratch->beginDebug(); + // Sub routine needs a different approach from EXECUTE BLOCK. // EXECUTE BLOCK needs "ports", which creates DSQL messages using the client charset. // Sub routine doesn't need ports and should generate BLR as declared in its metadata. @@ -4457,6 +4459,8 @@ void ExecBlockNode::genBlr(DsqlCompilerScratch* dsqlScratch) dsqlScratch->appendUChar(blr_end); dsqlScratch->genReturn(true); dsqlScratch->appendUChar(blr_end); + + dsqlScratch->endDebug(); } // Revert parameters order for EXECUTE BLOCK statement @@ -5967,6 +5971,7 @@ static RegisterNode regModifyNode2(blr_modify2); DmlNode* ModifyNode::parse(thread_db* tdbb, MemoryPool& pool, CompilerScratch* csb, const UCHAR blrOp) { // Parse the original and new contexts. + USHORT context = (unsigned int) csb->csb_blr_reader.getByte(); if (context >= csb->csb_rpt.getCount() || !(csb->csb_rpt[context].csb_flags & csb_used)) @@ -6556,7 +6561,7 @@ const StmtNode* ModifyNode::modify(thread_db* tdbb, jrd_req* request, WhichTrigg if (!forNode) restartRequest(request, transaction); - + forNode->setWriteLockMode(request); return parentStmt; } diff --git a/src/dsql/dsql.cpp b/src/dsql/dsql.cpp index c161eeb5b5..5dea42e7f6 100644 --- a/src/dsql/dsql.cpp +++ b/src/dsql/dsql.cpp @@ -603,12 +603,8 @@ void DsqlDmlRequest::dsqlPass(thread_db* tdbb, DsqlCompilerScratch* scratch, boo else scratch->getStatement()->setBlrVersion(4); - scratch->beginDebug(); - GEN_request(scratch, node); - scratch->endDebug(); - // Create the messages buffers for (FB_SIZE_T i = 0; i < scratch->ports.getCount(); ++i) { From 48ce67206db7634c5ae15f8cdc3ad825f54f713a Mon Sep 17 00:00:00 2001 From: hvlad Date: Sun, 22 Mar 2020 23:36:43 +0200 Subject: [PATCH 22/24] Preserve locks when required and there is no next savepoint (for ex. no transaction-level savepoint) --- src/jrd/Savepoint.cpp | 11 ++++++----- src/jrd/Savepoint.h | 3 ++- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/jrd/Savepoint.cpp b/src/jrd/Savepoint.cpp index 0e50912d90..19b898bf33 100644 --- a/src/jrd/Savepoint.cpp +++ b/src/jrd/Savepoint.cpp @@ -234,7 +234,7 @@ void VerbAction::mergeTo(thread_db* tdbb, jrd_tra* transaction, VerbAction* next release(transaction); } -void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, VerbAction* preserveAction) +void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks, VerbAction* preserveAction) { // Undo changes recorded for this verb action. // After that, clear the verb action and prepare it for later reuse. @@ -258,7 +258,7 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, VerbAction* preserv if (!DPM_get(tdbb, &rpb, LCK_read)) BUGCHECK(186); // msg 186 record disappeared - if ((have_undo || preserveAction) && !(rpb.rpb_flags & rpb_deleted)) + if ((have_undo || preserveLocks) && !(rpb.rpb_flags & rpb_deleted)) VIO_data(tdbb, &rpb, transaction->tra_pool); else CCH_RELEASE(tdbb, &rpb.getWindow(tdbb)); @@ -268,7 +268,7 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, VerbAction* preserv if (!have_undo) { - if (preserveAction && rpb.rpb_b_page) + if (preserveLocks && rpb.rpb_b_page) { // Fetch previous record version and update in place current version with it record_param temp = rpb; @@ -305,7 +305,8 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, VerbAction* preserv delete temp.rpb_record; - RBM_SET(transaction->tra_pool, &preserveAction->vct_records, rpb.rpb_number.getValue()); + if (preserveAction) + RBM_SET(transaction->tra_pool, &preserveAction->vct_records, rpb.rpb_number.getValue()); } else VIO_backout(tdbb, &rpb, transaction); @@ -445,7 +446,7 @@ Savepoint* Savepoint::rollback(thread_db* tdbb, Savepoint* prior, bool preserveL if (preserveLocks && m_next) preserveAction = m_next->createAction(action->vct_relation); - action->undo(tdbb, m_transaction, preserveAction); + action->undo(tdbb, m_transaction, preserveLocks, preserveAction); m_actions = action->vct_next; action->vct_next = m_freeActions; diff --git a/src/jrd/Savepoint.h b/src/jrd/Savepoint.h index e264ff2ba9..81340de405 100644 --- a/src/jrd/Savepoint.h +++ b/src/jrd/Savepoint.h @@ -94,7 +94,8 @@ namespace Jrd UndoItemTree* vct_undo; // Data for undo records void mergeTo(thread_db* tdbb, jrd_tra* transaction, VerbAction* nextAction); - void undo(thread_db* tdbb, jrd_tra* transaction, VerbAction* preserveAction); + void undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks, + VerbAction* preserveAction); void garbageCollectIdxLite(thread_db* tdbb, jrd_tra* transaction, SINT64 recordNumber, VerbAction* nextAction, Record* goingRecord); From 67491910a4cbf17186bf41648b2d84d9fda4edb7 Mon Sep 17 00:00:00 2001 From: hvlad Date: Fri, 27 Mar 2020 12:01:05 +0200 Subject: [PATCH 23/24] Update documentation --- doc/README.read_consistency.md | 55 +++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/doc/README.read_consistency.md b/doc/README.read_consistency.md index b2c2429cbe..52bcef203c 100644 --- a/doc/README.read_consistency.md +++ b/doc/README.read_consistency.md @@ -125,32 +125,53 @@ So, there are three kinds of read-committed transactions now: ### Update conflicts handling -When statement executed within read committed read consistency transaction its database view is +When statement executed within *read committed read consistency* transaction its database view is not changed (similar to snapshot transaction). Therefore it is useless to wait for commit of concurrent transaction in the hope to re-read new committed record version. On read, behavior is similar to read committed *record version* transaction - do not wait for active transaction and walk backversions chain looking for record version visible to the current snapshot. -When update conflict happens engine behavior is changed. If concurrent transaction is active, -engine waits (according to transaction lock timeout) and if concurrent transaction still not -committed, update conflict error is returned. If concurrent transaction is committed, engine -creates new snapshot and restart top-level statement execution. In both cases all work performed -up to the top-level statement is undone. +For *read committed read consistency* mode handling of update conflicts by the engine is changed +significantly. +When update conflict is detected the following is performed: +a) transaction isolation mode temporarily switched to the read committed *no record version mode* +b) engine put write lock on conflicted record +c) engine continue to evaluate remaining records of update\delete cursor and put write locks + on it too +d) when there is no more records to fetch, engine start to undo all actions performed since + top-level statement execution starts and put write locks on every updated\deleted record, + all inserted records are removed +e) then engine restores transaction isolation mode as read committed *read consistency*, creates + new statement-level snapshot and restart execution of top-level statement. -This is the same logic as user applications uses for update conflict handling usually, but it -is a bit more efficient as it excludes network roundtrips to\from client host. This restart -logic is not applied to the selectable stored procedures if update conflict happens after any -record returned to the client application. In this case *isc_update_conflict* error is returned. +Such algorithm allows to ensure that after restart already updated records remains locked, +will be visible to the new snapshot, and could be updated again with no further conflicts. +Also, because of read consistency mode, set of modified records remains consistent. -Note: by historical reasons isc_update_conflict reported as secondary error code with primary -error code isc_deadlock. +Notes: +- restart algorithm above is applied to the UPDATE, DELETE, SELECT WITH LOCK and MERGE + statements, with and without RETURNING clause, executing directly by user applicaiton or + as a part of some PSQL object (stored procedure\function, trigger, EXECUTE BLOCK, etc) +- if UPDATE\DELETE statement is positioned on some explicit cursor (WHERE CURRENT OF) then + engine skip step (c) above, i.e. not fetches and not put write locks on remaining records + of cursor +- if top-level statement is SELECT'able and update conflict happens after one or more records + was returned to the application, then update conflict error is reported as usual and restart + is not initiated +- restart is not initiated for statements in autonomous blocks +- after 10 attempts engine stop restarts and report update conflict +- by historical reasons isc_update_conflict reported as secondary error code with primary + error code isc_deadlock. -### No more precommitted transactions -*Read-committed read only* (RCRO) transactions currently marked as committed immediately when -transaction started. This is correct if read-committed transaction needs no database snapshot. -But it is not correct to mark RCRO transaction as committed as it can break statement-level -snapshot consistency. +### Precommitted transactions + +*Read-committed read only* transactions marked as committed immediately when transaction started. +This is correct if read-committed transaction needs no database snapshot. But it is not correct +to mark *read consistency read only* transaction as committed as it can break statement-level +snapshot consistency. Therefore *read consistency read only* transactions is not precommitted +on start. Other kinds of read committed read only transactions (*[no] record version*) works as +before and marked as committed when such transaction started. ### Support for new READ COMMITTED READ CONSISTENCY isolation level #### SQL syntax From 7c5e3b3af501cce0a93ec1c6d44b355918b965e2 Mon Sep 17 00:00:00 2001 From: hvlad Date: Sat, 28 Mar 2020 12:15:33 +0200 Subject: [PATCH 24/24] Update documentation, consistent style. Thanks to Roman and Nikolay. --- doc/README.read_consistency.md | 60 +++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/doc/README.read_consistency.md b/doc/README.read_consistency.md index 52bcef203c..fca9b49253 100644 --- a/doc/README.read_consistency.md +++ b/doc/README.read_consistency.md @@ -125,23 +125,23 @@ So, there are three kinds of read-committed transactions now: ### Update conflicts handling -When statement executed within *read committed read consistency* transaction its database view is +When statement executed within READ COMMITTED READ CONSISTENCY transaction its database view is not changed (similar to snapshot transaction). Therefore it is useless to wait for commit of concurrent transaction in the hope to re-read new committed record version. On read, behavior is -similar to read committed *record version* transaction - do not wait for active transaction and +similar to READ COMMITTED *RECORD VERSION* transaction - do not wait for active transaction and walk backversions chain looking for record version visible to the current snapshot. -For *read committed read consistency* mode handling of update conflicts by the engine is changed +For READ COMMITTED *READ CONSISTENCY* mode handling of update conflicts by the engine is changed significantly. When update conflict is detected the following is performed: -a) transaction isolation mode temporarily switched to the read committed *no record version mode* +a) transaction isolation mode temporarily switched to the READ COMMITTED *NO RECORD VERSION MODE* b) engine put write lock on conflicted record c) engine continue to evaluate remaining records of update\delete cursor and put write locks on it too d) when there is no more records to fetch, engine start to undo all actions performed since - top-level statement execution starts and put write locks on every updated\deleted record, - all inserted records are removed -e) then engine restores transaction isolation mode as read committed *read consistency*, creates + top-level statement execution starts and preserve already taken write locks for every + updated\deleted\locked record, all inserted records are removed +e) then engine restores transaction isolation mode as READ COMMITTED *READ CONSISTENCY*, creates new statement-level snapshot and restart execution of top-level statement. Such algorithm allows to ensure that after restart already updated records remains locked, @@ -149,28 +149,33 @@ will be visible to the new snapshot, and could be updated again with no further Also, because of read consistency mode, set of modified records remains consistent. Notes: -- restart algorithm above is applied to the UPDATE, DELETE, SELECT WITH LOCK and MERGE - statements, with and without RETURNING clause, executing directly by user applicaiton or - as a part of some PSQL object (stored procedure\function, trigger, EXECUTE BLOCK, etc) -- if UPDATE\DELETE statement is positioned on some explicit cursor (WHERE CURRENT OF) then - engine skip step (c) above, i.e. not fetches and not put write locks on remaining records - of cursor -- if top-level statement is SELECT'able and update conflict happens after one or more records - was returned to the application, then update conflict error is reported as usual and restart - is not initiated -- restart is not initiated for statements in autonomous blocks -- after 10 attempts engine stop restarts and report update conflict -- by historical reasons isc_update_conflict reported as secondary error code with primary - error code isc_deadlock. +- restart algorithm above is applied to the UPDATE, DELETE, SELECT WITH LOCK and MERGE statements, + with and without RETURNING clause, executing directly by user applicaiton or as a part of some + PSQL object (stored procedure\function, trigger, EXECUTE BLOCK, etc) +- if UPDATE\DELETE statement is positioned on some explicit cursor (WHERE CURRENT OF) then engine + skip step (c) above, i.e. not fetches and not put write locks on remaining records of cursor +- if top-level statement is SELECT'able and update conflict happens after one or more records was + returned to the application, then update conflict error is reported as usual and restart is not + initiated +- restart is not initiated for statements in autonomous blocks (IN AUTONOMOUS TRANSACTION DO ...) +- after 10 attempts engine aborts restart algorithm, releases all write locks, restores transaction + isolation mode as READ COMMITTED *READ CONSISTENCY* and report update conflict +- any not handled error at step (c) above stops restart algorithm and engine continue processing + in usual way, for example error could be catched and handled by PSQL WHEN block or reported to + the application if not handled +- UPDATE\DELETE triggers will fire multiply times for the same record if statement execution was + restarted and record is updated\deleted again +- by historical reasons isc_update_conflict reported as secondary error code with primary error + code isc_deadlock. ### Precommitted transactions -*Read-committed read only* transactions marked as committed immediately when transaction started. +READ COMMITTED READ ONLY transactions marked as committed immediately when transaction started. This is correct if read-committed transaction needs no database snapshot. But it is not correct -to mark *read consistency read only* transaction as committed as it can break statement-level -snapshot consistency. Therefore *read consistency read only* transactions is not precommitted -on start. Other kinds of read committed read only transactions (*[no] record version*) works as +to mark READ CONSISTENCY READ ONLY transaction as committed as it can break statement-level +snapshot consistency. Therefore READ CONSISTENCY READ ONLY transactions is not precommitted +on start. Other kinds of READ COMMITTED READ ONLY transactions ([NO] RECORD VERSION) works as before and marked as committed when such transaction started. ### Support for new READ COMMITTED READ CONSISTENCY isolation level @@ -223,15 +228,16 @@ value of *oldest active snapshot* which could see *given* record version. If few in a chain got the same mark then all of them after the first one could be removed. This allows to keep versions chains short. -To make it works, engine maintains list of all active database snapshots. This list is kept in shared +To make it work, engine maintains list of all active database snapshots. This list is kept in shared memory. The initial size of shared memory block could be set in firebird.conf using new setting **SnapshotsMemSize**. Default value is 64KB. It could grow automatically, when necessary. -When engine need to find "*oldest active snapshot* which could see *given* record version" it -just search for CN of transaction that created given record version at the sorted array of active +When engine needs to find "*oldest active snapshot* which could see *given* record version" it just +searches for CN of transaction that created given record version in the sorted array of active snapshots. Garbage collection of intermediate record versions run by: - sweep - background garbage collector in SuperServer - every user attachment after update or delete record + - table scan at index creation