From f1d60b9596e730885227c3760c5d91f3b70a5872 Mon Sep 17 00:00:00 2001 From: dimitr Date: Sat, 14 Feb 2015 07:34:58 +0000 Subject: [PATCH] Fixed CORE-4383: Index and BLOBs garbage collection doesn't work for update_in_place(). --- src/jrd/req.h | 1 + src/jrd/tra.h | 87 ++++++++++++++++++++++++++++++++++++++++--------- src/jrd/vio.cpp | 51 ++++++++++++++--------------- 3 files changed, 98 insertions(+), 41 deletions(-) diff --git a/src/jrd/req.h b/src/jrd/req.h index 42cd9b4230..1d920a2fea 100644 --- a/src/jrd/req.h +++ b/src/jrd/req.h @@ -164,6 +164,7 @@ public: const UCHAR REC_same_tx = 1; // record inserted/updated and deleted by same tx const UCHAR REC_gc_active = 2; // relation garbage collect record block in use const UCHAR REC_new_version = 4; // savepoint created new record version and deleted it +const UCHAR REC_undo_active = 8; // record block in use for undo purposes // save record_param block diff --git a/src/jrd/tra.h b/src/jrd/tra.h index 86eeb0c4db..dc5a773c57 100644 --- a/src/jrd/tra.h +++ b/src/jrd/tra.h @@ -102,6 +102,9 @@ const char* const TRA_UNDO_SPACE = "fb_undo_"; class jrd_tra : public pool_alloc { + static const size_t MAX_UNDO_RECORDS = 2; + typedef Firebird::HalfStaticArray UndoRecordList; + public: enum wait_t { tra_no_wait, @@ -129,7 +132,7 @@ public: tra_sorts(*p), tra_blob_space(NULL), tra_undo_space(NULL), - tra_undo_record(NULL), + tra_undo_records(*p), tra_user_management(NULL), tra_autonomous_pool(NULL), tra_autonomous_cnt(0) @@ -220,7 +223,7 @@ private: TempSpace* tra_blob_space; // temp blob storage TempSpace* tra_undo_space; // undo log storage - Record* tra_undo_record; // temporary record used for the undo purposes + UndoRecordList tra_undo_records; // temporary records used for the undo purposes UserManagement* tra_user_management; MemoryPool* tra_autonomous_pool; USHORT tra_autonomous_cnt; @@ -258,17 +261,47 @@ public: return tra_undo_space; } - Record* getUndoRecord(USHORT length) + Record* getUndoRecord(USHORT length, SINT64 number, const Format* format, UCHAR flags) { - if (!tra_undo_record || tra_undo_record->rec_length < length) + Record** recordPtr = NULL; + + for (Record** iter = tra_undo_records.begin(); iter != tra_undo_records.end(); ++iter) { - delete tra_undo_record; - tra_undo_record = FB_NEW_RPT(*tra_pool, length) Record(*tra_pool); + fb_assert(*iter); + + if (!((*iter)->rec_flags & REC_undo_active)) + { + recordPtr = iter; + break; + } } - memset(tra_undo_record, 0, sizeof(Record) + length); + Record* record = NULL; - return tra_undo_record; + if (recordPtr) + { + record = *recordPtr; + + if (record->rec_length < length) + { + delete record; + *recordPtr = record = FB_NEW_RPT(*tra_pool, length) Record(*tra_pool); + } + else + memset(record, 0, sizeof(Record) + length); + } + else + { + fb_assert(tra_undo_records.getCount() < MAX_UNDO_RECORDS); + record = FB_NEW_RPT(*tra_pool, length) Record(*tra_pool); + tra_undo_records.add(record); + } + + record->rec_number.setValue(number); + record->rec_length = length; + record->rec_format = format; + record->rec_flags = (flags | REC_undo_active); + return record; } UserManagement* getUserManagement(); @@ -466,16 +499,10 @@ public: { flags |= newFlags; - Record* const record = transaction->getUndoRecord(length); - record->rec_number.setValue(number); - record->rec_flags = flags; - record->rec_length = length; - record->rec_format = format; + Record* const record = transaction->getUndoRecord(length, number, format, flags); if (length) - { transaction->getUndoSpace()->read(offset, record->rec_data, length); - } return record; } @@ -526,6 +553,36 @@ inline void Savepoint::deleteActions(VerbAction* list) } }; +class AutoUndoRecord +{ +public: + AutoUndoRecord(jrd_tra* transaction, UndoItem* undo, USHORT flags = 0) + : m_record(undo ? undo->setupRecord(transaction, flags) : NULL) + {} + + ~AutoUndoRecord() + { + if (m_record) + { + fb_assert(m_record->rec_flags & REC_undo_active); + m_record->rec_flags &= ~REC_undo_active; + } + } + + operator Record*() + { + return m_record; + } + + Record* operator->() + { + return m_record; + } + +private: + Record* const m_record; +}; + } //namespace Jrd #endif // JRD_TRA_H diff --git a/src/jrd/vio.cpp b/src/jrd/vio.cpp index f5c88e3918..6cc88a73a0 100644 --- a/src/jrd/vio.cpp +++ b/src/jrd/vio.cpp @@ -3180,7 +3180,7 @@ void VIO_verb_cleanup(thread_db* tdbb, jrd_tra* transaction) } else { - Record* const record = action->vct_undo->current().setupRecord(transaction); + AutoUndoRecord record(transaction, &action->vct_undo->current()); const bool same_tx = (record->rec_flags & REC_same_tx) != 0; // Have we done BOTH an update and delete to this record @@ -3209,6 +3209,7 @@ void VIO_verb_cleanup(thread_db* tdbb, jrd_tra* transaction) VIO_backout(tdbb, &rpb, transaction); } } + if (record->rec_length != 0) { Record* dead_record = rpb.rpb_record; @@ -3256,7 +3257,8 @@ void VIO_verb_cleanup(thread_db* tdbb, jrd_tra* transaction) BUGCHECK(186); // msg 186 record disappeared } CCH_RELEASE(tdbb, &rpb.getWindow(tdbb)); - Record* const record = action->vct_undo->current().setupRecord(transaction); + + AutoUndoRecord record(transaction, &action->vct_undo->current()); const bool same_tx = (record->rec_flags & REC_same_tx) != 0; const bool new_ver = (record->rec_flags & REC_new_version) != 0; if (record->rec_length != 0) @@ -5354,19 +5356,17 @@ static void verb_post(thread_db* tdbb, VerbAction* action; for (action = transaction->tra_save_point->sav_verb_actions; action; action = action->vct_next) { - if (action->vct_relation == rpb->rpb_relation) { + if (action->vct_relation == rpb->rpb_relation) break; - } } if (!action) { - if ( (action = transaction->tra_save_point->sav_verb_free) ) { + if ( (action = transaction->tra_save_point->sav_verb_free) ) transaction->tra_save_point->sav_verb_free = action->vct_next; - } - else { + else action = FB_NEW(*tdbb->getDefaultPool()) VerbAction(); - } + action->vct_next = transaction->tra_save_point->sav_verb_actions; transaction->tra_save_point->sav_verb_actions = action; action->vct_relation = rpb->rpb_relation; @@ -5380,9 +5380,9 @@ static void verb_post(thread_db* tdbb, // An update-in-place is being posted to this savepoint, and this // savepoint hasn't seen this record before. - if (!action->vct_undo) { + if (!action->vct_undo) action->vct_undo = new UndoItemTree(tdbb->getDefaultPool()); - } + const UCHAR flags = same_tx ? REC_same_tx : 0; action->vct_undo->add(UndoItem(transaction, rpb->rpb_number, old_data, flags)); } @@ -5391,33 +5391,33 @@ static void verb_post(thread_db* tdbb, // An insert/update followed by a delete is posted to this savepoint, // and this savepoint hasn't seen this record before. - if (!action->vct_undo) { + if (!action->vct_undo) action->vct_undo = new UndoItemTree(tdbb->getDefaultPool()); - } + const UCHAR flags = REC_same_tx | (new_ver ? REC_new_version : 0); action->vct_undo->add(UndoItem(rpb->rpb_number, flags)); } } else if (same_tx) { - Record* undo = NULL; - if (action->vct_undo && action->vct_undo->locate(rpb->rpb_number.getValue())) - { - // An insert/update followed by a delete is posted to this savepoint, - // and this savepoint has already undo for this record. - undo = action->vct_undo->current().setupRecord(transaction, REC_same_tx); - } - else + const bool found = (action->vct_undo && action->vct_undo->locate(rpb->rpb_number.getValue())); + + // An insert/update followed by a delete is posted to this savepoint, + // and this savepoint has already undo for this record. + AutoUndoRecord undo(transaction, found ? &action->vct_undo->current() : NULL, REC_same_tx); + + if (!found) { // An insert/update followed by a delete is posted to this savepoint, // and this savepoint has seen this record before but it doesn't have undo data. - if (!action->vct_undo) { + if (!action->vct_undo) action->vct_undo = new UndoItemTree(tdbb->getDefaultPool()); - } + const UCHAR flags = REC_same_tx | REC_new_version; action->vct_undo->add(UndoItem(rpb->rpb_number, flags)); } + if (old_data) { // The passed old_data will not be used. Thus, garbage collect. @@ -5427,15 +5427,14 @@ static void verb_post(thread_db* tdbb, } else if (old_data) { + const bool found = (action->vct_undo && action->vct_undo->locate(rpb->rpb_number.getValue())); + // We are posting an update-in-place, but the current savepoint has // already undo data for this record. The old_data will not be used, // so make sure we garbage collect before we lose track of the // in-place-updated record. - Record* undo = NULL; - if (action->vct_undo && action->vct_undo->locate(rpb->rpb_number.getValue())) { - undo = action->vct_undo->current().setupRecord(transaction); - } + AutoUndoRecord undo(transaction, found ? &action->vct_undo->current() : NULL); garbage_collect_idx(tdbb, rpb, /*new_rpb,*/ old_data, undo); }