From db7c9ba7c56a7b7839b0e98060a2d334bf4b8ad8 Mon Sep 17 00:00:00 2001 From: Florian Charlier Date: Fri, 5 Aug 2022 16:29:59 +0000 Subject: [PATCH] [IMP] tools, website_{*}: increment multiple fields at once * = blog, forum, slides For performance reasons, allow to increment multiple fields of the same record within the same query. With python tests. Task-2663320 Part of odoo/odoo#79615 --- addons/website_blog/controllers/main.py | 2 +- addons/website_forum/models/forum.py | 2 +- addons/website_slides/controllers/main.py | 4 +- addons/website_slides/models/slide_slide.py | 2 +- odoo/addons/test_performance/models/models.py | 13 ++++ .../security/ir.model.access.csv | 1 + .../tests/test_performance.py | 77 ++++++++++++++++++- odoo/tools/sql.py | 38 +++++---- 8 files changed, 119 insertions(+), 20 deletions(-) diff --git a/addons/website_blog/controllers/main.py b/addons/website_blog/controllers/main.py index 47b68e456fc..e197fa5403d 100644 --- a/addons/website_blog/controllers/main.py +++ b/addons/website_blog/controllers/main.py @@ -295,7 +295,7 @@ class WebsiteBlog(http.Controller): response = request.render("website_blog.blog_post_complete", values) if blog_post.id not in request.session.get('posts_viewed', []): - if sql.increment_field_skiplock(blog_post, 'visits'): + if sql.increment_fields_skiplock(blog_post, 'visits'): if not request.session.get('posts_viewed'): request.session['posts_viewed'] = [] request.session['posts_viewed'].append(blog_post.id) diff --git a/addons/website_forum/models/forum.py b/addons/website_forum/models/forum.py index 3068628130a..8ed0d505ceb 100644 --- a/addons/website_forum/models/forum.py +++ b/addons/website_forum/models/forum.py @@ -904,7 +904,7 @@ class Post(models.Model): def _set_viewed(self): self.ensure_one() - return sql.increment_field_skiplock(self, 'views') + return sql.increment_fields_skiplock(self, 'views') def _get_access_action(self, access_uid=None, force_website=False): """ Instead of the classic form view, redirect to the post on the website directly """ diff --git a/addons/website_slides/controllers/main.py b/addons/website_slides/controllers/main.py index 4582cd20d85..a839551edc7 100644 --- a/addons/website_slides/controllers/main.py +++ b/addons/website_slides/controllers/main.py @@ -67,9 +67,7 @@ class WebsiteSlides(WebsiteProfile): if not slide.channel_id.is_member: viewed_slides = request.session.setdefault('viewed_slides', set()) if slide.id not in viewed_slides: - if tools.sql.increment_field_skiplock(slide, 'public_views'): - # Increment total views immediately as compute is not triggered - tools.sql.increment_field_skiplock(slide, 'total_views') + if tools.sql.increment_fields_skiplock(slide, 'public_views', 'total_views'): viewed_slides.add(slide.id) request.session.touch() else: diff --git a/addons/website_slides/models/slide_slide.py b/addons/website_slides/models/slide_slide.py index 38858fe4c75..31fa287f9c0 100644 --- a/addons/website_slides/models/slide_slide.py +++ b/addons/website_slides/models/slide_slide.py @@ -882,7 +882,7 @@ class Slide(models.Model): ('partner_id', '=', target_partner.id) ]) if quiz_attempts_inc and existing_sudo: - sql.increment_field_skiplock(existing_sudo, 'quiz_attempts_count') + sql.increment_fields_skiplock(existing_sudo, 'quiz_attempts_count') existing_sudo.invalidate_recordset(['quiz_attempts_count']) new_slides = self_sudo - existing_sudo.mapped('slide_id') diff --git a/odoo/addons/test_performance/models/models.py b/odoo/addons/test_performance/models/models.py index 95c0ef9bb54..2f1bef28210 100644 --- a/odoo/addons/test_performance/models/models.py +++ b/odoo/addons/test_performance/models/models.py @@ -67,3 +67,16 @@ class Eggs(models.Model): _description = 'Test Performance Eggs' name = fields.Char() + + +class Mozzarella(models.Model): + _name = 'test_performance.mozzarella' + _description = 'Test Performance Mozzarella' + + value = fields.Integer(default=0, required=True) + value_plus_one = fields.Integer(compute="_value_plus_one", required=True, store=True) + + @api.depends('value') + def _value_plus_one(self): + for record in self: + record.value_plus_one = record.value + 1 diff --git a/odoo/addons/test_performance/security/ir.model.access.csv b/odoo/addons/test_performance/security/ir.model.access.csv index 510f23fa10f..e5e3e139190 100644 --- a/odoo/addons/test_performance/security/ir.model.access.csv +++ b/odoo/addons/test_performance/security/ir.model.access.csv @@ -4,3 +4,4 @@ access_test_performance_line,access_test_performance_line,model_test_performance access_test_performance_tag,access_test_performance_tag,model_test_performance_tag,,1,1,1,1 access_test_performance_bacon,access_test_performance_bacon,model_test_performance_bacon,,1,1,1,1 access_test_performance_eggs,access_test_performance_eggs,model_test_performance_eggs,,1,1,1,1 +access_test_performance_mozzarella,access_test_performance_mozzarella,model_test_performance_mozzarella,,1,1,1,1 diff --git a/odoo/addons/test_performance/tests/test_performance.py b/odoo/addons/test_performance/tests/test_performance.py index a0b651c860f..d3192f3eb94 100644 --- a/odoo/addons/test_performance/tests/test_performance.py +++ b/odoo/addons/test_performance/tests/test_performance.py @@ -3,12 +3,15 @@ from collections import defaultdict import json +import logging from odoo.addons.base.tests.common import SavepointCaseWithUserDemo from odoo.tests.common import TransactionCase, users, warmup, tagged -from odoo.tools import mute_logger, json_default +from odoo.tools import mute_logger, json_default, sql from odoo import Command +_logger = logging.getLogger(__name__) + class TestPerformance(SavepointCaseWithUserDemo): @@ -610,3 +613,75 @@ class TestMapped(TransactionCase): with self.assertQueryCount(3): for rec in recs: rec.line_ids.mapped('value') + + +@tagged('increment_perf') +class TestIncrementFieldsSkipLock(TransactionCase): + """ Test the behavior of the function `increment_fields_skiplock`. + + Note that, per-design, the function will not always update the requested + records in case of a (table/row-level) lock. This is reflected in these + tests as we make sure to check the integrity of the results whether + any record was updated or not. + """ + @classmethod + def setUpClass(cls): + super().setUpClass() + + cls.record = cls.env['test_performance.mozzarella'].create([{ + 'value': 1, + 'value_plus_one': 2, + }]) + cls.other_record = cls.env['test_performance.mozzarella'].create([{ + 'value': 10, + 'value_plus_one': 11, + }]) + + def test_increment_fields_skiplock_one_field(self): + """ Test that we can increment the value of a single field of a record + with `increment_fields_skiplock` and that it doesn't trigger _compute + methods of fields depending on it. + + If the test fails because of changes making that _compute **are** + triggered, be sure to also check `increment_fields_skiplock` uses on the + codebase when updating this test class. + """ + with self.assertQueryCount(1): + did_update = sql.increment_fields_skiplock(self.record, 'value') + _logger.info('increment_fields_skiplock did %supdate the field', '' if did_update else 'not ') + + # increment_fields_skiplock does not invalidate the cache + self.record.invalidate_recordset() + + with self.assertQueryCount(1): # Read + if did_update: + self.assertEqual(self.record.value, 2, "according to increment_fields_skiplock's output, this number should have been incremented.") + else: + # Making sure the test random locks do not break the test; but updated and the values must be consistent + self.assertEqual(self.record.value, 1, "according to increment_fields_skiplock output's, this number should NOT have been incremented.") + + self.assertEqual(self.record.value_plus_one, 2, "This value should not have been incremented, irrespective of the presence of a lock or not.") + + self.assertEqual(self.other_record.value, 10, "other_record should not have been updated.") + self.assertEqual(self.other_record.value_plus_one, 11, "other_record should not have been updated.") + + def test_increment_fields_skiplock_multiple_fields(self): + """ Test that we can update several fields on the same rows with one request. + """ + with self.assertQueryCount(1): + did_update = sql.increment_fields_skiplock(self.record, 'value', 'value_plus_one') + _logger.info('increment_fields_skiplock did %supdate the fields', '' if did_update else 'not ') + + # increment_fields_skiplock does not invalidate the cache + self.record.invalidate_recordset() + + with self.assertQueryCount(1): # Read + if did_update: + self.assertEqual(self.record.value, 2, "according to increment_fields_skiplock's output, this number should have been incremented.") + self.assertEqual(self.record.value_plus_one, 3, "according to increment_fields_skiplock's output, this number should have been incremented.") + else: # Making sure the test random locks do not break the test; but updated and the values must be consistent + self.assertEqual(self.record.value, 1, "according to increment_fields_skiplock output's, this number should NOT have been incremented.") + self.assertEqual(self.record.value_plus_one, 2, "according to increment_fields_skiplock's output, this number should NOT have been incremented.") + + self.assertEqual(self.other_record.value, 10, "other_record should not have been updated.") + self.assertEqual(self.other_record.value_plus_one, 11, "other_record should not have been updated.") diff --git a/odoo/tools/sql.py b/odoo/tools/sql.py index c18e9f94c88..637fa69aabe 100644 --- a/odoo/tools/sql.py +++ b/odoo/tools/sql.py @@ -5,6 +5,8 @@ import logging import psycopg2 +from psycopg2.sql import SQL, Identifier + import odoo.sql_db from collections import defaultdict from contextlib import closing @@ -313,25 +315,35 @@ def reverse_order(order): return ', '.join(items) -def increment_field_skiplock(record, field): +def increment_fields_skiplock(records, *fields): """ - Increment 'friendly' the [field] of the current [record](s) + Increment 'friendly' the given `fields` of the current `records`. If record is locked, we just skip the update. It doesn't invalidate the cache since the update is not critical. - :rtype: bool - if field has been incremented or not + :param records: recordset to update + :param fields: integer fields to increment + :returns: whether the specified fields were incremented on any record. + :rtype: bool """ - if not record: + if not records: return False - assert record._fields[field].type == 'integer' + for field in fields: + assert records._fields[field].type == 'integer' - cr = record._cr - query = """ - UPDATE {table} SET {field} = {field} + 1 WHERE id IN ( - SELECT id from {table} WHERE id in %(ids)s FOR UPDATE SKIP LOCKED - ) RETURNING id - """.format(table=record._table, field=field) - cr.execute(query, {'ids': tuple(record.ids)}) + query = SQL(""" + UPDATE {table} + SET {sets} + WHERE id IN (SELECT id FROM {table} WHERE id = ANY(%(ids)s) FOR UPDATE SKIP LOCKED) + """).format( + table=Identifier(records._table), + sets=SQL(', ').join(map( + SQL('{0} = {0} + 1').format, + map(Identifier, fields) + )) + ) - return bool(cr.fetchone()) + cr = records._cr + cr.execute(query, {'ids': records.ids}) + return bool(cr.rowcount)