Skip to content

Conversation

@rco-odoo
Copy link
Member

Consider a recordset of new records, and a loop like

for record in records:
    for line in record.line_ids:
        line.value

The implementation of record.line_ids does not actually prefetch all records. It actually fetches the field on the records' origin (their corresponding real records), but only assigns the current new record in cache. As the prefetching relies on the cached values of line_ids, the prefetching mechanism is actually broken on line.

The fix consists in assigning all the records to prefetch in this case. This does not add unexpected prefetching, since the origin records are prefetched as one batch anyway.

@robodoo
Copy link
Contributor

robodoo commented Feb 18, 2025

Pull request status dashboard

@C3POdoo C3POdoo requested review from a team, HydrionBurst and xmo-odoo and removed request for a team February 18, 2025 12:29
@C3POdoo C3POdoo added the RD research & development, internal work label Feb 18, 2025
Copy link
Contributor

@ryv-odoo ryv-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I have just two small comments/questions.

odoo/fields.py Outdated
Comment on lines +1203 to +1206
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for rec in recs:
if rec._origin:
value = self.convert_to_cache(rec._origin[self.name], rec, validate=False)
env.cache.set(rec, self, value)
for rec in recs:
if (origin_rec := rec._origin):
value = self.convert_to_cache(origin_rec[self.name], rec, validate=False)
env.cache.set(rec, self, value)

Maybe to avoid recreating rec._origin twice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odoo 16 requires Python 3.7 minimum, and the walrus operator has been introduced in Python 3.8.
I will adapt this in forward-ports.

Copy link
Contributor

@william-andre william-andre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on my original issue in 18.0 and it works as expected 👍

Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.
@rco-odoo rco-odoo force-pushed the 16.0-prefetch-new-lines-rco branch from 3e31c0b to d2fac4f Compare February 18, 2025 15:15
@rco-odoo
Copy link
Member Author

@robodoo r+

robodoo pushed a commit that referenced this pull request Feb 18, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes #198124

Signed-off-by: Raphael Collet <rco@odoo.com>
@robodoo robodoo closed this Feb 18, 2025
@fw-bot fw-bot deleted the 16.0-prefetch-new-lines-rco branch March 4, 2025 17:22
gurneyalex pushed a commit to camptocamp/odoo that referenced this pull request Mar 26, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
gurneyalex pushed a commit to camptocamp/odoo that referenced this pull request Mar 31, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
gurneyalex pushed a commit to camptocamp/odoo that referenced this pull request Mar 31, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
gurneyalex pushed a commit to camptocamp/odoo that referenced this pull request Apr 3, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
gurneyalex pushed a commit to camptocamp/odoo that referenced this pull request Apr 3, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
santostelmo pushed a commit to camptocamp/odoo that referenced this pull request Apr 4, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
santostelmo pushed a commit to camptocamp/odoo that referenced this pull request Apr 14, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
santostelmo pushed a commit to camptocamp/odoo that referenced this pull request Apr 16, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
santostelmo pushed a commit to camptocamp/odoo that referenced this pull request Jun 5, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
santostelmo pushed a commit to camptocamp/odoo that referenced this pull request Jun 20, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
santostelmo pushed a commit to camptocamp/odoo that referenced this pull request Jun 20, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
santostelmo pushed a commit to camptocamp/odoo that referenced this pull request Oct 9, 2025
Consider a recordset of new records, and a loop like

    for record in records:
        for line in record.line_ids:
            line.value

The implementation of `record.line_ids` does not actually prefetch all
`records`.  It actually fetches the field on the records' origin (their
corresponding real records), but only assigns the current new record in
cache.  As the prefetching relies on the cached values of `line_ids`,
the prefetching mechanism is actually broken on `line`.

The fix consists in assigning all the records to prefetch in this case.
This does not add unexpected prefetching, since the origin records are
prefetched as one batch anyway.

closes odoo#198124

Signed-off-by: Raphael Collet <rco@odoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RD research & development, internal work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants