[FIX] large database restore memory usage fix #17663

Jerther · 2017-06-16T12:28:41Z

Description of the issue/feature this PR addresses:
When restoring a large database archive with the web interface on a low memory system, a MemoryError exception can occur. For example, restoring a 3 GB archive on a 4 GB RAM system results in such error. Olivier Laurent (olt) from Odoo technical support team came up with this patch. I tested it and it works very well. Odoo memory usage dropped significantly when restoring a database.

Current behavior before PR:
Restore a 3GB database archive on a 4 GB RAM system. A lot of memory used and ultimately a MemoryError occurs.

Desired behavior after PR is merged:
Restore a 3GB database archive on a 4 GB RAM system. An acceptable amount of memory used and a successfully restored database.

Additional details:
Without this patch, it seems like Python would not use swap space even if there is plenty available. Very strange. With this patch, everything works well.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

Fixes MemoryError when restoring large database archive.

Fixes a memory issue when restoring large database archive.

xmo-odoo · 2017-06-21T09:08:33Z

addons/web/controllers/main.py

-            dispatch_rpc('db', 'restore', [master_pwd, name, data, str2bool(copy)])
+            data = ''
+            for chunk in iter(lambda: backup_file.read(8190), b''):
+                data += base64.b64encode(chunk)            dispatch_rpc('db', 'restore', [master_pwd, name, data, str2bool(copy)])


I think this is broken.

Also data could probably be a bytearray (explicitly mutable bytes) rather than rely on CPython optimisations.

Oops, my mistake.

Could you elaborate on data being a bytearray? As I see it, dispatch_rpc() needs data to be a base64 encoded string. Or maybe I'm missing something.

base64 is a bytes -> bytes conversion, so data is more or less a bytestring (if dispatch_rpc needs a native string, the base64 content should be converted correctly or it's going to blow up badly in Python 3).

Bytes are immutable, concatenating immutable string is fundamentally quadratic. This code implicitly relies on a CPython optimisation[0] to reclaim (amortised) linear behaviour. I would rather we avoid this issue and reliance, by using bytearray. The bytearray can be converted into regular bytes at the end of the loop.

[0] it does and can not exist in pypy

odony · 2017-06-21T15:20:11Z

This discussion is interesting, but it seems to me it would be quite a bit more efficient to entirely skip the b64encode + decode steps, avoiding putting twice the whole dump in memory.
Unless I'm mistaken, the /web/database/restore controller receives a file-like stream of bytes containing the dump. It could directly put it in a temporary file using the file's save() method. And then proceed to restore it in the same fashion exp_restore() does it, with restore_db().

That legacy exp_* XML-RPC API using b64-encoded (in-memory) data is deprecated and not meant for large databases. We don't have to go through it for all db management operations.

Jerther · 2017-06-21T19:02:55Z

How about this? It does save the data in a temporary file, then just pass the path to this file to the restore method. tested and works.

EDIT: reread your comment @odony and decided to go ahead and remove dispatch_rpc.

Jerther · 2017-07-10T19:07:03Z

Ping?

Yenthe666 · 2017-08-07T11:33:57Z

Hi @odony and @xmo-odoo I've just tested this (because I had a customer with this problem) and I can verify that this works fine on both V9 and V10 with bigger databases. Looks like a good improvement.

odony · 2017-08-07T12:46:32Z

addons/web/controllers/main.py

@@ -701,13 +703,17 @@ def backup(self, master_pwd, name, backup_format = 'zip'):

    @http.route('/web/database/restore', type='http', auth="none", methods=['POST'], csrf=False)
    def restore(self, master_pwd, backup_file, name, copy=False):
+        temp_path = tempfile.mkstemp()[1]
+        with open(temp_path, 'w') as data_file:
+            backup_file.save(data_file)


this step (the whole with block) should be in the try/finally, shouldn't it?

odony · 2017-08-07T12:47:52Z

addons/web/controllers/main.py

@@ -701,13 +703,17 @@ def backup(self, master_pwd, name, backup_format = 'zip'):

    @http.route('/web/database/restore', type='http', auth="none", methods=['POST'], csrf=False)
    def restore(self, master_pwd, backup_file, name, copy=False):
+        temp_path = tempfile.mkstemp()[1]


any specific reason to use mkstemp rather than a NamedTempFile, as done in exp_restore?

odony · 2017-08-07T12:48:43Z

odoo/service/db.py

@@ -211,9 +211,13 @@ def dump_db(db_name, stream, backup_format='zip'):
            return stdout

 def exp_restore(db_name, data, copy=False):
+    def chunks(d, n=8192):


Do you still need this chunking system now?

Since we don't use exp_restore anymore in the scope of this PR, this modification could be dropped but IIRC exp_restore is still used in the restful api and this chunking system addresses a critical point of memory usage where the whole database was loaded into RAM by .decode('base64')

And move save operation in try block.

Yenthe666 · 2017-08-26T13:43:52Z

Ping for @xmo-odoo and @oco-odoo is there any change needed anymore?

KangOl · 2017-08-28T09:20:32Z

addons/web/controllers/main.py

@@ -702,12 +704,15 @@ def backup(self, master_pwd, name, backup_format = 'zip'):
    @http.route('/web/database/restore', type='http', auth="none", methods=['POST'], csrf=False)
    def restore(self, master_pwd, backup_file, name, copy=False):
        try:
-            data = base64.b64encode(backup_file.read())
-            dispatch_rpc('db', 'restore', [master_pwd, name, data, str2bool(copy)])
+            with tempfile.NamedTemporaryFile(delete=False) as data_file:


Why not let the contextmanager delete the file?

Because I wanted the file closed to avoid potential access and concurrency problems in db.restore_db(). Otherwise, I agree this part could be rewritten to take advantage of the context manager.

Because I wanted the file closed to avoid potential access and concurrency problems in db.restore_db().

Indeed IIRC Windows/NTFS uses exclusive read locks by default, so if the file is still open here it won't be readable by psql/pg_restore/…

Closing the file should also ensure everything is properly flushed.

Yenthe666 · 2017-08-28T09:31:32Z

By the way, I believe this one is related to #12376 too but 12376 should be closed in favour of this one.
Either a backport to 9.0 would be great or we should rebase it against 9.0. Thoughts?

Yenthe666 · 2017-09-16T17:08:13Z

@KangOl, @xmo-odoo and @odony is anything further needed? I would really like to get this in. This is an important PR.

Yenthe666 · 2017-10-16T06:28:17Z

@KangOl, @xmo-odoo and @odony can we please pick up this PR again? It has been open for months and it is a gem.

Yenthe666 · 2017-11-05T18:20:44Z

@odony @mart-e and @xmo-odoo come on guys.. We're almost there, can we please push for the last part?
On a sidenote: I've been bitten by this one again, so I might be slightly frustrated about this issue.

Yenthe666 · 2017-11-16T09:46:39Z

Ping.

Yenthe666 · 2017-12-13T09:56:14Z

@odony @mart-e and @xmo-odoo come on guys 😞

Yenthe666 · 2018-01-10T08:22:04Z

Looks like we're finally getting a fix (at #22131)
Too bad the original code here is not used and that there is no credit to @Jerther as the code looks very similar..

mart-e · 2018-01-10T08:24:18Z

@Yenthe666 look twice, the author of the code is kept in the commit

Yenthe666 · 2018-01-10T08:26:34Z

Yep, I was wrong, I'm sorry! I'm very happy that this PR is finally being handled, it'll improve this feature a lot. Thanks guys.

Fixes MemoryError when restoring large database archive. Closes odoo#17663 opw-788273

Fixes MemoryError when restoring large database archive. Closes #17663 opw-788273

nim-odoo · 2018-01-12T09:20:18Z

Closed with 270b2eb
Thanks for your contribution.

Yenthe666 · 2018-01-12T09:31:06Z

Hooray! Thanks a lot @nim-odoo 🎉

Fixes MemoryError when restoring large database archive. Closes odoo#17663 opw-788273

MuhammedNoufalI · 2022-02-12T13:13:51Z

When I try to restore database backup to another server (digital ocean to test server on aws), showing an error like this, I tried to change odoo base to same as of the live server.Live server has 2vcpu, 4gb, extra swap space and test server have less resource.

Traceback (most recent call last): File "/odoo14/odoo14-server/odoo/api.py", line 792, in get field_cache = field_cache[record.env.cache_key(field)] KeyError: (None,)

During handling of the above exception, another exception occurred:

Traceback (most recent call last): File "/odoo14/odoo14-server/odoo/fields.py", line 972, in get value = env.cache.get(record,\ self) \ \ File\ "/odoo14/odoo14-server/odoo/api.py",\ line\ 796,\ in\ get \ \ \ \ raise\ CacheMiss(record,\ field) odoo.exceptions.CacheMiss:\ 'res.users(20,).image_128'