From 840609975a355a42d4bdb257f77856bd25dd03f3 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Thu, 2 Jul 2020 17:08:05 +0000 Subject: [PATCH] [IMP] core: better way to set/update magic fields The goal of this change is to simplify the code managing `create_date` and `write_date` in methods `create()` and `write()`, and also to remove weird behaviors caused by the way those fields were updated. Assume we update a simple field on a record. This adds pending updates for the field and `write_date`. However, the value of `write_date` is not known yet: it will be updated as `NOW() AT TIME ZONE 'UTC'` in SQL. So `write_date` is actually given a dummy value in pending updates, and it is invalidated from cache, until its value is flushed to the database and fetched again. Now assume we access another field on the record, and that field is not in cache. The prefetching mechanism will read all column fields, including `write_date`, and flush them first. # this adds pending updates foo: 42, write_uid: 1, write_date: False record.foo = 42 # assume 'bar' is not in cache; this prefetches all column fields, # which flushes the pending updates above before reading them back result = record.bar We can avoid flushing pending updates if the values read from database do not overwrite existing values in cache. If you assume that the value of a pending update is in cache (in the example, `foo: 42`), you don't need to flush the corresponding field. Indeed, the value of `foo` will remain 42 in cache, whatever its value in the database. This assumption (pending updates are in cache) is true for all fields *except* for `write_date`: it is invalidated from cache, and given a dummy value in pending updates. This branch actually makes this assumption true for all fields. The avoidance of flushing pending updates will be done in another commit. In order to directly assign `write_date` its value, we use a cache for the value `NOW() AT TIME ZONE 'UTC'` from the database. This costs at most one query per transaction, and potentially saves a few queries. Co-authored-by: Victor Feyens --- .../hr_holidays/tests/test_company_leave.py | 2 +- addons/test_mail/tests/test_performance.py | 32 +++++------ .../tests/test_performance.py | 4 +- .../tests/test_assetsbundle.py | 3 +- .../test_new_api/tests/test_new_fields.py | 4 +- .../tests/test_performance.py | 14 ++--- odoo/models.py | 54 ++++++++----------- odoo/sql_db.py | 10 ++++ 8 files changed, 62 insertions(+), 61 deletions(-) diff --git a/addons/hr_holidays/tests/test_company_leave.py b/addons/hr_holidays/tests/test_company_leave.py index 486c353425c..2fa68336e4e 100644 --- a/addons/hr_holidays/tests/test_company_leave.py +++ b/addons/hr_holidays/tests/test_company_leave.py @@ -313,7 +313,7 @@ class TestCompanyLeave(TransactionCase): }) company_leave._compute_date_from_to() - with self.assertQueryCount(__system__=735, admin=865): + with self.assertQueryCount(__system__=736, admin=865): # Original query count: 1987 # Without tracking/activity context keys: 5154 company_leave.action_validate() diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index ecc2b110715..20788ed23c7 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -231,7 +231,7 @@ class TestMailAPIPerformance(BaseMailPerformance): #voip module read activity_type during create leading to one less query in enterprise on action_feedback category = activity.activity_type_id.category - with self.assertQueryCount(__system__=16, emp=20): + with self.assertQueryCount(__system__=16, emp=18): activity.action_feedback(feedback='Zizisse Done !') @users('__system__', 'emp') @@ -248,7 +248,7 @@ class TestMailAPIPerformance(BaseMailPerformance): record.write({'name': 'Dupe write'}) - with self.assertQueryCount(__system__=17, emp=20): + with self.assertQueryCount(__system__=17, emp=19): record.action_close('Dupe feedback') self.assertEqual(record.activity_ids, self.env['mail.activity']) @@ -270,7 +270,7 @@ class TestMailAPIPerformance(BaseMailPerformance): 'partner_ids': [(4, customer_id)], }) - with self.assertQueryCount(__system__=25, emp=32): + with self.assertQueryCount(__system__=24, emp=31): composer.send_mail() @users('__system__', 'emp') @@ -281,7 +281,7 @@ class TestMailAPIPerformance(BaseMailPerformance): test_record = self.env['mail.test.ticket'].browse(self.test_record_full.id) test_template = self.env['mail.template'].browse(self.test_template_full.id) # TODO FIXME non deterministic, last check 25 Mar 2020. Runbot at most +7 compared to local. - with self.assertQueryCount(__system__=11, emp=13): + with self.assertQueryCount(__system__=11, emp=12): composer = self.env['mail.compose.message'].with_context({ 'default_composition_mode': 'comment', 'default_model': test_record._name, @@ -290,7 +290,7 @@ class TestMailAPIPerformance(BaseMailPerformance): }).create({}) composer.onchange_template_id_wrapper() - with self.assertQueryCount(__system__=33, emp=40): + with self.assertQueryCount(__system__=32, emp=39): composer.send_mail() # remove created partner to ensure tests are the same each run @@ -302,7 +302,7 @@ class TestMailAPIPerformance(BaseMailPerformance): def test_message_assignation_email(self): self.user_test.write({'notification_type': 'email'}) record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(__system__=37, emp=38): + with self.assertQueryCount(__system__=36, emp=37): record.write({ 'user_id': self.user_test.id, }) @@ -355,7 +355,7 @@ class TestMailAPIPerformance(BaseMailPerformance): def test_message_post_one_email_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(__system__=28, emp=31): + with self.assertQueryCount(__system__=27, emp=30): record.message_post( body='

Test Post Performances with an email ping

', partner_ids=self.customer.ids, @@ -481,7 +481,7 @@ class TestMailComplexPerformance(BaseMailPerformance): 'recipient_ids': [(4, pid) for pid in self.partners.ids], }) mail_ids = mail.ids - with self.assertQueryCount(__system__=7, emp=7): + with self.assertQueryCount(__system__=6, emp=6): self.env['mail.mail'].sudo().browse(mail_ids).send() self.assertEqual(mail.body_html, '

Test

') @@ -494,7 +494,7 @@ class TestMailComplexPerformance(BaseMailPerformance): self.container.message_subscribe(self.user_portal.partner_id.ids) record = self.container.with_user(self.env.user) - with self.assertQueryCount(__system__=63, emp=64): + with self.assertQueryCount(__system__=61, emp=62): record.message_post( body='

Test Post Performances

', message_type='comment', @@ -511,7 +511,7 @@ class TestMailComplexPerformance(BaseMailPerformance): record = self.container.with_user(self.env.user) template_id = self.env.ref('test_mail.mail_test_container_tpl').id - with self.assertQueryCount(__system__=73, emp=75): + with self.assertQueryCount(__system__=70, emp=71): record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -582,7 +582,7 @@ class TestMailComplexPerformance(BaseMailPerformance): }) rec1 = rec.with_context(active_test=False) # to see inactive records self.assertEqual(rec1.message_partner_ids, self.partners | self.env.user.partner_id) - with self.assertQueryCount(__system__=37, emp=38): + with self.assertQueryCount(__system__=36, emp=37): rec.write({'user_id': self.user_portal.id}) self.assertEqual(rec1.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) # write tracking message @@ -602,7 +602,7 @@ class TestMailComplexPerformance(BaseMailPerformance): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(__system__=108, emp=109): + with self.assertQueryCount(__system__=105, emp=106): rec = self.env['mail.test.ticket'].create({ 'name': 'Test', 'container_id': container_id, @@ -631,7 +631,7 @@ class TestMailComplexPerformance(BaseMailPerformance): rec1 = rec.with_context(active_test=False) # to see inactive records self.assertEqual(rec1.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) self.assertEqual(len(rec1.message_ids), 1) - with self.assertQueryCount(__system__=77, emp=77): + with self.assertQueryCount(__system__=75, emp=75): rec.write({ 'name': 'Test2', 'container_id': self.container.id, @@ -668,7 +668,7 @@ class TestMailComplexPerformance(BaseMailPerformance): rec1 = rec.with_context(active_test=False) # to see inactive records self.assertEqual(rec1.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(__system__=85, emp=85): + with self.assertQueryCount(__system__=83, emp=83): rec.write({ 'name': 'Test2', 'container_id': container_id, @@ -701,7 +701,7 @@ class TestMailComplexPerformance(BaseMailPerformance): rec1 = rec.with_context(active_test=False) # to see inactive records self.assertEqual(rec1.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) - with self.assertQueryCount(__system__=29, emp=30): + with self.assertQueryCount(__system__=28, emp=29): rec.write({ 'name': 'Test2', 'customer_id': customer_id, @@ -951,7 +951,7 @@ class TestMailHeavyPerformancePost(BaseMailPerformance): ] self.attachements = self.env['ir.attachment'].with_user(self.env.user).create(self.vals) attachement_ids = self.attachements.ids - with self.assertQueryCount(emp=79): + with self.assertQueryCount(emp=76): self.cr.sql_log = self.warm and self.cr.sql_log_count record.with_context({}).message_post( body='

Test body

', diff --git a/addons/test_mass_mailing/tests/test_performance.py b/addons/test_mass_mailing/tests/test_performance.py index 85fdf26bae8..127857c4d4f 100644 --- a/addons/test_mass_mailing/tests/test_performance.py +++ b/addons/test_mass_mailing/tests/test_performance.py @@ -51,7 +51,7 @@ class TestMassMailPerformance(TestMassMailPerformanceBase): }) # runbot needs +50 compared to local - with self.assertQueryCount(__system__=1714, marketing=1715): + with self.assertQueryCount(__system__=1664, marketing=1664): mailing.action_send_mail() self.assertEqual(mailing.sent, 50) @@ -91,7 +91,7 @@ class TestMassMailBlPerformance(TestMassMailPerformanceBase): }) # runbot needs +62 compared to local - with self.assertQueryCount(__system__=1992, marketing=1993): + with self.assertQueryCount(__system__=1942, marketing=1942): mailing.action_send_mail() self.assertEqual(mailing.sent, 50) diff --git a/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py b/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py index 2ac03de2f8e..5cd488c0c10 100644 --- a/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py +++ b/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py @@ -706,8 +706,9 @@ class TestAssetsBundleWithIRAMock(FileTouchable): # has really been modified. If we do not update the write_date to a posterior date, we are # not able to reproduce the case where we compile this bundle again without changing # anything. - self.env['ir.attachment'].flush(['checksum']) + self.env['ir.attachment'].flush(['checksum', 'write_date']) self.cr.execute("update ir_attachment set write_date=clock_timestamp() + interval '10 seconds' where id = (select max(id) from ir_attachment)") + self.env['ir.attachment'].invalidate_cache(['write_date']) # Compile a fourth time, without changes self._bundle(self._get_asset(), False, False) diff --git a/odoo/addons/test_new_api/tests/test_new_fields.py b/odoo/addons/test_new_api/tests/test_new_fields.py index c4c04982688..0195277d990 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -3115,7 +3115,7 @@ class TestFieldParametersValidation(common.TransactionCase): def insert(model, *fnames): """ Return the expected query string to INSERT the given columns. """ - columns = ['create_uid', 'create_date', 'write_uid', 'write_date'] + sorted(fnames) + columns = sorted(fnames + ('create_uid', 'create_date', 'write_uid', 'write_date')) return 'INSERT INTO "{}" ("id", {}) VALUES (nextval(%s), {}) RETURNING id'.format( model._table, ", ".join('"{}"'.format(column) for column in columns), @@ -3125,7 +3125,7 @@ def insert(model, *fnames): def update(model, *fnames): """ Return the expected query string to UPDATE the given columns. """ - columns = sorted(fnames) + ['write_uid', 'write_date'] + columns = sorted(fnames + ('write_uid', 'write_date')) return 'UPDATE "{}" SET {} WHERE id IN %s'.format( model._table, ", ".join('"{}" = %s'.format(column) for column in columns), diff --git a/odoo/addons/test_performance/tests/test_performance.py b/odoo/addons/test_performance/tests/test_performance.py index d95f51c7d8b..04f5c089f5b 100644 --- a/odoo/addons/test_performance/tests/test_performance.py +++ b/odoo/addons/test_performance/tests/test_performance.py @@ -136,12 +136,12 @@ class TestPerformance(SavepointCaseWithUserDemo): self.assertEqual(rec1.line_ids, lines) # delete N lines: O(1) queries - with self.assertQueryCount(14): + with self.assertQueryCount(15): rec1.invalidate_cache() rec1.write({'line_ids': [Command.delete(line.id) for line in lines[0]]}) self.assertEqual(rec1.line_ids, lines[1:]) - with self.assertQueryCount(12): + with self.assertQueryCount(13): rec1.invalidate_cache() rec1.write({'line_ids': [Command.delete(line.id) for line in lines[1:]]}) self.assertFalse(rec1.line_ids) @@ -151,12 +151,12 @@ class TestPerformance(SavepointCaseWithUserDemo): lines = rec1.line_ids # unlink N lines: O(1) queries - with self.assertQueryCount(14): + with self.assertQueryCount(15): rec1.invalidate_cache() rec1.write({'line_ids': [Command.unlink(line.id) for line in lines[0]]}) self.assertEqual(rec1.line_ids, lines[1:]) - with self.assertQueryCount(12): + with self.assertQueryCount(13): rec1.invalidate_cache() rec1.write({'line_ids': [Command.unlink(line.id) for line in lines[1:]]}) self.assertFalse(rec1.line_ids) @@ -190,7 +190,7 @@ class TestPerformance(SavepointCaseWithUserDemo): self.assertEqual(rec2.line_ids, lines) # empty N lines in rec2: O(1) queries - with self.assertQueryCount(__system__=13, demo=13): + with self.assertQueryCount(14): rec1.invalidate_cache() rec2.write({'line_ids': [Command.clear()]}) self.assertFalse(rec2.line_ids) @@ -210,13 +210,13 @@ class TestPerformance(SavepointCaseWithUserDemo): self.assertEqual(rec1.line_ids, lines[1:]) self.assertEqual(rec2.line_ids, lines[0]) - with self.assertQueryCount(7): + with self.assertQueryCount(6): rec1.invalidate_cache() rec2.write({'line_ids': [Command.set(lines.ids)]}) self.assertFalse(rec1.line_ids) self.assertEqual(rec2.line_ids, lines) - with self.assertQueryCount(5): + with self.assertQueryCount(4): rec1.invalidate_cache() rec2.write({'line_ids': [Command.set(lines.ids)]}) self.assertEqual(rec2.line_ids, lines) diff --git a/odoo/models.py b/odoo/models.py index 96215434e2b..55f28cf11fc 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -3582,6 +3582,12 @@ Fields: if not(self.env.uid == SUPERUSER_ID and not self.pool.ready): bad_names.update(LOG_ACCESS_COLUMNS) + # set magic fields + vals = {key: val for key, val in vals.items() if key not in bad_names} + if self._log_access: + vals.setdefault('write_uid', self.env.uid) + vals.setdefault('write_date', self.env.cr.now()) + determine_inverses = defaultdict(list) # {inverse: fields} records_to_inverse = {} # {field: records} relational_names = [] @@ -3645,24 +3651,9 @@ Fields: real_recs = self.filtered('id') - # If there are only fields that do not trigger _write (e.g. only - # determine inverse), the below ensures that `write_date` and - # `write_uid` are updated (`test_orm.py`, `test_write_date`) - if self._log_access and self.ids: - towrite = env.all.towrite[self._name] - for record in real_recs: - towrite[record.id]['write_uid'] = self.env.uid - towrite[record.id]['write_date'] = False - self.env.cache.invalidate([ - (self._fields['write_date'], self.ids), - (self._fields['write_uid'], self.ids), - ]) - # for monetary field, their related currency field must be cached # before the amount so it can be rounded correctly for fname in sorted(vals, key=lambda x: self._fields[x].type=='monetary'): - if fname in bad_names: - continue field = self._fields[fname] field.write(self, vals[fname]) @@ -3722,6 +3713,12 @@ Fields: # determine records that require updating parent_path parent_records = self._parent_store_update_prepare(vals) + if self._log_access: + # set magic fields (already done by write(), but not for computed fields) + vals = dict(vals) + vals.setdefault('write_uid', self.env.uid) + vals.setdefault('write_date', self.env.cr.now()) + # determine SQL values columns = [] # list of (column_name, format, value) @@ -3737,12 +3734,6 @@ Fields: assert field.column_type columns.append((name, field.column_format, val)) - if self._log_access: - if not vals.get('write_uid'): - columns.append(('write_uid', '%s', self._uid)) - if not vals.get('write_date'): - columns.append(('write_date', '%s', AsIs("(now() at time zone 'UTC')"))) - # update columns if columns: query = 'UPDATE "%s" SET %s WHERE id IN %%s' % ( @@ -3810,6 +3801,15 @@ Fields: # add missing defaults vals = self._add_missing_default_values(vals) + # set magic fields + for name in bad_names: + vals.pop(name, None) + if self._log_access: + vals.setdefault('create_uid', self.env.uid) + vals.setdefault('create_date', self.env.cr.now()) + vals.setdefault('write_uid', self.env.uid) + vals.setdefault('write_date', self.env.cr.now()) + # distribute fields into sets for various purposes data = {} data['stored'] = stored = {} @@ -3817,8 +3817,6 @@ Fields: data['inherited'] = inherited = defaultdict(dict) data['protected'] = protected = set() for key, val in vals.items(): - if key in bad_names: - continue field = self._fields.get(key) if not field: raise ValueError("Invalid field %r on model %r" % (key, self._name)) @@ -3919,18 +3917,10 @@ Fields: other_fields = set() # non-column fields translated_fields = set() # translated fields - # column names, formats and values (for common fields) - columns0 = [('id', "nextval(%s)", self._sequence)] - if self._log_access: - columns0.append(('create_uid', "%s", self._uid)) - columns0.append(('create_date', "%s", AsIs("(now() at time zone 'UTC')"))) - columns0.append(('write_uid', "%s", self._uid)) - columns0.append(('write_date', "%s", AsIs("(now() at time zone 'UTC')"))) - for data in data_list: # determine column values stored = data['stored'] - columns = [column for column in columns0 if column[0] not in stored] + columns = [('id', "nextval(%s)", self._sequence)] for name, val in sorted(stored.items()): field = self._fields[name] assert field.store diff --git a/odoo/sql_db.py b/odoo/sql_db.py index 7df9f6011fb..92aa9bcf56a 100644 --- a/odoo/sql_db.py +++ b/odoo/sql_db.py @@ -257,6 +257,7 @@ class Cursor(BaseCursor): self._default_log_exceptions = True self.cache = {} + self._now = None def __build_dict(self, row): return {d.name: row[i] for i, d in enumerate(self._obj.description)} @@ -444,6 +445,7 @@ class Cursor(BaseCursor): flush_env(self) self.precommit.run() result = self._cnx.commit() + self._now = None self.prerollback.clear() self.postrollback.clear() self.postcommit.run() @@ -457,6 +459,7 @@ class Cursor(BaseCursor): self.postcommit.clear() self.prerollback.run() result = self._cnx.rollback() + self._now = None self.postrollback.run() return result @@ -468,6 +471,13 @@ class Cursor(BaseCursor): def closed(self): return self._closed + def now(self): + """ Return the transaction's timestamp ``NOW() AT TIME ZONE 'UTC'``. """ + if self._now is None: + self.execute("SELECT (now() AT TIME ZONE 'UTC')") + self._now = self.fetchone()[0] + return self._now + class TestCursor(BaseCursor): """ A pseudo-cursor to be used for tests, on top of a real cursor. It keeps