Broken lock of statements cache during multithreaded index activation

[dmitry-starodubov]

Server (last 5.0) crashes when trying to dereference NULL pointer (self->lock) in DsqlStatementCache::blockingAst.
The crash stacks are in the attachment.

It can be reproduced in this way.
In attachment there is a backup of a database with 1000 empty tables and 4000 indexes.
Restore it if with "-i" option to leave the indexes inactive.
Set MaxParallelWorkers = 64 in firebird.conf.
Download the plume utility and build it.

Then run it as follows:
./plume -u sysdba -p masterkey -t 24 -P 4 -d localhost:/tmp/t5.fdb

I get the server crash after two - four runs.
stacks.txt
t5.zip

[dmitry-starodubov]

Hello. The crash on the artificial test is fixed, but when I try to activate the indexes on the real database, the server hangs with the stack in the attachment.
hang_stack.txt
According to the lock table the lock is acquired by thread 229152, but there is no such thread in the server process.
locks.txt

The server hangs after an error:

Warning trouble on index IDX_O_OTHER_GUID
COLLATION WIN1251_UNICODE for CHARACTER SET WIN1251 is not installed
-An ICU library with collation version 153.88 is required but was not found. You may try to install ICU version 63.1, used to register the collation in this database or look for 'gfix -icu' in Firebird documentation.

Perhaps, that thread have got this error and finished but did not release the lock.

[AlexPeshkoff]

On 11/14/22 14:52, Dmitry Starodubov wrote: According to the lock table the lock is acquired by thread 229152, but there is no such thread in the server process.

To make sure - our thread ID on linux is LWP's PID.

[asfernandes]

If you do not send another reproducible test case, things will be difficult.

I'm not sure we didn't have some fundamental problem in Lock class or its usage. Before my change, the usage pattern was almost identical to others cases (for example, att_repl_lock), except a possible memory release.

Tests for lck_logical seems weird when multiple threads may be using the same instance, even considering that the AST / other code has some protection with AsyncContextHolder.

Also its usage (for example, inside Attachment).

AFAIU we need to use some atomics for safety/correctness.

[dmitry-starodubov]

On 11/14/22 14:52, Dmitry Starodubov wrote: According to the lock table the lock is acquired by thread 229152, but there is no such thread in the server process.
To make sure - our thread ID on linux is LWP's PID.

Yes, I know. There are threads 229151 and 229153 in the server process, but 229152 is absent.

[dmitry-starodubov]

If you do not send another reproducible test case, things will be difficult.

I sent you an email with a link to download the database. The problem is reproduced in 1 or 2 runs of plume on it.

[asfernandes]

I sent you an email with a link to download the database. The problem is reproduced in 1 or 2 runs of plume on it.

Thanks. Investigating.

[dyemanov]

I'm unable to reproduce the freeze after applying the following patch:

diff --git a/src/dsql/DsqlStatementCache.cpp b/src/dsql/DsqlStatementCache.cpp
index e28c9282c4..e9901ee04a 100644
--- a/src/dsql/DsqlStatementCache.cpp
+++ b/src/dsql/DsqlStatementCache.cpp
@@ -141,10 +141,12 @@ void DsqlStatementCache::putStatement(thread_db* tdbb, const string& text, USHOR
 	activeStatementList.pushBack(std::move(newStatement));
 	map.put(key, --activeStatementList.end());
 
-	ensureLockIsCreated(tdbb);
-
-	if (lock->lck_logical == LCK_none)
+	if (missingLock)
+	{
+		ensureLockIsCreated(tdbb);
 		LCK_lock(tdbb, lock, LCK_SR, LCK_WAIT);
+		missingLock = false;
+	}
 
 #ifdef DSQL_STATEMENT_CACHE_DEBUG
 	dump();
@@ -189,8 +191,9 @@ void DsqlStatementCache::purge(thread_db* tdbb)
 
 	cacheSize = 0;
 
-	if (lock && lock->lck_logical != LCK_none)
+	if (lock)
 		LCK_release(tdbb, lock);
+	missingLock = true;
 }
 
 void DsqlStatementCache::purgeAllAttachments(thread_db* tdbb)
@@ -199,10 +202,12 @@ void DsqlStatementCache::purgeAllAttachments(thread_db* tdbb)
 
 	fb_assert(!lock || lock->lck_logical == LCK_none);
 
-	ensureLockIsCreated(tdbb);
+	Lock tempLock(tdbb, 0, LCK_dsql_statement_cache);
+
+	if (!LCK_lock(tdbb, &tempLock, LCK_EX, LCK_WAIT))	// notify others
+		status_exception::raise(tdbb->tdbb_status_vector);
 
-	LCK_lock(tdbb, lock, LCK_EX, LCK_WAIT);	// notify others
-	LCK_release(tdbb, lock);
+	LCK_release(tdbb, &tempLock);
 }
 
 void DsqlStatementCache::buildStatementKey(thread_db* tdbb, RefStrPtr& key, const string& text, USHORT clientDialect,
diff --git a/src/dsql/DsqlStatementCache.h b/src/dsql/DsqlStatementCache.h
index 2c70f11460..b8e6469031 100644
--- a/src/dsql/DsqlStatementCache.h
+++ b/src/dsql/DsqlStatementCache.h
@@ -127,6 +127,7 @@ private:
 	Firebird::DoublyLinkedList<StatementEntry> activeStatementList;
 	Firebird::DoublyLinkedList<StatementEntry> inactiveStatementList;
 	Firebird::AutoPtr<Lock> lock;
+	bool missingLock = true;
 	unsigned maxCacheSize = 0;
 	unsigned cacheSize = 0;
 };

[aafemt]

+	if (lock)
 		LCK_release(tdbb, lock);
+	missingLock = true;

IMHO, if (!missingLock) would be more consistent here but in any case I'm afraid it only decrease probability of thread races not eliminate them.

[dyemanov]

AFAIU we need to use some atomics for safety/correctness.

I believe atomics are needed only for dbb-owned locks. In other cases flags should be enough.

[asfernandes]

I'm unable to reproduce the freeze after applying the following patch:

It changed nothing for me.

I'm running plume without parallel workers as following:

plume -u sysdba -p masterkey -t 128 -P 1 -d localhost:/tmp/db.fdb