const-correctness when working with shared formats + better locking logic in getFormat()

Author: AlexPeshkoff

src/dsql/DdlNodes.epp

@@ -9156,7 +9156,7 @@ void RelationNode::getArrayDesc(thread_db* tdbb, const TEXT* field_name, Ods::In
 }
 
 
-Format* RelationNode::makeFormat(thread_db* tdbb, jrd_tra* transaction, Cached::Relation* relation,
+const Format* RelationNode::makeFormat(thread_db* tdbb, jrd_tra* transaction, Cached::Relation* relation,
 	USHORT* version, TemporaryField* stack)
 {
 /**************************************
@@ -9206,6 +9206,8 @@ Format* RelationNode::makeFormat(thread_db* tdbb, jrd_tra* transaction, Cached::
 		fb_assert(false);
 	}
 
+	// Single thread write access to formats in relation is guaranteed here by metadata cache
+
 	Format* format = Format::newFormat(relation->getPool(), count + 1);
 	format->fmt_version = version ? *version : 0;
 
@@ -9277,10 +9279,10 @@ Format* RelationNode::makeFormat(thread_db* tdbb, jrd_tra* transaction, Cached::
 
 	format->fmt_length = offset;
 
-	Format* old_format;
+	const Format* old_format;
 	if (!(relation->rel_flags & REL_temp_ltt) &&
 		format->fmt_version &&
-		(old_format = MET_format(tdbb, relation, (format->fmt_version - 1))) &&
+		(old_format = relation->getFormat(tdbb, (format->fmt_version - 1))) &&
 		(*old_format == *format))
 	{
 		delete format;

src/dsql/DdlNodes.h

@@ -1716,7 +1716,7 @@ class RelationNode : public DdlNode
 public:
 	static void makeVersion(thread_db* tdbb, jrd_tra* transaction, const QualifiedName& relName);
 	static void raiseTooManyVersionsError(const int obj_type, const QualifiedName& obj_name);
-	static Format* makeFormat(thread_db* tdbb, jrd_tra* transaction, Cached::Relation* relation,
+	static const Format* makeFormat(thread_db* tdbb, jrd_tra* transaction, Cached::Relation* relation,
 		USHORT* version, TemporaryField* stack);
 
 private:

src/jrd/ExtEngineManager.cpp

@@ -1808,7 +1808,7 @@ void ExtEngineManager::makeTrigger(thread_db* tdbb, CompilerScratch* csb, Jrd::T
 		MsgMetadata* fieldsMsg = FB_NEW MsgMetadata;
 		metadata->triggerFields = fieldsMsg;
 
-		Format* relFormat = relation->rel_current_format;
+		auto* relFormat = relation->rel_current_format;
 
 		for (FB_SIZE_T i = 0; i < relation->rel_fields->count(); ++i)
 		{

src/jrd/Relation.cpp

@@ -765,7 +765,7 @@ Lock* RelationPermanent::createLock(thread_db* tdbb, MemoryPool& pool, lck_t lck
 
 void RelationPermanent::addFormat(Format* fmt)
 {
-	MutexLockGuard g(rel_formats_grow, FB_FUNCTION);
+	MutexLockGuard guard(rel_formats_grow, FB_FUNCTION);
 
 	rel_formats.grow(fmt->fmt_version + 1, true);
 	rel_formats.writeAccessor()->value(fmt->fmt_version) = fmt;
@@ -1153,7 +1153,7 @@ const Format* jrd_rel::currentFormat(thread_db* tdbb)
 	if (!tdbb)
 		tdbb = JRD_get_thread_data();
 
-	rel_current_format = MET_format(tdbb, getPermanent(), rel_current_fmt);
+	rel_current_format = getPermanent()->getFormat(tdbb, rel_current_fmt);
 
 	return rel_current_format;
 }

src/jrd/Relation.h

@@ -645,7 +645,7 @@ class jrd_rel final : public ObjectBase
 
 public:
 	USHORT				rel_current_fmt;	// Current format number
-	Format*				rel_current_format;	// Current record format
+	const Format*		rel_current_format;	// Current record format
 	USHORT				rel_dbkey_length;	// RDB$DBKEY length
 
 	vec<jrd_fld*>*		rel_fields;			// vector of field blocks
@@ -984,7 +984,7 @@ class RelationPermanent : public Firebird::PermanentStorage
 	typedef SharedReadVector<Format*, 16> Formats;
 
 private:
-	SharedReadVector<Format*, 16> rel_formats;	// Known record formats
+	Formats rel_formats;				// Known record formats
 	Firebird::Mutex rel_formats_grow;	// Mutex to grow rel_formats
 
 public:
@@ -994,6 +994,7 @@ class RelationPermanent : public Firebird::PermanentStorage
 	}
 
 	void addFormat(Format* fmt);
+	const Format* getFormat(thread_db* tdbb, USHORT );
 
 	Indices			rel_indices;		// Active indices
 	QualifiedName	rel_name;			// ascii relation name

src/jrd/evl.cpp

@@ -426,7 +426,7 @@ bool EVL_field(jrd_rel* relation, Record* record, USHORT id, dsc* desc)
 					break;
 				}
 
-				format = MET_format(tdbb, relation->getPermanent(), format->fmt_version + 1);
+				format = relation->getPermanent()->getFormat(tdbb, format->fmt_version + 1);
 				fb_assert(format);
 			}
 

src/jrd/met.epp

@@ -949,7 +949,7 @@ void MET_error(const TEXT* string, ...)
 			 Arg::Gds(isc_random) << Arg::Str(s));
 }
 
-Format* MET_format(thread_db* tdbb, RelationPermanent* relation, USHORT number)
+const Format* RelationPermanent::getFormat(thread_db* tdbb, USHORT number)
 {
 /**************************************
  *
@@ -965,23 +965,30 @@ Format* MET_format(thread_db* tdbb, RelationPermanent* relation, USHORT number)
 	Attachment* attachment = tdbb->getAttachment();
 	Database* dbb = tdbb->getDatabase();
 
-	Format* format;
-	auto formats = relation->getFormats();
-	if ((number < formats->getCount()) && (format = formats->value(number)))
-	{
-		return format;
+	{ // scope
+		auto formats = getFormats();
+		if ((number < formats->getCount()) && formats->value(number))
+			return formats->value(number);
+	}
+
+	MutexLockGuard guard(rel_formats_grow, FB_FUNCTION);
+
+	{ // scope
+		auto formats = getFormats();
+		if ((number < formats->getCount()) && formats->value(number))
+			return formats->value(number);
 	}
 
-	// System relations don't have their formats stored inside RDB$FORMATS,
+	// System relations and LTT don't have their formats stored inside RDB$FORMATS,
 	// so it's absolutely pointless trying to find one there
-	fb_assert(!relation->isSystem());
-	fb_assert(!(relation->rel_flags & REL_temp_ltt));
+	fb_assert(!isSystem());
+	fb_assert(!(rel_flags & REL_temp_ltt));
 
-	format = NULL;
+	Format* format = NULL;
 	AutoCacheRequest request(tdbb, irq_r_format, IRQ_REQUESTS);
 
 	FOR(REQUEST_HANDLE request)
-		X IN RDB$FORMATS WITH X.RDB$RELATION_ID EQ relation->getId() AND
+		X IN RDB$FORMATS WITH X.RDB$RELATION_ID EQ getId() AND
 			X.RDB$FORMAT EQ number
 	{
 		blb* blob = blb::open(tdbb, attachment->getSysTransaction(), &X.RDB$DESCRIPTOR);
@@ -993,7 +1000,7 @@ Format* MET_format(thread_db* tdbb, RelationPermanent* relation, USHORT number)
 		unsigned bufferPos = 2;
 		USHORT count = buffer[0] | (buffer[1] << 8);
 
-		format = Format::newFormat(relation->getPool(), count);
+		format = Format::newFormat(getPool(), count);
 
 		Array<Ods::Descriptor> odsDescs;
 		Ods::Descriptor* odsDesc = odsDescs.getBuffer(count);
@@ -1025,20 +1032,20 @@ Format* MET_format(thread_db* tdbb, RelationPermanent* relation, USHORT number)
 
 			desc.dsc_address = tmpArray.getBuffer(desc.dsc_length, false);
 			memcpy(desc.dsc_address, p, desc.dsc_length);
-			EVL_make_value(tdbb, &desc, &format->fmt_defaults[offset], &relation->getPool());
+			EVL_make_value(tdbb, &desc, &format->fmt_defaults[offset], &getPool());
 
 			p += desc.dsc_length;
 		}
 	}
 	END_FOR
 
-	if (!format)
-		format = Format::newFormat(relation->getPool());
-
-	format->fmt_version = number;
+	if (format)
+	{
+		format->fmt_version = number;
 
-	// Link the format block into the world
-	relation->addFormat(format);
+		// Link the format block into the world
+		addFormat(format);
+	}
 
 	return format;
 }
@@ -3396,7 +3403,7 @@ ScanResult jrd_rel::scan(thread_db* tdbb, ObjectBase::Flag& flags)
 
 	delete csb;
 
-	rel_current_format = MET_format(tdbb, rel_perm, rel_current_fmt);
+	rel_current_format = rel_perm->getFormat(tdbb, rel_current_fmt);
 	dependencies = false;
 
 	if (rel_fields)

src/jrd/met_proto.h

@@ -90,7 +90,6 @@ Jrd::Cached::Relation*	MET_change_fields(Jrd::thread_db*, Jrd::jrd_tra*, const d
 void		MET_delete_dependencies(Jrd::thread_db*, const Jrd::QualifiedName&, int);
 void		MET_delete_shadow(Jrd::thread_db*, USHORT);
 void		MET_error(const TEXT*, ...);
-Jrd::Format*	MET_format(Jrd::thread_db*, Jrd::RelationPermanent*, USHORT);
 bool		MET_get_char_coll_subtype_info(Jrd::thread_db*, USHORT, Jrd::SubtypeInfo* info);
 Jrd::DmlNode*	MET_get_dependencies(Jrd::thread_db*, Jrd::jrd_rel*, const UCHAR*, const ULONG,
 								Jrd::CompilerScratch*, Jrd::bid*, Jrd::Statement**,

src/jrd/replication/Applier.cpp

@@ -1203,7 +1203,7 @@ const Format* Applier::findFormat(thread_db* tdbb, jrd_rel* relation, ULONG leng
 	auto format = relation->currentFormat(tdbb);
 
 	while (format->fmt_length != length && format->fmt_version)
-		format = MET_format(tdbb, relation->getPermanent(), format->fmt_version - 1);
+		format = relation->getPermanent()->getFormat(tdbb, format->fmt_version - 1);
 
 	if (format->fmt_length != length)
 	{

src/jrd/validation.cpp

@@ -2791,7 +2791,7 @@ Validation::RTN Validation::walk_record(jrd_rel* relation, const rhd* header, US
 		length -= RHD_SIZE;
 	}
 
-	const auto format = MET_format(vdr_tdbb, getPermanent(relation), header->rhd_format);
+	const auto format = relation->getPermanent()->getFormat(vdr_tdbb, header->rhd_format);
 	auto remainingLength = format->fmt_length;
 
 	auto calculateLength = [remainingLength](ULONG length, const UCHAR* data, bool notPacked)