unusable teleport_tubes file after updating to 5.6.0

[BuckarooBanzay]

After updating pandorabox from 5.5.1 to 5.6.0 i got greeted with this:

minetest_1             | 2022-08-12 05:35:09: ERROR[Main]: /data/world//worldmods/pipeworks/teleport_tube.lua:48: attempt to index upvalue 'tp_tube_db' (a nil value)
minetest_1             | 2022-08-12 05:35:09: ERROR[Main]: stack traceback:
minetest_1             | 2022-08-12 05:35:09: ERROR[Main]: 	/data/world//worldmods/pipeworks/teleport_tube.lua:48: in function 'read_tube_db'
minetest_1             | 2022-08-12 05:35:09: ERROR[Main]: 	/data/world//worldmods/pipeworks/teleport_tube.lua:293: in function 'get_db'

Didn't investigate any further but it looks like the save-file was serialized with the new algo and somehow got corrupted :/

Here the files, before and after:

• teleport_tubes_pandorabox_5.5.1.txt
• teleport_tubes_pandorabox_5.6.0.txt

[appgurueu]

1. This is way larger data than serialize should ever handle - consider splitting it across multiple files or storing it in a proper database;
2. pipeworks should wrap the call to minetest.deserialize in an assert;
3. Executing this with LuaJIT, I get

$ luajit teleport_tubes_pandorabox_5.6.0.txt 
luajit: teleport_tubes_pandorabox_5.6.0.txt:1: main function has more than 65536 constants

as expected; loading the 5.5.1 file works just fine however. The problem presumably is that the new serialization takes up one constant slot for every reference it uses (e.g. r%X = ...), halving the number of constants you can have.

The relevant issue is minetest/minetest#7574

[BuckarooBanzay]

main function has more than 65536 constants

Yeah, i was afraid of that :/

consider splitting it across multiple files or storing it in a proper database;

A proper database isn't really a portable option right now (with a "plain" mod/lua setup)
I think either using json with a single file or sharding it per-player in the mod-storage would be a better long-term solution 🤔

[TurkeyMcMac]

Mod storage might work well, since the SQLite3 backend doesn't have to write everything when only one entry changes. There's some duplication of data in memory, but I have a PR open for that: minetest/minetest#12562

[TurkeyMcMac]

Here's a patch that I haven't tested at all:

diff --git a/teleport_tube.lua b/teleport_tube.lua
index ad6a51c..7c05104 100644
--- a/teleport_tube.lua
+++ b/teleport_tube.lua
@@ -1,8 +1,9 @@
 local S = minetest.get_translator("pipeworks")
-local filename=minetest.get_worldpath() .. "/teleport_tubes"
+local filename=minetest.get_worldpath() .. "/teleport_tubes" -- Only used for backward-compat
+local storage=minetest.get_mod_storage()
 
 local tp_tube_db = nil -- nil forces a read
-local tp_tube_db_version = 2.0
+local tp_tube_db_version = 3.0
 
 -- cached rceiver list: hash(pos) => {receivers}
 local cache = {}
@@ -12,47 +13,78 @@ local function hash(pos)
 end
 
 local function save_tube_db()
-	local file, err = io.open(filename, "w")
-	if file then
-		tp_tube_db.version = tp_tube_db_version
-		file:write(minetest.serialize(tp_tube_db))
-		tp_tube_db.version = nil
-		io.close(file)
-	else
-		error(err)
+	-- reset tp-tube cache
+	cache = {}
+
+	local fields = {version = tp_tube_db_version}
+	for key, val in pairs(tp_tube_db) do
+		fields[key] = minetest.serialize(val)
 	end
+	storage:from_table({fields = fields})
+end
+
+local function save_tube_db_entry(hash)
 	-- reset tp-tube cache
 	cache = {}
+
+	local val = tp_tube_db[hash]
+	storage:set_string(hash, val and minetest.serialize(val) or "")
 end
 
 local function migrate_tube_db()
+	local old_version = tp_tube_db.version or 0
+	tp_tube_db.version = nil
+	if old_version < 2.0 then
 		local tmp_db = {}
-		tp_tube_db.version = nil
 		for _, val in pairs(tp_tube_db) do
 			if(val.channel ~= "") then -- skip unconfigured tubes
 				tmp_db[hash(val)] = val
 			end
 		end
 		tp_tube_db = tmp_db
-		save_tube_db()
+	end
+	save_tube_db()
 end
 
 local function read_tube_db()
 	local file = io.open(filename, "r")
-	if file ~= nil then
+	if not file then
+		tp_tube_db = {}
+
+		for key, val in pairs(storage:to_table().fields) do
+			if tonumber(key) then
+				tp_tube_db[key] = minetest.deserialize(val)
+			elseif key == "version" then
+				tp_tube_db[key] = tonumber(val)
+			else
+				error("Unknown field in teleport tube DB: " .. key)
+			end
+		end
+
+		if tp_tube_db.version == nil then
+			tp_tube_db.version = tp_tube_db_version
+			storage:set_string("version", tp_tube_db.version)
+		elseif tp_tube_db.version > tp_tube_db_version then
+			error("Cannot read teleport tube DB of version " .. tp_tube_db.version)
+		end
+	else
 		local file_content = file:read("*all")
 		io.close(file)
 
 		if file_content and file_content ~= "" then
 			tp_tube_db = minetest.deserialize(file_content)
-			if(not tp_tube_db.version or tonumber(tp_tube_db.version) < tp_tube_db_version) then
-				migrate_tube_db()
-			end
-			tp_tube_db.version = nil -- we add it back when saving
-			return tp_tube_db -- we read sucessfully
+		else
+			tp_tube_db = {version = 2.0}
 		end
 	end
-	tp_tube_db = {}
+
+	if(not tp_tube_db.version or tonumber(tp_tube_db.version) < tp_tube_db_version) then
+		migrate_tube_db()
+	end
+	tp_tube_db.version = nil
+
+	os.remove(filename)
+
 	return tp_tube_db
 end
 
@@ -69,7 +101,7 @@ local function set_tube(pos, channel, can_receive)
 	if tube then
 		tube.channel = channel
 		tube.cr = can_receive
-		save_tube_db()
+		save_tube_db_entry(hash)
 		return
 	end
 
@@ -88,13 +120,14 @@ local function set_tube(pos, channel, can_receive)
 	end
 
 	tp_tube_db[hash] = {x=pos.x,y=pos.y,z=pos.z,channel=channel,cr=can_receive}
-	save_tube_db()
+	save_tube_db_entry(hash)
 end
 
 local function remove_tube(pos)
 	local tubes = tp_tube_db or read_tube_db()
-	tubes[hash(pos)] = nil
-	save_tube_db()
+	local hash = hash(pos)
+	tubes[hash] = nil
+	save_tube_db_entry(hash)
 end
 
 local function read_node_with_vm(pos)
@@ -114,7 +147,6 @@ local function get_receivers(pos, channel)
 
 	local tubes = tp_tube_db or read_tube_db()
 	local receivers = {}
-	local dirty = false
 	for key, val in pairs(tubes) do
 		-- skip all non-receivers and the tube that it came from as early as possible, as this is called often
 		if (val.cr == 1 and val.channel == channel and (val.x ~= pos.x or val.y ~= pos.y or val.z ~= pos.z)) then
@@ -125,13 +157,10 @@ local function get_receivers(pos, channel)
 				table.insert(receivers, val)
 			else
 				tp_tube_db[key] = nil
-				dirty = true
+				save_tube_db_entry(key)
 			end
 		end
 	end
-	if dirty then
-		save_tube_db()
-	end
 	-- cache the result for next time
 	cache[hash] = receivers
 	return receivers
@@ -290,6 +319,7 @@ end
 pipeworks.tptube = {
 	hash = hash,
 	save_tube_db = save_tube_db,
+	save_tube_db_entry = save_tube_db_entry,
 	get_db = function() return tp_tube_db or read_tube_db() end,
 	set_tube = set_tube,
 	update_meta = update_meta,

[S-S-X]

Would it be better to leave file there and instead of checking for file check for existence of new db and add documentation note that file can be removed?

[TurkeyMcMac]

I'll open a PR.

[SwissalpS]

It seems better to check for existance of new db first and fallback to find the file. After update, the file can then be removed or left. That would avoid accidents when somebody restores the file.