tryton-env: Remove index management from field definition

issue5757

COLLABORATOR=cedric.krier@b2ck.com

[reviewbot, 2021-02-26 18:18:59]

flake8 OK
URL: https://codereview.tryton.org/361251002

[albert, 2021-02-26 22:02:46]

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
trytond/trytond/backend/table.py:193: :param table: The table of which the index
is created.
of -> on

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
trytond/trytond/backend/table.py:195: expressions are usually Columns but may be
SQL expressions refering
referring

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
trytond/trytond/backend/table.py:197: :param **kwargs: Additional keywords
arguments.
Additional keyword arguments

[dave, 2021-02-27 18:48:09]

On 2021/02/26 22:02:46, albert wrote:
>
https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
> File trytond/trytond/backend/table.py (right):
> 
>
https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
> trytond/trytond/backend/table.py:193: :param table: The table of which the
index
> is created.
> of -> on
> 
>
https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
> trytond/trytond/backend/table.py:195: expressions are usually Columns but may
be
> SQL expressions refering
> referring
> 
>
https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
> trytond/trytond/backend/table.py:197: :param **kwargs: Additional keywords
> arguments.
> Additional keyword arguments

+1 to all of these.

[nicoe, 2021-08-30 07:33:19]

https://codereview.tryton.org/361251002/diff/371671002/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/371671002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:885:
Database.index_translators.append(SimilarityTranslator)
I wonder if as we support natively unaccent the Range and Similarity translator
shouldn't support it also (through an option).

[reviewbot, 2021-09-03 17:07:03]

https://codereview.tryton.org/361251002/diff/373491003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/database.py:29:
'trytond.backend.SimilarityIndex' imported but unused

https://codereview.tryton.org/361251002/diff/373491003/trytond/trytond/ir/que...
trytond/trytond/ir/queue.py:42:
the backslash is redundant between brackets

https://codereview.tryton.org/361251002/diff/373491003/trytond/trytond/ir/que...
trytond/trytond/ir/queue.py:43:
continuation line unaligned for hanging indent

https://codereview.tryton.org/361251002/diff/373491003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:327:
the backslash is redundant between brackets

https://codereview.tryton.org/361251002/diff/373491003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:328:
continuation line unaligned for hanging indent


URL: https://codereview.tryton.org/361251002

[reviewbot, 2021-09-07 17:30:00]

https://codereview.tryton.org/361251002/diff/352681002/trytond/trytond/backen...
trytond/trytond/backend/sqlite/database.py:29:
'trytond.backend.SimilarityIndex' imported but unused

https://codereview.tryton.org/361251002/diff/352681002/trytond/trytond/ir/que...
trytond/trytond/ir/queue.py:42:
the backslash is redundant between brackets

https://codereview.tryton.org/361251002/diff/352681002/trytond/trytond/ir/que...
trytond/trytond/ir/queue.py:43:
continuation line unaligned for hanging indent

https://codereview.tryton.org/361251002/diff/352681002/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:327:
the backslash is redundant between brackets

https://codereview.tryton.org/361251002/diff/352681002/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:328:
continuation line unaligned for hanging indent


URL: https://codereview.tryton.org/361251002

[reviewbot, 2021-09-08 16:01:38]

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:36:
'sql.Literal' imported but unused

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/sqlite/database.py:29:
'trytond.backend.SimilarityIndex' imported but unused

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/ir/que...
trytond/trytond/ir/queue.py:42:
the backslash is redundant between brackets

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/ir/que...
trytond/trytond/ir/queue.py:43:
continuation line unaligned for hanging indent

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:86:
the backslash is redundant between brackets

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:87:
continuation line unaligned for hanging indent

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/ir/tri...
trytond/trytond/ir/trigger.py:333:
the backslash is redundant between brackets

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/ir/tri...
trytond/trytond/ir/trigger.py:334:
continuation line unaligned for hanging indent

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:327:
the backslash is redundant between brackets

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:328:
continuation line unaligned for hanging indent


URL: https://codereview.tryton.org/361251002

[ced, 2021-10-03 19:40:06]

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/table.py (right):

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:414: 'name': self.name,
The name of the index should also be limited to namedatalen.

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:415: 'only': ' ONLY' if
self.kwargs.get('pg_only') else '',
only is not supported by pg <= 10.

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/backen...
trytond/trytond/backend/table.py:203: self.kwargs = kwargs
I do not see the point to have unused kwargs.

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (right):

https://codereview.tryton.org/361251002/diff/341541002/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:186: cls._indexes = []
I do not like to store backend specific object in the ModelSQL.

https://codereview.tryton.org/361251002/diff/365911002/trytond/CHANGELOG
File trytond/CHANGELOG (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/CHANGELOG#newc...
trytond/CHANGELOG:1: * Remove index management from field definition
I guess it is better to say that it is moved somewhere else

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/__init__.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:15: 'IndexRequest', 'EqualityIndex',
'RangeIndex', 'SimilarityIndex']
I think this should be defined in modelsql.py

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:18: class IndexRequest:
Not sure about the suffix Request.
This is just an index definition.
For example the constraint are named Unique not UniqueRequest

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:26: class IndexUsage:
I think the usage should have a string attribute like 'name' which can be used
to distinct them without having to test issubclass. This will avoid the need to
import them.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:47: def get(cls, request):
Maybe this could be defined as a method on Database or TableHandler

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/database.py:285: class IndexTranslatorInterface:
I think we could drop the "Translator".

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:817: class
EqualityTranslator(PGIndexMixin, IndexTranslatorInterface):
For me the should be named HashIndex because it is confusing to have a naming
similarity with the Index definition.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:851: return 1
This is always returned

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:890: cls._check_trgm_extension()
This should be managed on the Database

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/module...
File trytond/trytond/modules/__init__.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/module...
trytond/trytond/modules/__init__.py:288: pool.create_indexes()
I do not like the idea that it is the pool that created indexes.
I think it will be better to manage that like the models_to_update_history.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/pool.py
File trytond/trytond/pool.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/pool.p...
trytond/trytond/pool.py:286: cursor.execute(*translator.sql(name,
index_request))
For me the name should be generated by the translator.
So if multiple index requests end to create the same index we should create only
one.

[nicoe, 2021-11-10 09:53:33]

https://codereview.tryton.org/361251002/diff/365911002/trytond/CHANGELOG
File trytond/CHANGELOG (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/CHANGELOG#newc...
trytond/CHANGELOG:1: * Remove index management from field definition
On 2021/10/03 19:40:05, ced wrote:
> I guess it is better to say that it is moved somewhere else

Done.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/__init__.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:15: 'IndexRequest', 'EqualityIndex',
'RangeIndex', 'SimilarityIndex']
On 2021/10/03 19:40:05, ced wrote:
> I think this should be defined in modelsql.py

Then I think it would be better in a dedicated file in trytond.model
But I am afraid that there might be a circular importation issue when importing
the IndexUsage in the backend implementations.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:18: class IndexRequest:
On 2021/10/03 19:40:05, ced wrote:
> Not sure about the suffix Request.
> This is just an index definition.
> For example the constraint are named Unique not UniqueRequest

Done.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:26: class IndexUsage:
On 2021/10/03 19:40:05, ced wrote:
> I think the usage should have a string attribute like 'name' which can be used
> to distinct them without having to test issubclass. This will avoid the need
to
> import them.

What's the problem with importing them?

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:47: def get(cls, request):
On 2021/10/03 19:40:05, ced wrote:
> Maybe this could be defined as a method on Database or TableHandler

Done.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/database.py:285: class IndexTranslatorInterface:
On 2021/10/03 19:40:05, ced wrote:
> I think we could drop the "Translator".

Well this is the object that translate the expression of the index into the SQL.
Dropping the name would make it a bit like the it's the interface of the Index
defined in trytond.model

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:817: class
EqualityTranslator(PGIndexMixin, IndexTranslatorInterface):
On 2021/10/03 19:40:06, ced wrote:
> For me the should be named HashIndex because it is confusing to have a naming
> similarity with the Index definition.

Done.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:851: return 1
On 2021/10/03 19:40:06, ced wrote:
> This is always returned

Done.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:890: cls._check_trgm_extension()
On 2021/10/03 19:40:06, ced wrote:
> This should be managed on the Database

Do you think it would be used for something else?

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/module...
File trytond/trytond/modules/__init__.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/module...
trytond/trytond/modules/__init__.py:288: pool.create_indexes()
On 2021/10/03 19:40:06, ced wrote:
> I do not like the idea that it is the pool that created indexes.
> I think it will be better to manage that like the models_to_update_history.

Done.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/pool.py
File trytond/trytond/pool.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/pool.p...
trytond/trytond/pool.py:286: cursor.execute(*translator.sql(name,
index_request))
On 2021/10/03 19:40:06, ced wrote:
> For me the name should be generated by the translator.
> So if multiple index requests end to create the same index we should create
only
> one.

Done.

[reviewbot, 2021-11-10 10:14:36]

patch is not applicable
URL: https://codereview.tryton.org/361251002

[ced, 2021-11-10 10:18:12]

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/__init__.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:15: 'IndexRequest', 'EqualityIndex',
'RangeIndex', 'SimilarityIndex']
On 2021/11/10 09:53:31, nicoe wrote:
> On 2021/10/03 19:40:05, ced wrote:
> > I think this should be defined in modelsql.py
> 
> Then I think it would be better in a dedicated file in trytond.model
> But I am afraid that there might be a circular importation issue when
importing
> the IndexUsage in the backend implementations.

For me the backend should not import ORM stuffs. Just like it does not import
fields. So we should use the same design as for field by using keyword strings.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:26: class IndexUsage:
On 2021/11/10 09:53:31, nicoe wrote:
> On 2021/10/03 19:40:05, ced wrote:
> > I think the usage should have a string attribute like 'name' which can be
used
> > to distinct them without having to test issubclass. This will avoid the need
> to
> > import them.
> 
> What's the problem with importing them?

Reduce import complexity, ease of extension etc. Just like for the fields.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/database.py:285: class IndexTranslatorInterface:
On 2021/11/10 09:53:32, nicoe wrote:
> On 2021/10/03 19:40:05, ced wrote:
> > I think we could drop the "Translator".
> 
> Well this is the object that translate the expression of the index into the
SQL.
> Dropping the name would make it a bit like the it's the interface of the Index
> defined in trytond.model

Except that it does not really translate. It is just selecting the best option.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:890: cls._check_trgm_extension()
On 2021/11/10 09:53:32, nicoe wrote:
> On 2021/10/03 19:40:06, ced wrote:
> > This should be managed on the Database
> 
> Do you think it would be used for something else?

I do not know but it is a property of the database.

[nicoe, 2022-02-03 22:14:55]

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/__init__.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/__init__.py:26: class IndexUsage:
On 2021/11/10 10:18:11, ced wrote:
> On 2021/11/10 09:53:31, nicoe wrote:
> > On 2021/10/03 19:40:05, ced wrote:
> > > I think the usage should have a string attribute like 'name' which can be
> used
> > > to distinct them without having to test issubclass. This will avoid the
need
> > to
> > > import them.
> > 
> > What's the problem with importing them?
> 
> Reduce import complexity, ease of extension etc. Just like for the fields.

They've been moved to trytond.model.index, it would be easier to discuss about
the current implementation.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/database.py:285: class IndexTranslatorInterface:
On 2021/11/10 10:18:11, ced wrote:
> On 2021/11/10 09:53:32, nicoe wrote:
> > On 2021/10/03 19:40:05, ced wrote:
> > > I think we could drop the "Translator".
> > 
> > Well this is the object that translate the expression of the index into the
> SQL.
> > Dropping the name would make it a bit like the it's the interface of the
Index
> > defined in trytond.model
> 
> Except that it does not really translate. It is just selecting the best
option.

I disagree the Index object should not have the same name than the object that
materialize the abstract representation into the database.

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/365911002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:890: cls._check_trgm_extension()
On 2021/11/10 10:18:11, ced wrote:
> On 2021/11/10 09:53:32, nicoe wrote:
> > On 2021/10/03 19:40:06, ced wrote:
> > > This should be managed on the Database
> > 
> > Do you think it would be used for something else?
> 
> I do not know but it is a property of the database.

Done.

[reviewbot, 2022-02-03 22:19:00]

patch is not applicable
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-02-07 10:31:45]

patch is not applicable
URL: https://codereview.tryton.org/361251002

[ced, 2022-02-22 19:11:07]

I'm wondering if we should not keep the select=True or to automatically add an
index declaration for the target of One2Many.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:293: chosen_translator = translator
Will it not be cleaner to use filter and sort.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:297: class IndexTranslatorInterface:
I think it should be in table not database.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:297: class IndexTranslatorInterface:
For me it is strange to name a class with Translator.
I see this as the implementation of an Index for a specific backend.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:306:
usage.__class__.__name__.encode('utf8')))))
I think it should use the TableHandler.convert_name because there are length
limits.

And probably we do not need to have 'index_' prefix as it consumes space for no
additional information.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:306:
usage.__class__.__name__.encode('utf8')))))
I do not think we must use the class name because for example TrigramTranslator
and BTreeTranslator can generate the same btree index but with a different name.

Or the classes must generate only one kind of index.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:42: from trytond.model.index
import EqualityIndex, RangeIndex, SimilarityIndex
I'm afraid that will make circular dependency and that make trouble in the
future (and maybe in custom backend).

The Index class could be passed are argument of the methods.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:734: connection =
self.get_connection()
The connection must be put back.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:739:
self._extensions[extension_name] = bool(cursor.fetchone())
Is it not always one because fetchone returns a tuple?
It is probably safer to use rowcount like has_proc.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:760: class PGIndexMixin:
Not sure we need to prefix with PG

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:781: query = template % values
Maybe it will be cleaner to use f-string

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:817: params['collate'] = ('
COLLATE "%s"'
better to break at opening (

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:821: params['order'] = ' ' +
usage.options['order'].upper()
maybe call only once upper

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:822: if
usage.options.get('nulls_first'):
Why having an _

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:825: params['nulls'] = ' NULLS
LAST'
As they are mutually exclusive, it may be better to use:

options['nulls'] = 'FIRST'

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:825: params['nulls'] = ' NULLS
LAST'
Or even better to include the NULLS in the order because there is no point to
have NULLS close without an order (maybe it is even not valid).

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:838: return
len(request.expressions) * 100
Should it not count only the expression with Equality.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:847: return [(e, u) for e, u in
request.expressions
break on opening [

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:856: score = 1
I think that if there is no proper usage, we should not create a btree.
For example similarity usage does not benefit from btree.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:871: return [(e, u) for e, u in
request.expressions
break at opening [

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:879: if any(not isinstance(u,
SimilarityIndex)
break at opening (

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:883: return 0
Why?

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:896: method = 'btree'
I do not think that TrigramTranslator should create btree indexes.

Probably that BTreeTranslator should increase its score for usage with
collation.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
File trytond/trytond/model/index.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
trytond/trytond/model/index.py:1: class Index:
Missing copyright header

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
trytond/trytond/model/index.py:24: pass
I'm wondering if it will not be a better place to put in modelsql.py as it can
only be used for ModelSQL.

Also I think we could use the class Index as namespace like:

class Index:
   ...

   class Equality(Usage):
      ...

This will avoid to have to put Index suffix everywhere.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:188: cls._indexes = {}
I think I would prefer to use a list have comparable Index. So developer could
make a list.remove.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/module...
File trytond/trytond/modules/__init__.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/module...
trytond/trytond/modules/__init__.py:298: cursor.execute(*translator.sql(index))
I think it will be cleaner to create a __post_register__ (similar to
__post_setup__).
And to have this code in the __post_register__ of ModelSQL.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/res/us...
File trytond/trytond/res/user.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/res/us...
trytond/trytond/res/user.py:114: name = fields.Char('Name')
Why is not there a similarity index?

[nicoe, 2022-03-10 21:04:16]

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:293: chosen_translator = translator
On 2022/02/22 19:11:06, ced wrote:
> Will it not be cleaner to use filter and sort.

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:297: class IndexTranslatorInterface:
On 2022/02/22 19:11:06, ced wrote:
> For me it is strange to name a class with Translator.
> I see this as the implementation of an Index for a specific backend.

What's in trytond.model.index are indexes.
Here I use translator because it translates this definition into the required
SQL for the backend.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:297: class IndexTranslatorInterface:
On 2022/02/22 19:11:06, ced wrote:
> I think it should be in table not database.

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:306:
usage.__class__.__name__.encode('utf8')))))
On 2022/02/22 19:11:06, ced wrote:
> I do not think we must use the class name because for example
TrigramTranslator
> and BTreeTranslator can generate the same btree index but with a different
name.

I think that the name generated should reflect the index definition.

Indeed two translators could generate the same sql query but the name should be
the same as the will translate the same index.

Moreover only one translator will be selected for each index.

> Or the classes must generate only one kind of index.

Which classes?

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/database.py:306:
usage.__class__.__name__.encode('utf8')))))
On 2022/02/22 19:11:06, ced wrote:
> I think it should use the TableHandler.convert_name

Accessing the right convert_name might prove a bit tricky. Currently it's not
overriden by the db classes but it might be in the future. I think I'll put a 

> because there are length limits.

Given the length limit is the usage of the MD5 really that important?

> And probably we do not need to have 'index_' prefix as it consumes space for
no
> additional information.

Indeed.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:42: from trytond.model.index
import EqualityIndex, RangeIndex, SimilarityIndex
On 2022/02/22 19:11:07, ced wrote:
> I'm afraid that will make circular dependency and that make trouble in the
> future (and maybe in custom backend).
> 
> The Index class could be passed are argument of the methods.

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:734: connection =
self.get_connection()
On 2022/02/22 19:11:06, ced wrote:
> The connection must be put back.

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:739:
self._extensions[extension_name] = bool(cursor.fetchone())
On 2022/02/22 19:11:07, ced wrote:
> Is it not always one because fetchone returns a tuple?
> It is probably safer to use rowcount like has_proc.

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:760: class PGIndexMixin:
On 2022/02/22 19:11:06, ced wrote:
> Not sure we need to prefix with PG

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:781: query = template % values
On 2022/02/22 19:11:07, ced wrote:
> Maybe it will be cleaner to use f-string

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:817: params['collate'] = ('
COLLATE "%s"'
On 2022/02/22 19:11:06, ced wrote:
> better to break at opening (

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:821: params['order'] = ' ' +
usage.options['order'].upper()
On 2022/02/22 19:11:07, ced wrote:
> maybe call only once upper

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:822: if
usage.options.get('nulls_first'):
On 2022/02/22 19:11:06, ced wrote:
> Why having an _

I think that people that will define index will prefer to do something like
Index(table, (table.column, RangeIndex()), nulls_first=True)

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:825: params['nulls'] = ' NULLS
LAST'
On 2022/02/22 19:11:07, ced wrote:
> Or even better to include the NULLS in the order because there is no point to
> have NULLS close without an order (maybe it is even not valid).

It's valid. People could want to specify only the null ordering.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:825: params['nulls'] = ' NULLS
LAST'
On 2022/02/22 19:11:06, ced wrote:
> As they are mutually exclusive, it may be better to use:
> 
> options['nulls'] = 'FIRST'

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:838: return
len(request.expressions) * 100
On 2022/02/22 19:11:06, ced wrote:
> Should it not count only the expression with Equality.

I don't know what I was thinking when writing this scoring function :).

A sensible one would probably return 0 if there is any non EqualityIndex and
otherwise return the length * 100.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:847: return [(e, u) for e, u in
request.expressions
On 2022/02/22 19:11:06, ced wrote:
> break on opening [

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:856: score = 1
On 2022/02/22 19:11:06, ced wrote:
> I think that if there is no proper usage, we should not create a btree.
> For example similarity usage does not benefit from btree.

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:871: return [(e, u) for e, u in
request.expressions
On 2022/02/22 19:11:07, ced wrote:
> break at opening [

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:879: if any(not isinstance(u,
SimilarityIndex)
On 2022/02/22 19:11:07, ced wrote:
> break at opening (

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:883: return 0
On 2022/02/22 19:11:06, ced wrote:
> Why?

I misread the pg documentation about multicolumn indexes.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:896: method = 'btree'
On 2022/02/22 19:11:07, ced wrote:
> I do not think that TrigramTranslator should create btree indexes.

Indeed.

> Probably that BTreeTranslator should increase its score for usage with
> collation.

In fact nothing in the doc indicates that collation don't work with gin indexes.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
File trytond/trytond/model/index.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
trytond/trytond/model/index.py:1: class Index:
On 2022/02/22 19:11:07, ced wrote:
> Missing copyright header

Done.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
trytond/trytond/model/index.py:24: pass
On 2022/02/22 19:11:07, ced wrote:
> I'm wondering if it will not be a better place to put in modelsql.py as it can
> only be used for ModelSQL.
>
> Also I think we could use the class Index as namespace like:
> 
> class Index:
>    ...
> 
>    class Equality(Usage):
>       ...
> 
> This will avoid to have to put Index suffix everywhere.

I don't like this way of defining a namespace, we might as well use a module
like this file.
So it would be something like:

   from trytond.model import index

And then we use index.Similarity

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:188: cls._indexes = {}
On 2022/02/22 19:11:07, ced wrote:
> I think I would prefer to use a list have comparable Index.

I don't understand.

> So developer could make a list.remove.

I find it more readable to use names to remove an index with pop or del.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/module...
File trytond/trytond/modules/__init__.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/module...
trytond/trytond/modules/__init__.py:298: cursor.execute(*translator.sql(index))
On 2022/02/22 19:11:07, ced wrote:
> I think it will be cleaner to create a __post_register__ (similar to
> __post_setup__).
> And to have this code in the __post_register__ of ModelSQL.

Why not but it's opening a possibility to do all whole lot of things outside of
the normal process of initialization which is something you've never liked IIRC.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/res/us...
File trytond/trytond/res/user.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/res/us...
trytond/trytond/res/user.py:114: name = fields.Char('Name')
On 2022/02/22 19:11:07, ced wrote:
> Why is not there a similarity index?

I don't know is it that useful?
Isn't the goal also to remove a lot of probably useless indexes?

[nicoe, 2022-03-10 21:06:56]

On 2022/02/22 19:11:07, ced wrote:
> I'm wondering if we should not keep the select=True or to automatically add an
> index declaration for the target of One2Many.

I think the second choice is a more sensible one than the first one because
having select=True as a leftover will encourage people to keep using it while
they should pay attention and decide if the index should be used or not.

[reviewbot, 2022-03-10 21:33:23]

https://codereview.tryton.org/361251002/diff/386211002/trytond/trytond/backen...
trytond/trytond/backend/database.py:3:
'hashlib' imported but unused

https://codereview.tryton.org/361251002/diff/386211002/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:6:
'psycopg2.sql.Composed' imported but unused

https://codereview.tryton.org/361251002/diff/386211002/trytond/trytond/ir/que...
trytond/trytond/ir/queue.py:43:
continuation line unaligned for hanging indent

https://codereview.tryton.org/361251002/diff/386211002/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:502:
continuation line unaligned for hanging indent


URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-05-14 21:01:56]

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/database.py:3:
'hashlib' imported but unused

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:6:
'psycopg2.sql.Composed' imported but unused

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/que...
trytond/trytond/ir/queue_.py:43:
continuation line unaligned for hanging indent

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:502:
continuation line unaligned for hanging indent


URL: https://codereview.tryton.org/361251002

[ced, 2022-06-09 11:48:10]

On 2022/03/10 21:06:56, nicoe wrote:
> On 2022/02/22 19:11:07, ced wrote:
> > I'm wondering if we should not keep the select=True or to automatically add
an
> > index declaration for the target of One2Many.
> 
> I think the second choice is a more sensible one than the first one because
> having select=True as a leftover will encourage people to keep using it while
> they should pay attention and decide if the index should be used or not.

OK this must be implemented for all target of One2Many (like we add index on all
id).

[ced, 2022-06-09 11:55:50]

Missing documentation.

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:188: cls._indexes = {}
On 2022/03/10 21:04:16, nicoe wrote:
> On 2022/02/22 19:11:07, ced wrote:
> > I think I would prefer to use a list have comparable Index.
> 
> I don't understand.

Instead of using a dict and have to give a unique name to each index definition.
It will be better to use a list (or a set) on which we append just the
definition.
But to ease the customization, we need to be able to find index definition so
for that the index definitions must be comparable.

> > So developer could make a list.remove.
> 
> I find it more readable to use names to remove an index with pop or del.

But it will become a nightmare to give unique names. This is all the point of
naming indexes using their definition.
This is already a problem with the constraint so we should not repeat our
mistakes.

https://codereview.tryton.org/361251002/diff/419151003/trytond/CHANGELOG
File trytond/CHANGELOG (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/CHANGELOG#newc...
trytond/CHANGELOG:1: * Move index management to __setup__
I think it would be more instructive to say:

Add index definitions as ModelSQL attribute

The title must also match the changelog entry

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/database.py:3: import hashlib
not used

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/database.py:287: def get_index_translator(self, index):
maybe it could be named:

def index_translator_for(self, index)

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:240: self._extensions = {}
I think it should be a class attribute so it is not clean on each instantiation
like _has_proc.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:472: }
not for this patch

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:725:
self.put_connection(connection)
Should be inside a try/finally

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:473: def _sql(cls, method, name,
request):
I think the name could be computed here.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:477: expr_params = []
I think it should be named query_params or just params

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:507: def _get_expression_params(cls,
method, expression, usage):
I find the name confusing because params is already used for the query params.
Maybe _get_expression_variables

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:540: return len(request.expressions)
* 100
Should not it add 100 for each Equality expression ?

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:599: if any(u.__name__ !=
'Similarity' for _, u in request.expressions):
Should it not sum 100 for each Similarity expression.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/sqlite/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:326: if all(u.__name__ in
{'EqualityIndex', 'RangeIndex'}
Should be Equality and Range

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:328: return 100
I think it should sum 100 for each Equality or Range

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:333: query = (
Maybe also named it expr_template like in postgresql

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:335: '(%%(expressions)s)%%(where)s')
I think it will be cleaner to format only once.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:344: expr_params = []
idem query_params or params

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:213: _th_convert_name =
TableHandlerInterface.convert_name
For me this does not guarantee to use the proper convert_name of the real
TableHandler.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:216: def get_name(cls, request):
request -> index

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
request -> index

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
I'm wondering if it will not be better named: query
because SQL is the language

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:233: def score(cls, request):
request -> index

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/act...
File trytond/trytond/ir/action.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/act...
trytond/trytond/ir/action.py:155: ondelete='CASCADE')
What about action index? This is for a One2Many.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/act...
trytond/trytond/ir/action.py:165: table = cls.__table__()
I think it will be good by convention to instantiate table at the beginning like
for pool.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/act...
trytond/trytond/ir/action.py:998: required=True, ondelete='CASCADE')
idem used for a One2Many

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/cro...
File trytond/trytond/ir/cron.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/cro...
trytond/trytond/ir/cron.py:116: }),
not for this patch

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ema...
File trytond/trytond/ir/email_.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ema...
trytond/trytond/ir/email_.py:248: 'ir.email', "E-mail", required=True,
ondelete='CASCADE')
We need an index on email for the One2Many.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/err...
File trytond/trytond/ir/error.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/err...
trytond/trytond/ir/error.py:75: ], "State", readonly=True, sort=False)
We need an index at least for open and processing values.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/exp...
File trytond/trytond/ir/export.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/exp...
trytond/trytond/ir/export.py:57: export = fields.Many2One('ir.export', 'Export',
required=True,
idem for One2Many

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
File trytond/trytond/ir/model.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
trytond/trytond/ir/model.py:234: ondelete='CASCADE',
idem

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
trytond/trytond/ir/model.py:1218: (table.fs_id, index.Equality())),
should include also module and model for import_record

model should be at the end because post_import does not use it.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
trytond/trytond/ir/model.py:1219: })
need an index on module for populate_to_delete query.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
File trytond/trytond/ir/module.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
trytond/trytond/ir/module.py:383: ], string='State', required=True, sort=False)
do not change quote

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/res...
File trytond/trytond/ir/resource.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/res...
trytond/trytond/ir/resource.py:17: "Resource", selection='get_models',
required=True)
Need to be indexed for like search.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
File trytond/trytond/ir/rule.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
trytond/trytond/ir/rule.py:27: required=True, ondelete='CASCADE')
model may be needed to be indexed for the join.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
trytond/trytond/ir/rule.py:31: help="Add this rule to all users by default.")
global_p and default_p must be indexed for the get query.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
trytond/trytond/ir/rule.py:104: required=True, ondelete="CASCADE")
Must be indexed for the join in the get query

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ses...
File trytond/trytond/ir/session.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ses...
trytond/trytond/ir/session.py:28: table, (table.create_uid, index.Equality())),
Indeed I think we need:

- index on create_uid, key
- index on key, write_date
- index on key, create_date

or we may need to change the reset to use a COALESCE(write_date, create_date)
and so we just need an index on key, COALESCE(write_date, create_date)

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
File trytond/trytond/ir/translation.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:42: ]
not for this patch

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:88: (table.name, index.Equality())),
I think we need an index for the delete_ids query.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:88: (table.name, index.Equality())),
I think it should also contain res_id, value and fuzzy.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:1572: ])
not for this patch

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/icon.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/icon.py:31: table, (table.name, index.Equality())),
We could have an ordered index on sequence and id for the list_icons query

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/icon.py:31: table, (table.name, index.Equality())),
I do not think we need an index on name.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/menu.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/menu.py:88: parent = fields.Many2One('ir.ui.menu', 'Parent
Menu', ondelete='CASCADE')
We need an index for the One2Many

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/view.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:89: table, (table.inherit, index.Equality())),
I do not see for which queries they are.

Indeed I think it is more:

- model, inherit
- id, inherit

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:158: }
not for this patch.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:400: ('field', 'in', list(fields.keys())),
Need index for this query.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:506: (table.user, index.Equality()),
I think I will put user first and domain last to have a more balanced tree.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (left):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:233: history_table.index_action('id',
action='add')
We must add an index on id of history table.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/module...
File trytond/trytond/modules/__init__.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/module...
trytond/trytond/modules/__init__.py:298: cursor.execute(*translator.sql(index))
I think this loop should be managed by a method on ModelSQL.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
File trytond/trytond/res/user.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
trytond/trytond/res/user.py:173: ]
not for this patch

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
trytond/trytond/res/user.py:198: ]
idem

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
trytond/trytond/res/user.py:523: }
idem

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
trytond/trytond/res/user.py:1118: table, (table.key, index.Equality())),
Should be: key, application, state

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
trytond/trytond/res/user.py:1118: table, (table.key, index.Equality())),
I think we need also user, state

[ced, 2022-06-11 08:13:14]

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
I think it should return a plain SQL query because psycopg3 will not support
parameters in DDL:
https://www.psycopg.org/psycopg3/docs/basic/from_pg2.html#server-side-binding

[nicoe, 2022-06-17 16:35:33]

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (right):

https://codereview.tryton.org/361251002/diff/386021002/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:188: cls._indexes = {}
On 2022/06/09 11:55:47, ced wrote:
> On 2022/03/10 21:04:16, nicoe wrote:
> > On 2022/02/22 19:11:07, ced wrote:
> > > I think I would prefer to use a list have comparable Index.
> > 
> > I don't understand.
> 
> Instead of using a dict and have to give a unique name to each index
definition.
> It will be better to use a list (or a set) on which we append just the
> definition.
> But to ease the customization, we need to be able to find index definition so
> for that the index definitions must be comparable.
> 
> > > So developer could make a list.remove.
> > 
> > I find it more readable to use names to remove an index with pop or del.
> 
> But it will become a nightmare to give unique names. This is all the point of
> naming indexes using their definition.
> This is already a problem with the constraint so we should not repeat our
> mistakes.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/CHANGELOG
File trytond/CHANGELOG (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/CHANGELOG#newc...
trytond/CHANGELOG:1: * Move index management to __setup__
On 2022/06/09 11:55:47, ced wrote:
> I think it would be more instructive to say:
> 
> Add index definitions as ModelSQL attribute
> 
> The title must also match the changelog entry

I find that "Use declarative index defintion on ModelSQL" is more explicit.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/database.py:3: import hashlib
On 2022/06/09 11:55:47, ced wrote:
> not used

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/database.py:287: def get_index_translator(self, index):
On 2022/06/09 11:55:47, ced wrote:
> maybe it could be named:
> 
> def index_translator_for(self, index)

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/database.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:240: self._extensions = {}
On 2022/06/09 11:55:47, ced wrote:
> I think it should be a class attribute so it is not clean on each
instantiation
> like _has_proc.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:472: }
On 2022/06/09 11:55:48, ced wrote:
> not for this patch

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/database.py:725:
self.put_connection(connection)
On 2022/06/09 11:55:47, ced wrote:
> Should be inside a try/finally

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:473: def _sql(cls, method, name,
request):
On 2022/06/09 11:55:48, ced wrote:
> I think the name could be computed here.

What's the problem with get_name?

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:477: expr_params = []
On 2022/06/09 11:55:48, ced wrote:
> I think it should be named query_params or just params

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:507: def _get_expression_params(cls,
method, expression, usage):
On 2022/06/09 11:55:48, ced wrote:
> I find the name confusing because params is already used for the query params.
> Maybe _get_expression_variables

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:540: return len(request.expressions)
* 100
On 2022/06/09 11:55:48, ced wrote:
> Should not it add 100 for each Equality expression ?

In fact hash index supports only one expression.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:599: if any(u.__name__ !=
'Similarity' for _, u in request.expressions):
On 2022/06/09 11:55:48, ced wrote:
> Should it not sum 100 for each Similarity expression.

If there is any Equality or Range then it's probably an integer and the trgm
extension won't work on it.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/sqlite/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:326: if all(u.__name__ in
{'EqualityIndex', 'RangeIndex'}
On 2022/06/09 11:55:48, ced wrote:
> Should be Equality and Range

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:328: return 100
On 2022/06/09 11:55:48, ced wrote:
> I think it should sum 100 for each Equality or Range

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:333: query = (
On 2022/06/09 11:55:48, ced wrote:
> Maybe also named it expr_template like in postgresql

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:335: '(%%(expressions)s)%%(where)s')
On 2022/06/09 11:55:48, ced wrote:
> I think it will be cleaner to format only once.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/sqlite/table.py:344: expr_params = []
On 2022/06/09 11:55:48, ced wrote:
> idem query_params or params

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:213: _th_convert_name =
TableHandlerInterface.convert_name
On 2022/06/09 11:55:48, ced wrote:
> For me this does not guarantee to use the proper convert_name of the real
> TableHandler.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:216: def get_name(cls, request):
On 2022/06/09 11:55:48, ced wrote:
> request -> index

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
On 2022/06/09 11:55:48, ced wrote:
> request -> index

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
On 2022/06/09 11:55:48, ced wrote:
> I'm wondering if it will not be better named: query
> because SQL is the language

query is a bit generic no ?

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
On 2022/06/11 08:13:14, ced wrote:
> I think it should return a plain SQL query because psycopg3 will not support
> parameters in DDL:
> https://www.psycopg.org/psycopg3/docs/basic/from_pg2.html#server-side-binding

OK but shouldn't it be handled in the backend and thus when we'll adapt the code
for psycopg3?

sqlite won't complain if we use the standard way to send the query string and
the parameters. Other connectors won't either AFAIK.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:233: def score(cls, request):
On 2022/06/09 11:55:48, ced wrote:
> request -> index

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/act...
File trytond/trytond/ir/action.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/act...
trytond/trytond/ir/action.py:155: ondelete='CASCADE')
On 2022/06/09 11:55:48, ced wrote:
> What about action index? This is for a One2Many.

Indeed but now the indexes for the O2M are created.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/act...
trytond/trytond/ir/action.py:165: table = cls.__table__()
On 2022/06/09 11:55:48, ced wrote:
> I think it will be good by convention to instantiate table at the beginning
like
> for pool.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/act...
trytond/trytond/ir/action.py:998: required=True, ondelete='CASCADE')
On 2022/06/09 11:55:48, ced wrote:
> idem used for a One2Many

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/cro...
File trytond/trytond/ir/cron.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/cro...
trytond/trytond/ir/cron.py:116: }),
On 2022/06/09 11:55:48, ced wrote:
> not for this patch

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ema...
File trytond/trytond/ir/email_.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ema...
trytond/trytond/ir/email_.py:248: 'ir.email', "E-mail", required=True,
ondelete='CASCADE')
On 2022/06/09 11:55:48, ced wrote:
> We need an index on email for the One2Many.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/err...
File trytond/trytond/ir/error.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/err...
trytond/trytond/ir/error.py:75: ], "State", readonly=True, sort=False)
On 2022/06/09 11:55:48, ced wrote:
> We need an index at least for open and processing values.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
File trytond/trytond/ir/model.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
trytond/trytond/ir/model.py:1218: (table.fs_id, index.Equality())),
On 2022/06/09 11:55:48, ced wrote:
> should include also module and model for import_record
> 
> model should be at the end because post_import does not use it.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/mod...
trytond/trytond/ir/model.py:1219: })
On 2022/06/09 11:55:49, ced wrote:
> need an index on module for populate_to_delete query.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/res...
File trytond/trytond/ir/resource.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/res...
trytond/trytond/ir/resource.py:17: "Resource", selection='get_models',
required=True)
On 2022/06/09 11:55:49, ced wrote:
> Need to be indexed for like search.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
File trytond/trytond/ir/rule.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
trytond/trytond/ir/rule.py:27: required=True, ondelete='CASCADE')
On 2022/06/09 11:55:49, ced wrote:
> model may be needed to be indexed for the join.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
trytond/trytond/ir/rule.py:31: help="Add this rule to all users by default.")
On 2022/06/09 11:55:49, ced wrote:
> global_p and default_p must be indexed for the get query.

Indexing boolean values is tricky, I found this answer on SO:

https://stackoverflow.com/questions/42972726/postgresql-create-index-for-bool...

It recommends to use a partial index when we suspect that only a small portion
of the table will have a specific value.

Currently there is no usage where there is no index method maybe it could be
added.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
trytond/trytond/ir/rule.py:104: required=True, ondelete="CASCADE")
On 2022/06/09 11:55:49, ced wrote:
> Must be indexed for the join in the get query

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ses...
File trytond/trytond/ir/session.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ses...
trytond/trytond/ir/session.py:28: table, (table.create_uid, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> Indeed I think we need:
> 
> - index on create_uid, key
> - index on key, write_date
> - index on key, create_date
> 
> or we may need to change the reset to use a COALESCE(write_date, create_date)
> and so we just need an index on key, COALESCE(write_date, create_date)

We could use the COALESCE indeed but reading reset I don't remember why it takes
an optional domain argument.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
File trytond/trytond/ir/translation.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:88: (table.name, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> I think it should also contain res_id, value and fuzzy.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:88: (table.name, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> I think we need an index for the delete_ids query.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:1572: ])
On 2022/06/09 11:55:49, ced wrote:
> not for this patch

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/icon.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/icon.py:31: table, (table.name, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> We could have an ordered index on sequence and id for the list_icons query

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/icon.py:31: table, (table.name, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> I do not think we need an index on name.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/menu.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/menu.py:88: parent = fields.Many2One('ir.ui.menu', 'Parent
Menu', ondelete='CASCADE')
On 2022/06/09 11:55:49, ced wrote:
> We need an index for the One2Many

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/view.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:89: table, (table.inherit, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> I do not see for which queries they are.
> 
> Indeed I think it is more:
> 
> - model, inherit
> - id, inherit

The first one is for fields_view_get which will make ('model', '=', something).
But it might be redundant with your first index.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:158: }
On 2022/06/09 11:55:49, ced wrote:
> not for this patch.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:400: ('field', 'in', list(fields.keys())),
On 2022/06/09 11:55:49, ced wrote:
> Need index for this query.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:506: (table.user, index.Equality()),
On 2022/06/09 11:55:49, ced wrote:
> I think I will put user first and domain last to have a more balanced tree.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (left):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:233: history_table.index_action('id',
action='add')
On 2022/06/09 11:55:49, ced wrote:
> We must add an index on id of history table.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/module...
File trytond/trytond/modules/__init__.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/module...
trytond/trytond/modules/__init__.py:298: cursor.execute(*translator.sql(index))
On 2022/06/09 11:55:49, ced wrote:
> I think this loop should be managed by a method on ModelSQL.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
File trytond/trytond/res/user.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
trytond/trytond/res/user.py:1118: table, (table.key, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> I think we need also user, state

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/res/us...
trytond/trytond/res/user.py:1118: table, (table.key, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> Should be: key, application, state

Done.

[reviewbot, 2022-06-17 17:00:52]

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:6:
'psycopg2.sql.Composed' imported but unused

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:568:
import 'index' from line 27 shadowed by loop variable


URL: https://codereview.tryton.org/361251002

[ced, 2022-08-29 13:36:46]

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:473: def _sql(cls, method, name,
request):
On 2022/06/17 16:35:31, nicoe wrote:
> On 2022/06/09 11:55:48, ced wrote:
> > I think the name could be computed here.
> 
> What's the problem with get_name?

This method could be called with the same index definition but different names.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:599: if any(u.__name__ !=
'Similarity' for _, u in request.expressions):
On 2022/06/17 16:35:31, nicoe wrote:
> On 2022/06/09 11:55:48, ced wrote:
> > Should it not sum 100 for each Similarity expression.
> 
> If there is any Equality or Range then it's probably an integer and the trgm
> extension won't work on it.

But it could work for the other expression. So this does not disqualify the
index completely.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
On 2022/06/17 16:35:31, nicoe wrote:
> On 2022/06/09 11:55:48, ced wrote:
> > I'm wondering if it will not be better named: query
> > because SQL is the language
> 
> query is a bit generic no ?

For it is closer to what it is. I do not see what else could query be on this
object.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
On 2022/06/17 16:35:31, nicoe wrote:
> On 2022/06/11 08:13:14, ced wrote:
> > I think it should return a plain SQL query because psycopg3 will not support
> > parameters in DDL:
> >
https://www.psycopg.org/psycopg3/docs/basic/from_pg2.html#server-side-binding
> 
> OK but shouldn't it be handled in the backend and thus when we'll adapt the
code
> for psycopg3?

We already use SQL.format in table handler for DDL so we should keep that way
because it is cleaner.


> sqlite won't complain if we use the standard way to send the query string and
> the parameters. Other connectors won't either AFAIK.

I do not think we can rely on sqlite as example of DB-API implementation.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
File trytond/trytond/ir/rule.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/rul...
trytond/trytond/ir/rule.py:31: help="Add this rule to all users by default.")
On 2022/06/17 16:35:32, nicoe wrote:
> On 2022/06/09 11:55:49, ced wrote:
> > global_p and default_p must be indexed for the get query.
> 
> Indexing boolean values is tricky, I found this answer on SO:
> 
>
https://stackoverflow.com/questions/42972726/postgresql-create-index-for-bool...
> 
> It recommends to use a partial index when we suspect that only a small portion
> of the table will have a specific value.
> 
> Currently there is no usage where there is no index method maybe it could be
> added.

Acknowledged.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ses...
File trytond/trytond/ir/session.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ses...
trytond/trytond/ir/session.py:28: table, (table.create_uid, index.Equality())),
On 2022/06/17 16:35:32, nicoe wrote:
> On 2022/06/09 11:55:49, ced wrote:
> > Indeed I think we need:
> > 
> > - index on create_uid, key
> > - index on key, write_date
> > - index on key, create_date
> > 
> > or we may need to change the reset to use a COALESCE(write_date,
create_date)
> > and so we just need an index on key, COALESCE(write_date, create_date)
> 
> We could use the COALESCE indeed but reading reset I don't remember why it
takes
> an optional domain argument.

For customization.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/view.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:89: table, (table.inherit, index.Equality())),
On 2022/06/17 16:35:32, nicoe wrote:
> On 2022/06/09 11:55:49, ced wrote:
> > I do not see for which queries they are.
> > 
> > Indeed I think it is more:
> > 
> > - model, inherit
> > - id, inherit
> 
> The first one is for fields_view_get which will make ('model', '=',
something).
> But it might be redundant with your first index.

I do not understand.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/table.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:472:
Database.index_translators.append(cls)
I find it strange to declare Indexes in TableHandler but register them on
Database.
For me it makes more sense to register them also on TableHandler.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:509: def
_get_expression_variables(cls, method, expression, usage):
should not it be called: _get_expression_params

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:585: # XXX text_pattern_ops or
varchar_pattern_ops ?
as far as I can see in the PG code, there are indeed no differences.
And quick test show that you can use varchar_pattern_ops on TEXT column.
So for me varchar_pattern_ops is the good value.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/table.py:218: lambda x:
hashlib.shake_256(x).hexdigest(6),
As it is not for security, we could use shake_128 for speed.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/res...
File trytond/trytond/ir/resource.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/res...
trytond/trytond/ir/resource.py:17: "Resource", selection='get_models',
required=True)
could be a single line?

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/tra...
File trytond/trytond/ir/translation.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:1579: ])
not in this patch

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/icon.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/icon.py:35: (table.id, index.Range(order='ASC'))))
I think we could define it in SequenceOrderedMixin using order (because it is
missing NULLS FIRST).

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/view.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:449: (table.user, index.Equality())))
it is maybe better to put user first (as we may have more users than views).

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
File trytond/trytond/model/index.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
trytond/trytond/model/index.py:3: 
I think it will make more sense to define them in modelsql.py because they can
only be used by ModelSQL.

The Usages could be attributes of Index to keep some sort of namespaces.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
trytond/trytond/model/index.py:21: for s, o in zip(self.expressions,
other.expressions))
Is it not better to compare:

all((str(c), u) == (str(oc), ou) for (c, u), (oc, ou) in zip(...))

because the str of Usage will not contains the options.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:188: cls._indexes = set()
I think it should be named: _sql_indexes

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:352: 
I would prefer to define _create_indexes here with the other registering
methods.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/module...
File trytond/trytond/modules/__init__.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/module...
trytond/trytond/modules/__init__.py:260: (rel_column, index.Equality())))
I do not like to have automatic index creation in modules/__init__.py
It should be done in ModelSQL, probably in __post_setup__

[ced, 2022-08-29 14:47:29]

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/table.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:500: """
I think it will be better to use () and string continuation to avoid unnecessary
spaces (which will make query logging difficult.

[ced, 2022-10-02 12:28:00]

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:473: def _sql(cls, method, name,
request):
On 2022/08/29 13:36:45, ced wrote:
> On 2022/06/17 16:35:31, nicoe wrote:
> > On 2022/06/09 11:55:48, ced wrote:
> > > I think the name could be computed here.
> > 
> > What's the problem with get_name?
> 
> This method could be called with the same index definition but different
names.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:540: return len(request.expressions)
* 100
On 2022/06/17 16:35:31, nicoe wrote:
> On 2022/06/09 11:55:48, ced wrote:
> > Should not it add 100 for each Equality expression ?
> 
> In fact hash index supports only one expression.

Acknowledged.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:599: if any(u.__name__ !=
'Similarity' for _, u in request.expressions):
On 2022/08/29 13:36:45, ced wrote:
> On 2022/06/17 16:35:31, nicoe wrote:
> > On 2022/06/09 11:55:48, ced wrote:
> > > Should it not sum 100 for each Similarity expression.
> > 
> > If there is any Equality or Range then it's probably an integer and the trgm
> > extension won't work on it.
> 
> But it could work for the other expression. So this does not disqualify the
> index completely.

Indeed gin_trgm_ops works only on char.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
On 2022/08/29 13:36:45, ced wrote:
> On 2022/06/17 16:35:31, nicoe wrote:
> > On 2022/06/09 11:55:48, ced wrote:
> > > I'm wondering if it will not be better named: query
> > > because SQL is the language
> > 
> > query is a bit generic no ?
> 
> For it is closer to what it is. I do not see what else could query be on this
> object.

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/backen...
trytond/trytond/backend/table.py:226: def sql(cls, request):
On 2022/08/29 13:36:45, ced wrote:
> On 2022/06/17 16:35:31, nicoe wrote:
> > On 2022/06/11 08:13:14, ced wrote:
> > > I think it should return a plain SQL query because psycopg3 will not
support
> > > parameters in DDL:
> > >
> https://www.psycopg.org/psycopg3/docs/basic/from_pg2.html#server-side-binding
> > 
> > OK but shouldn't it be handled in the backend and thus when we'll adapt the
> code
> > for psycopg3?
> 
> We already use SQL.format in table handler for DDL so we should keep that way
> because it is cleaner.

For now even when we use SQL objects, we still use params. It is probably out of
the scope to remove params usage for DDL but it is still better to use SQL
objects to compose the query.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ses...
File trytond/trytond/ir/session.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ses...
trytond/trytond/ir/session.py:28: table, (table.create_uid, index.Equality())),
On 2022/06/09 11:55:49, ced wrote:
> Indeed I think we need:
> 
> - index on create_uid, key
> - index on key, write_date
> - index on key, create_date
> 
> or we may need to change the reset to use a COALESCE(write_date, create_date)
> and so we just need an index on key, COALESCE(write_date, create_date)

Done.

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/view.py (right):

https://codereview.tryton.org/361251002/diff/419151003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:89: table, (table.inherit, index.Equality())),
On 2022/08/29 13:36:45, ced wrote:
> On 2022/06/17 16:35:32, nicoe wrote:
> > On 2022/06/09 11:55:49, ced wrote:
> > > I do not see for which queries they are.
> > > 
> > > Indeed I think it is more:
> > > 
> > > - model, inherit
> > > - id, inherit
> > 
> > The first one is for fields_view_get which will make ('model', '=',
> something).
> > But it might be redundant with your first index.
> 
> I do not understand.

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
File trytond/trytond/backend/postgresql/table.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:472:
Database.index_translators.append(cls)
On 2022/08/29 13:36:45, ced wrote:
> I find it strange to declare Indexes in TableHandler but register them on
> Database.
> For me it makes more sense to register them also on TableHandler.

Indeed Database is a singleton so it makes more sense.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:500: """
On 2022/08/29 14:47:28, ced wrote:
> I think it will be better to use () and string continuation to avoid
unnecessary
> spaces (which will make query logging difficult.

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/postgresql/table.py:585: # XXX text_pattern_ops or
varchar_pattern_ops ?
On 2022/08/29 13:36:45, ced wrote:
> as far as I can see in the PG code, there are indeed no differences.
> And quick test show that you can use varchar_pattern_ops on TEXT column.
> So for me varchar_pattern_ops is the good value.

Acknowledged.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
File trytond/trytond/backend/table.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/backen...
trytond/trytond/backend/table.py:218: lambda x:
hashlib.shake_256(x).hexdigest(6),
On 2022/08/29 13:36:45, ced wrote:
> As it is not for security, we could use shake_128 for speed.

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/res...
File trytond/trytond/ir/resource.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/res...
trytond/trytond/ir/resource.py:17: "Resource", selection='get_models',
required=True)
On 2022/08/29 13:36:45, ced wrote:
> could be a single line?

Indeed not.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/tra...
File trytond/trytond/ir/translation.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/tra...
trytond/trytond/ir/translation.py:1579: ])
On 2022/08/29 13:36:45, ced wrote:
> not in this patch

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/icon.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/icon.py:35: (table.id, index.Range(order='ASC'))))
On 2022/08/29 13:36:45, ced wrote:
> I think we could define it in SequenceOrderedMixin using order (because it is
> missing NULLS FIRST).

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/ui/...
File trytond/trytond/ir/ui/view.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/ir/ui/...
trytond/trytond/ir/ui/view.py:449: (table.user, index.Equality())))
On 2022/08/29 13:36:45, ced wrote:
> it is maybe better to put user first (as we may have more users than views).

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
File trytond/trytond/model/index.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
trytond/trytond/model/index.py:3: 
On 2022/08/29 13:36:45, ced wrote:
> I think it will make more sense to define them in modelsql.py because they can
> only be used by ModelSQL.
> 
> The Usages could be attributes of Index to keep some sort of namespaces.

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
trytond/trytond/model/index.py:21: for s, o in zip(self.expressions,
other.expressions))
On 2022/08/29 13:36:45, ced wrote:
> Is it not better to compare:
> 
> all((str(c), u) == (str(oc), ou) for (c, u), (oc, ou) in zip(...))
> 
> because the str of Usage will not contains the options.

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
File trytond/trytond/model/modelsql.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/model/...
trytond/trytond/model/modelsql.py:188: cls._indexes = set()
On 2022/08/29 13:36:45, ced wrote:
> I think it should be named: _sql_indexes

Done.

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/module...
File trytond/trytond/modules/__init__.py (right):

https://codereview.tryton.org/361251002/diff/413311003/trytond/trytond/module...
trytond/trytond/modules/__init__.py:260: (rel_column, index.Equality())))
On 2022/08/29 13:36:45, ced wrote:
> I do not like to have automatic index creation in modules/__init__.py
> It should be done in ModelSQL, probably in __post_setup__

Done.

[reviewbot, 2022-10-02 12:44:06]

ERROR: /tmp/reviewbot-03tn23vr/trytond/trytond/model/order.py Imports are
incorrectly sorted and/or formatted.

URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-05 13:41:07]

patch is not applicable
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-05 17:41:09]

patch is not applicable
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-05 18:06:46]

ERROR: /tmp/reviewbot-nqtor0tq/trytond/trytond/model/order.py Imports are
incorrectly sorted and/or formatted.

URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-05 20:33:22]

checks OK
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-06 00:07:12]

checks OK
URL: https://codereview.tryton.org/361251002

[pokoli, 2022-10-06 07:05:03]

https://codereview.tryton.org/361251002/diff/439601003/trytond/trytond/model/...
File trytond/trytond/model/fields/field.py (right):

https://codereview.tryton.org/361251002/diff/439601003/trytond/trytond/model/...
trytond/trytond/model/fields/field.py:226: domain=None, states=None,
on_change=None,
select should be also removed from doc/ref/fields.rst

https://codereview.tryton.org/361251002/diff/441511003/modules/account_statem...
File modules/account_statement/statement.py (right):

https://codereview.tryton.org/361251002/diff/441511003/modules/account_statem...
modules/account_statement/statement.py:141: where=t.state.in_(['draft',
'validated'])),
why adding the where clause on the state index?

https://codereview.tryton.org/361251002/diff/441511003/modules/stock/move.py
File modules/stock/move.py (left):

https://codereview.tryton.org/361251002/diff/441511003/modules/stock/move.py#...
modules/stock/move.py:215: readonly=True, select=True,
Why not adding an index on shipment? AFAIU we will benefit when loading the
moves of each shipment, beeing move a big table it makes sense. 
As this is not a Many2One field on the database it won't have any index so it
will produce a table scan.

[ced, 2022-10-06 08:01:47]

https://codereview.tryton.org/361251002/diff/439601003/trytond/trytond/model/...
File trytond/trytond/model/fields/field.py (right):

https://codereview.tryton.org/361251002/diff/439601003/trytond/trytond/model/...
trytond/trytond/model/fields/field.py:226: domain=None, states=None,
on_change=None,
On 2022/10/06 07:05:03, pokoli wrote:
> select should be also removed from doc/ref/fields.rst 

Done.

https://codereview.tryton.org/361251002/diff/441511003/modules/account_statem...
File modules/account_statement/statement.py (right):

https://codereview.tryton.org/361251002/diff/441511003/modules/account_statem...
modules/account_statement/statement.py:141: where=t.state.in_(['draft',
'validated'])),
On 2022/10/06 07:05:03, pokoli wrote:
> why adding the where clause on the state index? 

because there are too few possible values for a full index to be useful. The
database engine will prefer a full scan.
So having a partial index on the value with the less number of record create a
small index that the database engine should prefer to use than a full scan.

https://codereview.tryton.org/361251002/diff/441511003/modules/stock/move.py
File modules/stock/move.py (left):

https://codereview.tryton.org/361251002/diff/441511003/modules/stock/move.py#...
modules/stock/move.py:215: readonly=True, select=True,
On 2022/10/06 07:05:03, pokoli wrote:
> Why not adding an index on shipment? AFAIU we will benefit when loading the
> moves of each shipment, beeing move a big table it makes sense. 
> As this is not a Many2One field on the database it won't have any index so it
> will produce a table scan. 

As there is a One2Many using it, there is an index created for it. But I see it
creates an hash index which is not useful for '<model>,%' clause.

[reviewbot, 2022-10-06 10:09:19]

checks OK
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-06 10:32:49]

checks OK
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-06 17:09:13]

checks OK
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-07 14:04:41]

checks OK
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-07 18:38:17]

checks OK
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-08 13:37:43]

checks OK
URL: https://codereview.tryton.org/361251002

[reviewbot, 2022-10-10 22:39:48]

checks OK
URL: https://codereview.tryton.org/361251002

[rietveld-bot_tryton.org, 2022-10-10 22:54:14]

New changeset df2d8e96dbe9 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account/rev/df2d8e96dbe9

[rietveld-bot_tryton.org, 2022-10-10 22:54:18]

New changeset cda4766e92ae by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_asset/rev/cda4766e92ae

[rietveld-bot_tryton.org, 2022-10-10 22:54:26]

New changeset 2e10cc6d79c4 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_budget/rev/2e10cc6d79c4

[rietveld-bot_tryton.org, 2022-10-10 22:54:30]

New changeset 06a9d2caacab by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_consolidation/rev/06a9d2caacab

[rietveld-bot_tryton.org, 2022-10-10 22:54:40]

New changeset ace59f4c0260 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_credit_limit/rev/ace59f4c0260

[rietveld-bot_tryton.org, 2022-10-10 22:54:46]

New changeset b3c9a61a669b by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_dunning/rev/b3c9a61a669b

[rietveld-bot_tryton.org, 2022-10-10 22:54:54]

New changeset 79bed88c32ce by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_dunning_fee/rev/79bed88c32ce

[rietveld-bot_tryton.org, 2022-10-10 22:55:01]

New changeset 5ca9b1b466a7 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_es_sii/rev/5ca9b1b466a7

[rietveld-bot_tryton.org, 2022-10-10 22:55:07]

New changeset 512633c84688 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_invoice/rev/512633c84688

[rietveld-bot_tryton.org, 2022-10-10 22:55:10]

New changeset 5db66bdb80c7 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_invoice_defer/rev/5db66bdb80c7

[rietveld-bot_tryton.org, 2022-10-10 22:55:14]

New changeset 5075864c2326 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_invoice_stock/rev/5075864c2326

[rietveld-bot_tryton.org, 2022-10-10 22:55:17]

New changeset d0e5f60cf8f3 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_payment/rev/d0e5f60cf8f3

[rietveld-bot_tryton.org, 2022-10-10 22:55:21]

New changeset c83414c63fee by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_payment_braintree/rev/c83414c63fee

[rietveld-bot_tryton.org, 2022-10-10 22:55:26]

New changeset 767b0033afb9 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_payment_clearing/rev/767b0033afb9

[rietveld-bot_tryton.org, 2022-10-10 22:55:29]

New changeset 53f68e5978bd by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_payment_sepa/rev/53f68e5978bd

[rietveld-bot_tryton.org, 2022-10-10 22:55:33]

New changeset 3f805f5c02d1 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_payment_stripe/rev/3f805f5c02d1

[rietveld-bot_tryton.org, 2022-10-10 22:55:37]

New changeset 32ce93c7fe77 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_product/rev/32ce93c7fe77

[rietveld-bot_tryton.org, 2022-10-10 22:55:42]

New changeset aa78d2c53e82 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_statement/rev/aa78d2c53e82

[rietveld-bot_tryton.org, 2022-10-10 22:55:50]

New changeset 26675add0041 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_stock_landed_cost/rev/26675add0041

[rietveld-bot_tryton.org, 2022-10-10 22:55:59]

New changeset 2cefada544cf by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/account_stock_shipment_cost/rev/2cefada544cf

[rietveld-bot_tryton.org, 2022-10-10 22:56:04]

New changeset 5c89b1563670 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/analytic_account/rev/5c89b1563670

[rietveld-bot_tryton.org, 2022-10-10 22:56:13]

New changeset ffee41c793a2 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/analytic_budget/rev/ffee41c793a2

[rietveld-bot_tryton.org, 2022-10-10 22:56:21]

New changeset 52843af96d31 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/attendance/rev/52843af96d31

[rietveld-bot_tryton.org, 2022-10-10 22:56:30]

New changeset 36171fada0ef by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/authentication_sms/rev/36171fada0ef

[rietveld-bot_tryton.org, 2022-10-10 22:56:34]

New changeset 7153212594a8 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/bank/rev/7153212594a8

[rietveld-bot_tryton.org, 2022-10-10 22:56:38]

New changeset 1c365704c939 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/carrier_carriage/rev/1c365704c939

[rietveld-bot_tryton.org, 2022-10-10 22:56:50]

New changeset 31b93ab55ecd by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/carrier_weight/rev/31b93ab55ecd

[rietveld-bot_tryton.org, 2022-10-10 22:56:55]

New changeset e3b14c6a24ab by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/commission/rev/e3b14c6a24ab

[rietveld-bot_tryton.org, 2022-10-10 22:57:08]

New changeset fa19dbc863a1 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/company/rev/fa19dbc863a1

[rietveld-bot_tryton.org, 2022-10-10 22:57:16]

New changeset 2cd86bb8172e by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/country/rev/2cd86bb8172e

[rietveld-bot_tryton.org, 2022-10-10 22:57:20]

New changeset 676141321e07 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/currency/rev/676141321e07

[rietveld-bot_tryton.org, 2022-10-10 22:57:23]

New changeset 0d341963f4cf by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/customs/rev/0d341963f4cf

[rietveld-bot_tryton.org, 2022-10-10 22:57:27]

New changeset 80ead4cccdac by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/dashboard/rev/80ead4cccdac

[rietveld-bot_tryton.org, 2022-10-10 22:57:31]

New changeset 4a4f957f9e6e by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/incoterm/rev/4a4f957f9e6e

[rietveld-bot_tryton.org, 2022-10-10 22:57:34]

New changeset 979429edf5b3 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/marketing_automation/rev/979429edf5b3

[rietveld-bot_tryton.org, 2022-10-10 22:57:37]

New changeset dda2f05a83cd by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/marketing_email/rev/dda2f05a83cd

[rietveld-bot_tryton.org, 2022-10-10 22:57:50]

New changeset 32880f8ede99 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/notification_email/rev/32880f8ede99

[rietveld-bot_tryton.org, 2022-10-10 22:57:54]

New changeset c0d49f1c6391 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/party/rev/c0d49f1c6391

[rietveld-bot_tryton.org, 2022-10-10 22:58:00]

New changeset ca0cd6fcd41d by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/party_relationship/rev/ca0cd6fcd41d

[rietveld-bot_tryton.org, 2022-10-10 22:58:09]

New changeset fa8b0cc88247 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/product/rev/fa8b0cc88247

[rietveld-bot_tryton.org, 2022-10-10 22:58:16]

New changeset 0722f7a1bf45 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/product_attribute/rev/0722f7a1bf45

[rietveld-bot_tryton.org, 2022-10-10 22:58:22]

New changeset 0e6388023e13 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/product_classification/rev/0e6388023e13

[rietveld-bot_tryton.org, 2022-10-10 22:58:30]

New changeset 3310bb86ba3b by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/product_classification_taxonomic/rev/3310bb86ba3b

[rietveld-bot_tryton.org, 2022-10-10 22:58:39]

New changeset 472be8f503ad by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/product_image/rev/472be8f503ad

[rietveld-bot_tryton.org, 2022-10-10 22:58:49]

New changeset 7d91f3a44bca by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/product_kit/rev/7d91f3a44bca

[rietveld-bot_tryton.org, 2022-10-10 22:58:55]

New changeset 3035e14a0bc6 by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/product_price_list/rev/3035e14a0bc6

[rietveld-bot_tryton.org, 2022-10-10 22:59:04]

New changeset dd5f9ab0b15e by Cédric Krier in branch 'default':
Use declarative index definition for ModelSQL
https://hg.tryton.org/modules/production/rev/dd5f9ab0b15e