Skip to content

Conversation

@cawo-odoo
Copy link
Contributor

@cawo-odoo cawo-odoo commented Nov 24, 2025

[ADD] pg.query_ids: query large numbers of ids memory-safely

This is mainly the code that has been recently added to orm.recompute_fields, here we're making it re-usasble.

[IMP] orm.recompute_fields: use new pg.query_ids

This code in recompute_fields has been made re-usable in a new util pg.query_ids. Use that.

[FIX] models.remove_model: MemoryError

Traceback (most recent call last):
[...]
  File "/tmp/tmpipxrg2eq/migrations/util/models.py", line 563, in merge_model
    remove_model(cr, source, drop_table=drop_table, ignore_m2m=ignore_m2m)
  File "/tmp/tmpipxrg2eq/migrations/util/models.py", line 138, in remove_model
    it = chunks([id for (id,) in cr.fetchall()], chunk_size, fmt=tuple)
MemoryError

@robodoo
Copy link
Contributor

robodoo commented Nov 24, 2025

Pull request status dashboard

@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch from 568311e to ac8c6d0 Compare November 24, 2025 14:47
@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch from ac8c6d0 to ffc872a Compare November 24, 2025 15:01
@KangOl
Copy link
Contributor

KangOl commented Nov 24, 2025

upgradeci retry with always only crm hr

@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch from 6aa7709 to 543204d Compare November 24, 2025 21:11
@cawo-odoo
Copy link
Contributor Author

so, this avoids the memoryerror in upg-3307099 just fine

@cawo-odoo cawo-odoo marked this pull request as ready for review November 25, 2025 14:29
@cawo-odoo cawo-odoo requested review from a team and asno-odoo November 25, 2025 14:29
Copy link
Contributor

@aj-fuentes aj-fuentes left a comment

Choose a reason for hiding this comment

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

I think we could we follow the plan of "providing an object that would iterate ids and do the cleanup as early as possible". The cleanup would happen at iteration finished, __exit__, or __del__. Whatever happens first.

Technically:

  • The class is primarily an iterator, once exhausted it will do all the cleanup.
  • We guard against double iteration. (this pattern already exists in the utils)
  • We double down the cleanup at __del__ just in case.
  • We triple down the cleanup at __exit__ to leverage the context manager semantic. We don't need to __enter__ into _ncr. Current dummy __enter__ returning self is just fine.

Feel free to improve the stub suggestions below if you agree.

@cawo-odoo
Copy link
Contributor Author

cawo-odoo commented Nov 26, 2025

I pushed a version inspired by your suggestions, but I didn't use them verbatim. Please resolve conversations when you feel they have been addressed.

2 explanations up-front:

  • It does not work to raise a real exception from __iter__ when the cursor is closed, because util.chunks works that way that it needs to catch StopIteration twice before it really stops iterating.
  • afaik, returning a value from __del__ is useless, it is ignored always

@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch 4 times, most recently from 8017ffb to 3b42f99 Compare November 27, 2025 13:50
@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch 4 times, most recently from 610edfa to 6f069fb Compare November 27, 2025 20:56
@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch 2 times, most recently from fbe3bc3 to 76ae21e Compare November 28, 2025 07:54
@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch from 76ae21e to a11d862 Compare November 28, 2025 15:19
@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch 5 times, most recently from 1d65991 to 894c027 Compare December 2, 2025 12:50
@cawo-odoo cawo-odoo requested a review from aj-fuentes December 2, 2025 14:51
Copy link
Contributor

@aj-fuentes aj-fuentes left a comment

Choose a reason for hiding this comment

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

I forgot to post this yesterday.

Some documentation nits.

@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch 2 times, most recently from 24afb9b to af35535 Compare December 5, 2025 06:35
This is mainly the code that has been recently added to `orm.recompute_fields`,
here we're making it re-usasble.
This code in recompute_fields has been made re-usable in a new util
pg.query_ids. Use that.
```
Traceback (most recent call last):
[...]
  File "/tmp/tmpipxrg2eq/migrations/util/models.py", line 563, in merge_model
    remove_model(cr, source, drop_table=drop_table, ignore_m2m=ignore_m2m)
  File "/tmp/tmpipxrg2eq/migrations/util/models.py", line 138, in remove_model
    it = chunks([id for (id,) in cr.fetchall()], chunk_size, fmt=tuple)
MemoryError
```

Some IR tables can be large. Avoid `cr.fetchall()` when getting ids by use of
pg.query_ids()
@cawo-odoo cawo-odoo force-pushed the master-imp_query_ids_server_side-cawo branch from af35535 to 30020ce Compare December 5, 2025 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants