search

OCA / openupgradelib

A library with support functions to be called from Odoo migration scripts.
GNU Affero General Public License v3.0
85 stars 171 forks source link

chunked: should it flush ? #362

Closed ivantodorovich closed 4 months ago

ivantodorovich commented 5 months ago

The chunked method is invalidating the cache on each iteration, but it's never flushing.

I'm encountering an issue with this in Odoo 16.0, where by using chunked I get into CacheMiss errors due to the cache being lost, including the previous one before it's being called ⚠️ .

By looking at the code I see it's invalidating the cache but it's never flushing. This looks like a red flag to me, although I've never encountered any issues with chunked before.

Perhaps it's due to https://github.com/odoo/odoo/pull/66938

pedrobaeza commented 5 months ago

chunked was programmed a long time ago, and it's not adapted to newer versions.

ivantodorovich commented 5 months ago

Ouch, that explains it then 😢 I'm worried now about all the other openupgradelib's method I used in the past 😓

💡 Perhaps we could have a warning system to manually whitelist methods for new versions

pedrobaeza commented 5 months ago

Well, this is the only method where it mixes ORM. The rest is thought for handling database things, so I don't think the rest are affected.

StefanRijnhart commented 5 months ago

It's a good question, and I don't think anybody explicitly checked this after 16.0 was released, but invalidate_all actually executes a flush by default: https://github.com/odoo/odoo/blob/16.0/odoo/api.py#L727-L736

ivantodorovich commented 5 months ago

It's a good question, and I don't think anybody explicitly checked this after 16.0 was released, but invalidate_all actually executes a flush by default: https://github.com/odoo/odoo/blob/16.0/odoo/api.py#L727-L736

Indeed it does, since: https://github.com/odoo/odoo/pull/66938

The thing is we've stopped calling invalidate_all for version > 10 (we call env.cache.invalidate instead).

The fix could be to simply go back to calling env.invalidate_all for version >= 16.

However I'm wondering if we should explicitly call flush for version >= 12 instead, when flush was introduced. By reading the code, IMO it's a miracle that it works. I can't explain why the data is not lost in versions 12, 13, 14 and 15; there must be something else that's flushing and saving it. The intention of this method is clearly to flush our changes to the db, whilst clearing the cache before the next iteration.

StefanRijnhart commented 5 months ago

@ivantodorovich Thanks for clarifying! I think it's a very good idea of yours to go back to invalidate_all now that it is reintroduced, and to flush explicitly for the earlier versions that support it.

ivantodorovich commented 4 months ago

I've created this PR with a fix proposal: