Backport #7747: Fix an issue where the garbage collection in indexes …

…and blobs is not performed in VIO_backout

The issue starts in VIO_record. There is a call record->reset(format) which clears the REC_gc_active flag by mistake. After that VIO_gc_record reuses the record which is already active.

src/jrd/Record.h

@@ -29,55 +29,49 @@
 
 namespace Jrd
 {
-	// Record flags
-	const UCHAR REC_fake_nulls	= 1;	// all fields simulate being NULLs
-	const UCHAR REC_gc_active	= 2;	// relation garbage collect record block in use
-	const UCHAR REC_undo_active	= 4;	// record block in use for undo purposes
-
 	class Record
 	{
-		template <UCHAR FLAG> friend class AutoTempRecord;
+		friend class AutoTempRecord;
 
 	public:
-		Record(MemoryPool& p, const Format* format, UCHAR flags = 0)
-			: m_precedence(p), m_data(p)
+		Record(MemoryPool& p, const Format* format, const bool temp_active = false)
+			: m_precedence(p), m_data(p), m_fake_nulls(false), m_temp_active(temp_active)
 		{
 			m_data.resize(format->fmt_length);
 			m_format = format;
-			m_flags = flags;
 		}
 
 		Record(MemoryPool& p, const Record* other)
 			: m_precedence(p), m_data(p, other->m_data),
-			  m_format(other->m_format), m_flags(other->m_flags)
+			  m_format(other->m_format), m_fake_nulls(other->m_fake_nulls), m_temp_active(false)
 		{}
 
-		void reset(const Format* format = NULL, UCHAR flags = 0)
+		void reset(const Format* format = NULL)
 		{
 			if (format && format != m_format)
 			{
 				m_data.resize(format->fmt_length);
 				m_format = format;
 			}
 
-			m_flags = flags;
+			m_fake_nulls = false;
 		}
 
 		void setNull(USHORT id)
 		{
-			fb_assert(!(m_flags & REC_fake_nulls));
+			fb_assert(!m_fake_nulls);
 			getData()[id >> 3] |= (1 << (id & 7));
 		}
 
 		void clearNull(USHORT id)
 		{
-			fb_assert(!(m_flags & REC_fake_nulls));
+			fb_assert(!m_fake_nulls);
 			getData()[id >> 3] &= ~(1 << (id & 7));
 		}
 
 		bool isNull(USHORT id) const
 		{
-			if (m_flags & REC_fake_nulls)
+			if (m_fake_nulls)
 				return true;
 
 			return ((getData()[id >> 3] & (1 << (id & 7))) != 0);
@@ -91,7 +85,7 @@ namespace Jrd
 			memset(getData() + null_bytes, 0, getLength() - null_bytes);
 
 			// Record has real NULLs now, so clear the "fake-nulls" flag
-			m_flags &= ~REC_fake_nulls;
+			m_fake_nulls = false;
 		}
 
 		PageStack& getPrecedence()
@@ -107,7 +101,7 @@ namespace Jrd
 		void copyFrom(const Record* other)
 		{
 			m_format = other->m_format;
-			m_flags = other->m_flags;
+			m_fake_nulls = other->m_fake_nulls;
 
 			copyDataFrom(other);
 		}
@@ -137,12 +131,12 @@ namespace Jrd
 
 		void fakeNulls()
 		{
-			m_flags |= REC_fake_nulls;
+			m_fake_nulls = true;
 		}
 
 		bool isNull() const
 		{
-			return (m_flags & REC_fake_nulls);
+			return m_fake_nulls;
 		}
 
 		ULONG getLength() const
@@ -160,9 +154,14 @@ namespace Jrd
 			return m_data.begin();
 		}
 
-		bool testFlags(UCHAR mask) const
+		bool isTempActive() const
 		{
-			return ((m_flags & mask) != 0);
+			return m_temp_active;
+		}
+
+		void setTempActive()
+		{
+			m_temp_active = true;
 		}
 
 		TraNumber getTransactionNumber() const
@@ -179,21 +178,21 @@ namespace Jrd
 		PageStack m_precedence;			// stack of higher precedence pages/transactions
 		Firebird::Array<UCHAR> m_data;	// space for record data
 		const Format* m_format;			// what the data looks like
-		UCHAR m_flags;					// misc record flags
-		TraNumber m_transaction_nr;   // transaction number for a record
+		TraNumber m_transaction_nr;		// transaction number for a record
+		bool m_fake_nulls;				// all fields simulate being NULLs
+		bool m_temp_active;				// record block in use for garbage collection or undo purposes
 	};
 
 	// Wrapper for reusable temporary records
 
-	template <UCHAR FLAG>
 	class AutoTempRecord
 	{
 	public:
 		explicit AutoTempRecord(Record* record = NULL)
 			: m_record(record)
 		{
 			// validate record and its flag
-			fb_assert(!record || (record->m_flags & FLAG));
+			fb_assert(!record || record->m_temp_active);
 		}
 
 		~AutoTempRecord()
@@ -206,7 +205,7 @@ namespace Jrd
 			// class object can be initialized just once
 			fb_assert(!m_record);
 			// validate record and its flag
-			fb_assert(!record || (record->m_flags & FLAG));
+			fb_assert(!record || record->m_temp_active);
 
 			m_record = record;
 			return m_record;
@@ -216,7 +215,8 @@ namespace Jrd
 		{
 			if (m_record)
 			{
-				m_record->m_flags &= ~FLAG;
+				fb_assert(m_record->m_temp_active);
+				m_record->m_temp_active = false;
 				m_record = NULL;
 			}
 		}
@@ -234,9 +234,6 @@ namespace Jrd
 	private:
 		Record* m_record;
 	};
-
-	typedef AutoTempRecord<REC_gc_active> AutoGCRecord;
-	typedef AutoTempRecord<REC_undo_active> AutoUndoRecord;
 } // namespace
 
 #endif // JRD_RECORD_H

src/jrd/Savepoint.cpp

@@ -86,7 +86,7 @@ void VerbAction::garbageCollectIdxLite(thread_db* tdbb, jrd_tra* transaction, SI
 	rpb.rpb_transaction_nr = transaction->tra_number;
 
 	Record* next_ver;
-	AutoUndoRecord undo_next_ver(transaction->findNextUndo(this, vct_relation, recordNumber));
+	AutoTempRecord undo_next_ver(transaction->findNextUndo(this, vct_relation, recordNumber));
 	AutoPtr<Record> real_next_ver;
 
 	next_ver = undo_next_ver;
@@ -108,7 +108,7 @@ void VerbAction::garbageCollectIdxLite(thread_db* tdbb, jrd_tra* transaction, SI
 		BUGCHECK(185);	// msg 185 wrong record version
 
 	Record* prev_ver = NULL;
-	AutoUndoRecord undo_prev_ver;
+	AutoTempRecord undo_prev_ver;
 	AutoPtr<Record> real_prev_ver;
 
 	if (nextAction && nextAction->vct_undo && nextAction->vct_undo->locate(recordNumber))
@@ -197,7 +197,7 @@ void VerbAction::mergeTo(thread_db* tdbb, jrd_tra* transaction, VerbAction* next
 				// because going version for sure has all index entries successfully set up (in contrast with undo)
 				// we can use lightweigth version of garbage collection without collection of full staying list
 
-				AutoUndoRecord this_ver(item.setupRecord(transaction));
+				AutoTempRecord this_ver(item.setupRecord(transaction));
 
 				garbageCollectIdxLite(tdbb, transaction, recordNumber, nextAction, this_ver);
 
@@ -313,7 +313,7 @@ void VerbAction::undo(thread_db* tdbb, jrd_tra* transaction, bool preserveLocks,
 			}
 			else
 			{
-				AutoUndoRecord record(vct_undo->current().setupRecord(transaction));
+				AutoTempRecord record(vct_undo->current().setupRecord(transaction));
 
 				Record* const save_record = rpb.rpb_record;
 				record_param new_rpb = rpb;

src/jrd/idx.cpp

@@ -345,7 +345,7 @@ void IDX_create_index(thread_db* tdbb,
 
 	// Checkout a garbage collect record block for fetching data.
 
-	AutoGCRecord gc_record(VIO_gc_record(tdbb, relation));
+	AutoTempRecord gc_record(VIO_gc_record(tdbb, relation));
 
 	// Unless this is the only attachment or a database restore, worry about
 	// preserving the page working sets of other attachments.

src/jrd/tra.h

@@ -360,15 +360,16 @@ class jrd_tra : public pool_alloc<type_tra>
 			Record* const record = *iter;
 			fb_assert(record);
 
-			if (!record->testFlags(REC_undo_active))
+			if (!record->isTempActive())
 			{
 				// initialize record for reuse
-				record->reset(format, REC_undo_active);
+				record->reset(format);
+				record->setTempActive();
 				return record;
 			}
 		}
 
-		Record* const record = FB_NEW_POOL(*tra_pool) Record(*tra_pool, format, REC_undo_active);
+		Record* const record = FB_NEW_POOL(*tra_pool) Record(*tra_pool, format, true);
 		tra_undo_records.add(record);
 
 		return record;

src/jrd/vio.cpp

@@ -350,7 +350,7 @@ inline void clearRecordStack(RecordStack& stack)
 	{
 		Record* r = stack.pop();
 		// records from undo log must not be deleted
-		if (!r->testFlags(REC_undo_active))
+		if (!r->isTempActive())
 			delete r;
 	}
 }
@@ -452,8 +452,8 @@ void VIO_backout(thread_db* tdbb, record_param* rpb, const jrd_tra* transaction)
 	Record* data = NULL;
 	Record* old_data = NULL;
 
-	AutoGCRecord gc_rec1;
-	AutoGCRecord gc_rec2;
+	AutoTempRecord gc_rec1;
+	AutoTempRecord gc_rec2;
 
 	bool samePage;
 	bool deleted;
@@ -2429,18 +2429,19 @@ Record* VIO_gc_record(thread_db* tdbb, jrd_rel* relation)
 		Record* const record = *iter;
 		fb_assert(record);
 
-		if (!record->testFlags(REC_gc_active))
+		if (!record->isTempActive())
 		{
 			// initialize record for reuse
-			record->reset(format, REC_gc_active);
+			record->reset(format);
+			record->setTempActive();
 			return record;
 		}
 	}
 
 	// Allocate a garbage collect record block if all are active
 
 	Record* const record = FB_NEW_POOL(*relation->rel_pool)
-		Record(*relation->rel_pool, format, REC_gc_active);
+		Record(*relation->rel_pool, format, true);
 	relation->rel_gc_records.add(record);
 	return record;
 }
@@ -2845,7 +2846,7 @@ bool VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, j
 	if (org_rpb->rpb_runtime_flags & (RPB_refetch | RPB_undo_read))
 	{
 		const bool undo_read = (org_rpb->rpb_runtime_flags & RPB_undo_read);
-		AutoGCRecord old_record;
+		AutoTempRecord old_record;
 		if (undo_read)
 		{
 			old_record = VIO_gc_record(tdbb, relation);
@@ -5107,7 +5108,7 @@ static UndoDataRet get_undo_data(thread_db* tdbb, jrd_tra* transaction,
 		rpb->rpb_runtime_flags |= RPB_undo_data;
 		CCH_RELEASE(tdbb, &rpb->getWindow(tdbb));
 
-		AutoUndoRecord undoRecord(undo.setupRecord(transaction));
+		AutoTempRecord undoRecord(undo.setupRecord(transaction));
 
 		Record* const record = VIO_record(tdbb, rpb, undoRecord->getFormat(), pool);
 		record->copyFrom(undoRecord);
@@ -5990,7 +5991,7 @@ static void purge(thread_db* tdbb, record_param* rpb)
 	// the record.
 
 	record_param temp = *rpb;
-	AutoGCRecord gc_rec(VIO_gc_record(tdbb, relation));
+	AutoTempRecord gc_rec(VIO_gc_record(tdbb, relation));
 	Record* record = rpb->rpb_record = gc_rec;
 
 	VIO_data(tdbb, rpb, relation->rel_pool);
@@ -6339,7 +6340,7 @@ void VIO_update_in_place(thread_db* tdbb,
 	// becomes meaningless.  What we need to do is replace the old "delta" record
 	// with an old "complete" record, update in placement, then delete the old delta record
 
-	AutoGCRecord gc_rec;
+	AutoTempRecord gc_rec;
 
 	record_param temp2;
 	const Record* prior = org_rpb->rpb_prior;