[TreeHunter9]

Description of the problem

The TIP cache consists of fixed-size blocks called TransactionStatusBlock (TSB) that contain a CN (Commit/Snapshot Number), where the transaction number serves as the index within that TSB. When a TSB fills up, the next TSB is created; that is, there can be several of them, and they are allocated as needed. A TSB is initialized upon the first connection to the database; it is then filled at runtime as transactions are executed. TSBs are also unloaded if no one is using a given block anymore. This is where the key problem lies: after a block is unloaded, it disappears, and if it is needed again, a new TSB will be allocated and initialized to zero, while the TIP will not be read from disk to load the current transaction statuses. In other words, when reading this TSB, we’ll get a CN equal to 0, where 0 represents the transaction status tra_active. This confusion leads to the following: a check begins to determine whether the transaction is actually active; the answer is negative; next, we read the TIP to obtain the current state, and it returns tra_committed. The two states received tra_active + tra_committed are interpreted as the transaction was committed after our transaction began, which means it should not be visible in our snapshot. As a result, these records become invisible to us, and we skip them, even though we should actually see them.

Reproducable example

0. In the firebird.conf, set ServerMode = Classic and TipCacheBlockSize = 65536 to fill the TSB more quickly (a single TSB can hold ~8k transactions);
1. Create a persistent connection to the database, this is needed to initialize the TIP cache and to ensure that GlobalTpcHeader (which stores latest_commit_number, used to generate the CN) remains active;
2. Connect to the database (using new connection) and start incrementing the transaction number and CN to create a second TSB, for example, using:

set term ^;

execute block as
  declare i bigint = 0;
  declare d integer;
begin
  while (i < 11240) do
  begin
    in autonomous transaction do
      select 1 from rdb$database into :d;
    i = i + 1;
  end
end^

set term ;^

The number 11240 was chosen to ensure we go exactly beyond the first TSB. Commit and disconnect;

3. Connect, create a table create table tab1(a int);, commit, insert a record insert into tab1 values (1);, commit, and disconnect;
4. Connect, execute select * from tab1, and see that nothing is returned;

The main idea behind the fix

Basically, do not purge or memset to zero TSB when there is at least one connection to the database. Do it only when:

• OIT is moved beyond TSB range (this logic was before me, doesn't change it);
• Last connection is disconnected;
• First connection is connected to database. This is done to remove any TSBs that may remain after a crash or an unfortunate shutdown;

Description of changes that is not straight forward related to TipCache

• I decided to improve the performance of directory iteration because it now can affect performance of first connection and last disconnection. On Windows implementation is basically the same, only small improvements. On Linux implementation has been reworked, it now uses a much faster method for iterating through files that does not require calling stat for each file.
• I aligned the SharedMemory initialization logic on Windows with Linux implementation. I discovered that on Windows there is initialization of zeros to newly mapped region, whereas on Linux the newly mapped region is left untouched, so this is creates some differences between platforms.
  I looked at Vlad's patch, which adds this initialization of zeros on Windows (by doing truncation of the file to 0 and then extend it back), and as I understand it, the main idea was not to perform memset, but to trigger an error to see whether any other instance of our server is holding the file (as I understand, because of IPC namespace/session differences, our Event will not be visible to other instance). I have reimplement this check by doing exclusive access to file, so if there is any other instance that holds the file, we will get an error about it. I hope I'm not missing something about it, so I think @hvlad should look at this.
  And I think, the SharedMemory should not memset mapped region to zero anyway, his responsibility is to simply perform a mmap. The decision of whether to perform memset or not should be left to the caller, not to the mapping mechanism.

Affected versions

v6 and v5. I haven't tested this on other versions, but if the code is the same, then the issue applies to them as well.

[hvlad]

Try this branch first: work/gh-9017-trace

[hvlad]

• I looked at Vlad's patch, which adds this initialization of zeros on Windows (by doing truncation of the file to 0 and then extend it back)

No, file was truncated before that patch. Lines 2227-2237 in file before patch

[TreeHunter9]

Try this branch first: work/gh-9017-trace

Yeah, I cannot reproduce the problem using my example on this branch, either on Linux or Windows. At first glance the patch looks correct, but I will take a closer look at it.

No, file was truncated before that patch. Lines 2227-2237 in file before patch

Hm, indeed, then I don't know why such difference between platforms was made.

[TreeHunter9]

At first glance the patch looks correct, but I will take a closer look at it.

I have found several issues with your patch:

	if (dbb->dbb_next_transaction < next_transaction)
		dbb->dbb_next_transaction = next_transaction;

	if (dbb->dbb_oldest_transaction < oldest_transaction)
		dbb->dbb_oldest_transaction = oldest_transaction;

dbb_next_transaction and dbb_oldest_transaction are updated without any protection, so there can be a data race. When shutdown is performing everything is fine because we are the last one who can touch these variables, but TRA_update_counters can be called from start_sweeper, so there is a possibility that we will overwrite other assignment. Maybe move this code under CCH_FETCH + CCH_RELEASE above, so it will be some sort of critical section?

	if (!cache->m_exclusive)
	{
		PathName fullName = makeSharedMemoryFileName(dbb, blockNumber, true);
		struct stat fileStat;

		if (os_utils::stat(fullName.c_str(), &fileStat) == 0)
			fileExists = (fileStat.st_size >= blockSize);
	}

In StatusBlockData::StatusBlockData() two threads can see that file is not exists, it means at shared memory initialization they both will memset this block. There is possibility for race condition, where one thread is successfully init the shmem and managed to write CN into it, then second thread is awakening and starting to do it's own init process with another one memset due to fileExists == true.

3. Inside TipCache::finalizeTpc() you first delete TSB that in you cache (m_blocks_memory), and then deletes all other TSB that is left on file system (releaseSharedMemory()) by looking from the newest block number and until you find a gap (e.g. starts from block 5, down to block 3 because file for that block is not existing, so we deleting block 5 and 4), but this logic is broken because you firstly delete TSB in cache.
   For example:

• att 1 is persistent connection;
• att 2 is creating a lot of transaction, so TSB 1, 2, 3, 4 is created, then att 2 is leaving;
• att 3 is connected, so at transaction start it will read only TSB 4 and put it in cache;
• att 1 is disconnected;
• att 3 is disconnected, so it will execute finalizeTpc() by firstly drop TSB 4, then start doing iteration over TSB on file system by doing lookup for TSB 4 first, it doesn't find it on disk, so basically cleanup for TSB on FS is over, so TSB 1, 2, 3 will stay on disk forever;

4. Theoretically, if server crashes (some sort of file corruption), we will leave all TSB blocks in place, and what if we will try to restore our old backup over existing file, will inode be changed? It's important because we generate TSB filename based on st_dev + st_ino, and if inode will not be changed, we accidentally can read TSB with garbage data. It can happens only if NT will move back (very old backup for example) out of range of TSB block, and on TipCache initialization we will not memset this TSB because it is out of our range. When our NT will move forward enough, we will reach this TSB block from previous version of database and will use it without memsetting it.
   This is all highly theoretical, but who knows what can happen on real objects.

5. It is possible that 2 attachments will execute finalizeTpc() and both of them gets m_exclusive = LCK_convert(tdbb, m_lock, LCK_EX, LCK_NO_WAIT); equal to false because both of them holding the LCK_SW lock after initializeTpc(). So there is a possibility that no one will purge the TSBs.
   P.S.: As I can see my patch also affected by this issue.

[TreeHunter9]

But overall I like your version of patch more, my version is more like a brute-force approach of deleting TSBs, so I think we will stick with your version.

But maybe some parts of my patch can be accepted, for example directory iterator reimplementation?