Refactor relation protection routines into a class.

Alex, please review changes related with DFW_reset_icu.

src/jrd/dfw.epp

@@ -349,6 +349,92 @@ public:
 	DeferredJob() : work(NULL), end(&work) { }
 };
 
+
+// Lock relation with protected_read level or raise existing relation lock
+// to this level to ensure nobody can write to this relation.
+// Used when new index is built.
+// releaseLock set to true if there was no existing lock before
+class ProtectRelations
+{
+public:
+	ProtectRelations(thread_db* tdbb, jrd_tra* transaction) :
+		m_tdbb(tdbb),
+		m_transaction(transaction),
+		m_locks()
+	{
+	}
+
+	ProtectRelations(thread_db* tdbb, jrd_tra* transaction, jrd_rel* relation) :
+		m_tdbb(tdbb),
+		m_transaction(transaction),
+		m_locks()
+	{
+		addRelation(relation);
+		lock();
+	}
+
+	~ProtectRelations()
+	{
+		unlock();
+	}
+
+	void addRelation(jrd_rel* relation)
+	{
+		FB_SIZE_T pos;
+		if (!m_locks.find(relation->rel_id, pos))
+			m_locks.insert(pos, relLock(relation));
+	}
+
+	bool exists(USHORT rel_id) const 
+	{
+		FB_SIZE_T pos;
+		return m_locks.find(rel_id, pos);
+	}
+
+	void lock()
+	{
+		relLock* item = m_locks.begin();
+		const relLock* const end = m_locks.end();
+		for (; item < end; item++)
+			item->takeLock(m_tdbb, m_transaction);
+	}
+
+	void unlock()
+	{
+		relLock* item = m_locks.begin();
+		const relLock* const end = m_locks.end();
+		for (; item < end; item++)
+			item->releaseLock(m_tdbb, m_transaction);
+	}
+
+private:
+	struct relLock
+	{
+		relLock(jrd_rel* relation = NULL) :
+			m_relation(relation),
+			m_lock(NULL),
+			m_release(false)
+		{
+		}
+
+		void takeLock(thread_db* tdbb, jrd_tra* transaction);
+		void releaseLock(thread_db* tdbb, jrd_tra* transaction);
+
+		static const USHORT generate(const relLock& item)
+		{
+			return item.m_relation->rel_id;
+		}
+
+		jrd_rel* m_relation;
+		Lock* m_lock;
+		bool m_release;
+	};
+
+	thread_db* m_tdbb;
+	jrd_tra* m_transaction;
+	SortedArray<relLock, InlineStorage<relLock, 2>, USHORT, relLock> m_locks;
+};
+
 } // namespace Jrd
 
 /*==================================================================
@@ -420,8 +506,6 @@ static blb*	setup_triggers(thread_db*, jrd_rel*, bool, trig_vec**, blb*);
 static void	setup_trigger_details(thread_db*, jrd_rel*, blb*, trig_vec**, const TEXT*, bool);
 static bool validate_text_type (thread_db*, const TemporaryField*);
 
-static Lock* protect_relation(thread_db*, jrd_tra*, jrd_rel*, bool&);
-static void release_protect_lock(thread_db*, jrd_tra*, Lock*);
 static void check_partners(thread_db*, const USHORT);
 static string get_string(const dsc* desc);
 static void setupSpecificCollationAttributes(thread_db*, jrd_tra*, const USHORT, const char*);
@@ -517,6 +601,54 @@ static void raiseTooManyVersionsError(const int obj_type, const string& obj_name
 			 Arg::Gds(isc_version_err));
 }
 
+void Jrd::ProtectRelations::relLock::takeLock(thread_db* tdbb, jrd_tra* transaction)
+{
+	m_lock = RLCK_transaction_relation_lock(tdbb, transaction, m_relation);
+
+	m_release = (m_lock->lck_logical == LCK_none);
+
+	bool inUse = false;
+
+	if (!m_release)
+	{
+		if ((m_lock->lck_logical < LCK_PR) &&
+			!LCK_convert(tdbb, m_lock, LCK_PR, transaction->getLockWait()))
+		{
+			inUse = true;
+		}
+	}
+	else
+	{
+		if (!LCK_lock(tdbb, m_lock, LCK_PR, transaction->getLockWait()))
+			inUse = true;
+	}
+
+	if (inUse)
+		raiseRelationInUseError(m_relation);
+}
+
+
+void Jrd::ProtectRelations::relLock::releaseLock(thread_db* tdbb, jrd_tra* transaction)
+{
+	if (!m_release)
+		return;
+
+	vec<Lock*>* vector = transaction->tra_relation_locks;
+	if (vector)
+	{
+		vec<Lock*>::iterator lock = vector->begin();
+		for (ULONG i = 0; i < vector->count(); ++i, ++lock)
+		{
+			if (*lock == m_lock)
+			{
+				LCK_release(tdbb, m_lock);
+				*lock = 0;
+				break;
+			}
+		}
+	}
+}
+
 static const UCHAR nonnull_validation_blr[] =
 {
 	blr_version5,
@@ -2150,8 +2282,6 @@ static bool check_not_null(thread_db* tdbb, SSHORT phase, DeferredWork* work, jr
 	SET_TDBB(tdbb);
 
 	Jrd::Attachment* attachment = tdbb->getAttachment();
-	Lock* relationLock = NULL;
-	bool releaseRelationLock = false;
 
 	switch (phase)
 	{
@@ -2160,14 +2290,13 @@ static bool check_not_null(thread_db* tdbb, SSHORT phase, DeferredWork* work, jr
 		return true;
 
 	case 3:
-		try
 		{
 			jrd_rel* relation = MET_lookup_relation(tdbb, work->dfw_name);
 			if (!relation || relation->rel_view_rse || work->dfw_ids.isEmpty())
 				break;
 
 			// Protect relation from modification
-			relationLock = protect_relation(tdbb, transaction, relation, releaseRelationLock);
+			ProtectRelations protectRelation(tdbb, transaction, relation);
 
 			SortedArray<int> fields;
 			AutoRequest handle;
@@ -2313,16 +2442,6 @@ static bool check_not_null(thread_db* tdbb, SSHORT phase, DeferredWork* work, jr
 				if (hasError)
 					ERR_post(errs);
 			}
-
-			if (relationLock && releaseRelationLock)
-				release_protect_lock(tdbb, transaction, relationLock);
-		}
-		catch (const Exception&)
-		{
-			if (relationLock && releaseRelationLock)
-				release_protect_lock(tdbb, transaction, relationLock);
-
-			throw;
 		}
 
 		break;
@@ -2599,8 +2718,7 @@ static bool create_expression_index(thread_db* tdbb, SSHORT phase, DeferredWork*
 		// Actually create the index
 
 		// Protect relation from modification to create consistent index
-		bool releaseRelationLock = false;
+		ProtectRelations protectRelation(tdbb, transaction, relation);
-		Lock* relationLock = protect_relation(tdbb, transaction, relation, releaseRelationLock);
 
 		SelectivityList selectivity(*tdbb->getDefaultPool());
 
@@ -2615,19 +2733,11 @@ static bool create_expression_index(thread_db* tdbb, SSHORT phase, DeferredWork*
 				transaction, selectivity);
 
 			fb_assert(work->dfw_id == idx.idx_id);
-
-			if (relationLock && releaseRelationLock) {
-				release_protect_lock(tdbb, transaction, relationLock);
-			}
 		}
 		catch (const Exception&)
 		{
 			tdbb->setTransaction(current_transaction);
 			tdbb->setRequest(current_request);
-
-			if (relationLock && releaseRelationLock) {
-				release_protect_lock(tdbb, transaction, relationLock);
-			}
 			throw;
 		}
 
@@ -3313,103 +3423,86 @@ static bool create_index(thread_db* tdbb, SSHORT phase, DeferredWork* work, jrd_
 
 		// Actually create the index
 
-		Lock *relationLock = NULL, *partnerLock = NULL;
-		bool releaseRelationLock = false, releasePartnerLock = false;
 		partner_relation = NULL;
-		try
+
+		// Protect relation from modification to create consistent index
+		ProtectRelations protectRelations(tdbb, transaction);
+		protectRelations.addRelation(relation);
+
+		if (idx.idx_flags & idx_foreign)
 		{
-			// Protect relation from modification to create consistent index
+			idx.idx_id = idx_invalid;
-			relationLock = protect_relation(tdbb, transaction, relation, releaseRelationLock);
 
-			if (idx.idx_flags & idx_foreign)
+			if (MET_lookup_partner(tdbb, relation, &idx, work->dfw_name.c_str()))
 			{
-				idx.idx_id = idx_invalid;
+				partner_relation = MET_lookup_relation_id(tdbb, idx.idx_primary_relation, true);
+			}
 
-				if (MET_lookup_partner(tdbb, relation, &idx, work->dfw_name.c_str()))
+			if (!partner_relation)
-				{
+			{
-					partner_relation = MET_lookup_relation_id(tdbb, idx.idx_primary_relation, true);
+				Firebird::MetaName constraint_name;
-				}
+				MET_lookup_cnstrt_for_index(tdbb, constraint_name, work->dfw_name);
+				ERR_post(Arg::Gds(isc_partner_idx_not_found) << Arg::Str(constraint_name));
+			}
 
-				if (!partner_relation)
+			// Get an protected_read lock on the both relations if the index being
-				{
+			// defined enforces a foreign key constraint. This will prevent
-					Firebird::MetaName constraint_name;
+			// the constraint from being violated during index construction.
-					MET_lookup_cnstrt_for_index(tdbb, constraint_name, work->dfw_name);
-					ERR_post(Arg::Gds(isc_partner_idx_not_found) << Arg::Str(constraint_name));
-				}
 
-				// Get an protected_read lock on the both relations if the index being
+			protectRelations.addRelation(partner_relation);
-				// defined enforces a foreign key constraint. This will prevent
-				// the constraint from being violated during index construction.
 
-				partnerLock = protect_relation(tdbb, transaction, partner_relation, releasePartnerLock);
+			int bad_segment;
-
+			if (!IDX_check_master_types(tdbb, idx, partner_relation, bad_segment))
-				int bad_segment;
+			{
-				if (!IDX_check_master_types(tdbb, idx, partner_relation, bad_segment))
+				ERR_post(Arg::Gds(isc_no_meta_update) <<
-				{
+							Arg::Gds(isc_partner_idx_incompat_type) << Arg::Num(bad_segment + 1));
-					ERR_post(Arg::Gds(isc_no_meta_update) <<
+			}
-							 Arg::Gds(isc_partner_idx_incompat_type) << Arg::Num(bad_segment + 1));
-				}
 
 /* hvlad: this code was never called but i preserve it for Claudio review and decision
 
-				// CVC: Currently, the server doesn't enforce FK creation more than at DYN level.
+			// CVC: Currently, the server doesn't enforce FK creation more than at DYN level.
-				// If DYN is bypassed, then FK creation succeeds and operation will fail at run-time.
+			// If DYN is bypassed, then FK creation succeeds and operation will fail at run-time.
-				// The aim is to check REFERENCES at DDL time instead of DML time and behave accordingly
+			// The aim is to check REFERENCES at DDL time instead of DML time and behave accordingly
-				// to ANSI SQL rules for REFERENCES rights.
+			// to ANSI SQL rules for REFERENCES rights.
-				// For testing purposes, I'm calling SCL_check_index, although most of the DFW ops are
+			// For testing purposes, I'm calling SCL_check_index, although most of the DFW ops are
-				// carried using internal metadata structures that are refreshed from system tables.
+			// carried using internal metadata structures that are refreshed from system tables.
 
-				// Don't bother if the master's owner is the same than the detail's owner.
+			// Don't bother if the master's owner is the same than the detail's owner.
-				// If both tables aren't defined in the same session, partner_relation->rel_owner_name
+			// If both tables aren't defined in the same session, partner_relation->rel_owner_name
-				// won't be loaded hence, we need to be careful about null pointers.
+			// won't be loaded hence, we need to be careful about null pointers.
 
-				if (relation->rel_owner_name.length() == 0 ||
+			if (relation->rel_owner_name.length() == 0 ||
-					partner_relation->rel_owner_name.length() == 0 ||
+				partner_relation->rel_owner_name.length() == 0 ||
-					relation->rel_owner_name != partner_relation->rel_owner_name)
+				relation->rel_owner_name != partner_relation->rel_owner_name)
-				{
-					SCL_check_index(tdbb, partner_relation->rel_name,
-									idx.idx_id + 1, SCL_references);
-				}
-*/
-			}
-
-			fb_assert(work->dfw_id <= dbb->dbb_max_idx);
-			idx.idx_id = work->dfw_id;
-			SelectivityList selectivity(*tdbb->getDefaultPool());
-			IDX_create_index(tdbb, relation, &idx, work->dfw_name.c_str(),
-							&work->dfw_id, transaction, selectivity);
-			fb_assert(work->dfw_id == idx.idx_id);
-			DFW_update_index(work->dfw_name.c_str(), idx.idx_id, selectivity, transaction);
-
-			if (partner_relation)
 			{
-				// signal to other processes about new constraint
+				SCL_check_index(tdbb, partner_relation->rel_name,
-				relation->rel_flags |= REL_check_partners;
+								idx.idx_id + 1, SCL_references);
-				LCK_lock(tdbb, relation->rel_partners_lock, LCK_EX, LCK_WAIT);
-				LCK_release(tdbb, relation->rel_partners_lock);
-
-				if (relation != partner_relation) {
-					partner_relation->rel_flags |= REL_check_partners;
-					LCK_lock(tdbb, partner_relation->rel_partners_lock, LCK_EX, LCK_WAIT);
-					LCK_release(tdbb, partner_relation->rel_partners_lock);
-				}
-			}
-			if (relationLock && releaseRelationLock) {
-				release_protect_lock(tdbb, transaction, relationLock);
-			}
-			if (partnerLock && releasePartnerLock) {
-				release_protect_lock(tdbb, transaction, partnerLock);
 			}
+*/
 		}
-		catch (const Firebird::Exception&)
+
+		protectRelations.lock();
+
+		fb_assert(work->dfw_id <= dbb->dbb_max_idx);
+		idx.idx_id = work->dfw_id;
+		SelectivityList selectivity(*tdbb->getDefaultPool());
+		IDX_create_index(tdbb, relation, &idx, work->dfw_name.c_str(),
+						&work->dfw_id, transaction, selectivity);
+		fb_assert(work->dfw_id == idx.idx_id);
+		DFW_update_index(work->dfw_name.c_str(), idx.idx_id, selectivity, transaction);
+
+		if (partner_relation)
 		{
-			if (relationLock && releaseRelationLock) {
+			// signal to other processes about new constraint
-				release_protect_lock(tdbb, transaction, relationLock);
+			relation->rel_flags |= REL_check_partners;
+			LCK_lock(tdbb, relation->rel_partners_lock, LCK_EX, LCK_WAIT);
+			LCK_release(tdbb, relation->rel_partners_lock);
+
+			if (relation != partner_relation) {
+				partner_relation->rel_flags |= REL_check_partners;
+				LCK_lock(tdbb, partner_relation->rel_partners_lock, LCK_EX, LCK_WAIT);
+				LCK_release(tdbb, partner_relation->rel_partners_lock);
 			}
-			if (partnerLock && releasePartnerLock) {
-				release_protect_lock(tdbb, transaction, partnerLock);
-			}
-			throw;
 		}
 
 		break;
@@ -3728,18 +3821,6 @@ static bool create_collation(thread_db* tdbb, SSHORT phase, DeferredWork* work,
 }
 
 
-namespace {
-
-struct TableLock
-{
-	jrd_rel* relation;
-	Lock* lock;
-	static const MetaName& generate(const TableLock& item) { return item.relation->rel_name; }
-};
-
-}
-
-
 void DFW_reset_icu(thread_db* tdbb)
 {
 /**************************************
@@ -3759,7 +3840,6 @@ void DFW_reset_icu(thread_db* tdbb)
 
 	jrd_tra* transaction = NULL;
 	jrd_tra* oldTransaction = tdbb->getTransaction();
-	SortedArray<TableLock, EmptyStorage<TableLock>, MetaName, TableLock> tables;
 
 	try
 	{
@@ -3767,12 +3847,16 @@ void DFW_reset_icu(thread_db* tdbb)
 		transaction = TRA_start(tdbb, 0, 0);
 		tdbb->setTransaction(transaction);
 
+		SortedArray<Firebird::MetaName> indices;
+		ProtectRelations tables(tdbb, transaction);
+
 		// Get list of affected indices & tables
 		const char* indSql =
-			"select ind.RDB$INDEX_NAME, ind.RDB$RELATION_NAME,"
+			"select ind.RDB$INDEX_NAME, rel.RDB$RELATION_ID,"
 			" coalesce(coll.RDB$BASE_COLLATION_NAME, coll.RDB$COLLATION_NAME), cs.RDB$CHARACTER_SET_NAME, "
 			" coll.RDB$SPECIFIC_ATTRIBUTES "
 			"from RDB$INDICES ind "
+			"join RDB$RELATIONS rel on ind.RDB$RELATION_NAME = rel.RDB$RELATION_NAME "
 			"join RDB$INDEX_SEGMENTS seg on ind.RDB$INDEX_NAME = seg.RDB$INDEX_NAME "
 			"join RDB$RELATION_FIELDS rfl on rfl.RDB$RELATION_NAME = ind.RDB$RELATION_NAME "
 			" and rfl.RDB$FIELD_NAME = seg.RDB$FIELD_NAME "
@@ -3782,9 +3866,9 @@ void DFW_reset_icu(thread_db* tdbb)
 			"join RDB$CHARACTER_SETS cs on coll.RDB$CHARACTER_SET_ID = cs.RDB$CHARACTER_SET_ID "
 			"where coll.RDB$SPECIFIC_ATTRIBUTES like '%COLL-VERSION=%' "
 			" and coalesce(ind.RDB$INDEX_INACTIVE, 0) = 0 "
-			"group by ind.RDB$INDEX_NAME, ind.RDB$RELATION_NAME, coll.RDB$BASE_COLLATION_NAME, "
+			"group by ind.RDB$INDEX_NAME, rel.RDB$RELATION_ID, coll.RDB$BASE_COLLATION_NAME, "
 			" coll.RDB$COLLATION_NAME, cs.RDB$CHARACTER_SET_NAME, coll.RDB$SPECIFIC_ATTRIBUTES";
-		SortedArray<Firebird::MetaName> indices;
+
 		{	// scope
 			AutoPreparedStatement ps(attachment->prepareStatement(tdbb, transaction, indSql));
 			AutoResultSet rs(ps->executeQuery(tdbb, transaction));
@@ -3807,26 +3891,18 @@ void DFW_reset_icu(thread_db* tdbb)
 				if (!indices.exist(t))
 					indices.add(rs->getMetaName(tdbb, 1));
 
-				t = rs->getMetaName(tdbb, 2);
+				USHORT rel_id = rs->getInt(tdbb, 2);
-				if (!tables.exist(t))
+				if (!tables.exists(rel_id))
 				{
-					TableLock lock;
+					jrd_rel* relation = MET_lookup_relation_id(tdbb, rel_id, false);
-					lock.relation = MET_lookup_relation(tdbb, t);
+					if (relation)
-					fb_assert(lock.relation);
+						tables.addRelation(relation);
-					lock.lock = NULL;
-					tables.add(lock);
 				}
 			}
 		}
 
 		// Lock the tables
-		for (unsigned n = 0; n < tables.getCount(); ++n)
+		tables.lock();
-		{
-			bool releaseLock;
-			Lock* lock = protect_relation(tdbb, transaction, tables[n].relation, releaseLock);
-			if (releaseLock)
-				tables[n].lock = lock;
-		}
 
 		// Change collation's attributes
 		const char* collSql =
@@ -6048,47 +6124,6 @@ static void put_summary_blob(thread_db* tdbb, blb* blob, rsr_t type, bid* blob_i
 }
 
 
-static Lock* protect_relation(thread_db* tdbb, jrd_tra* transaction, jrd_rel* relation,
-	bool& releaseLock)
-{
-/**************************************
- *
- * p r o t e c t _ r e l a t i o n
- *
- **************************************
- *
- * Functional description
- *	Lock relation with protected_read level or raise existing relation lock
- * to this level to ensure nobody can write to this relation.
- * Used when new index is built.
- * releaseLock set to true if there was no existing lock before
- *
- **************************************/
-	Lock* relLock = RLCK_transaction_relation_lock(tdbb, transaction, relation);
-
-	releaseLock = (relLock->lck_logical == LCK_none);
-
-	bool inUse = false;
-
-	if (!releaseLock) {
-		if ( (relLock->lck_logical < LCK_PR) &&
-			!LCK_convert(tdbb, relLock, LCK_PR, transaction->getLockWait()) )
-		{
-			inUse = true;
-		}
-	}
-	else {
-		if (!LCK_lock(tdbb, relLock, LCK_PR, transaction->getLockWait()))
-			inUse = true;
-	}
-
-	if (inUse)
-		raiseRelationInUseError(relation);
-
-	return relLock;
-}
-
-
 static void put_summary_record(thread_db* tdbb,
 							   blb*		blob,
 							   rsr_t	type,
@@ -6133,24 +6168,6 @@ static void put_summary_record(thread_db* tdbb,
 }
 
 
-static void release_protect_lock(thread_db* tdbb, jrd_tra*	transaction, Lock* relLock)
-{
-	vec<Lock*>* vector = transaction->tra_relation_locks;
-	if (vector) {
-		vec<Lock*>::iterator lock = vector->begin();
-		for (ULONG i = 0; i < vector->count(); ++i, ++lock)
-		{
-			if (*lock == relLock)
-			{
-				LCK_release(tdbb, relLock);
-				*lock = 0;
-				break;
-			}
-		}
-	}
-}
-
-
 static bool scan_relation(thread_db* tdbb, SSHORT phase, DeferredWork* work, jrd_tra*)
 {
 /**************************************