[IMP] pg,orm,models: query ids server side

[cawo-odoo]

[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

[IMP] pg.named_cursor: lessons learned from pg.query_ids

• it is not exactly right to call into the Cursor's __enter__ and __exit__ methods
• Cursor should be explicitly closed before it is garbage collected

[robodoo]

[KangOl]

upgradeci retry with always only crm hr

[cawo-odoo]

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

[aj-fuentes]

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]

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

[aj-fuentes]

Now that we ensure both are assigned, we can reorder for simplicity?

Suggested change

	if self._ncr:
	if self._ncr.closed:
	return
	self._ncr.close()
	self._cr.execute(format_query(self._cr, "DROP TABLE IF EXISTS {}", self._tmp_tbl))
	self._cr.execute(format_query(self._cr, "DROP TABLE IF EXISTS {}", self._tmp_tbl))
	if self._ncr and not self._ncr.closed:
	self._ncr.close()

[cawo-odoo]

I would prefer not running the DROP TABLE when we know it has already been done.

[aj-fuentes]

I think it's safer to run it always. It won't depend on any logic to "know that we have already done it". Leaking that table is worse than a noop DROP TABLE. But I'm OK with both options.

[aj-fuentes]

Leave this to the caller. In almost all cases we won't continue under this error and just fail the whole upgrade. The idea of wrapping the error is just to make it simpler to debug what failed.

Suggested change

cr.rollback()

[cawo-odoo]

I.e. we will not prevent further "broken transaction" exceptions from happening (e.g. from __del__? Then I really don't see the point in catching this here.

[aj-fuentes]

(note that there is a typo, missing call to .format(query))

Then I really don't see the point in catching this here.

The point is the error message, for humans.
Compare:

Traceback (most recent call last):
  File "<input>", line 1, in <module>
    util.query_ids(self.env.cr, "SELECT * FROM (VALUES (1), (1)) AS x(x)")
  File "/home/odoo/src/upgrade/migrations/util/pg.py", line 1979, in __init__
    raise ValueError("The query for ids is producing duplicate values: {}".format(query))
ValueError: The query for ids is producing duplicate values: SELECT * FROM (VALUES (1), (1)) AS x(x)

vs

Traceback (most recent call last):
  File "<input>", line 1, in <module>
    util.query_ids(self.env.cr, "SELECT * FROM (VALUES (1), (1)) AS x(x)")
  File "/home/odoo/src/upgrade/migrations/util/pg.py", line 1969, in __init__
    cr.execute(
  File "/home/odoo/src/odoo/16.0/odoo/sql_db.py", line 324, in execute
    res = self._obj.execute(query, params)
psycopg2.errors.UniqueViolation: could not create unique index "pk__upgrade_query_ids_d321bc27483a4d24a8a79f4e8a4ca920_id"
DETAIL:  Key (id)=(1) is duplicated.

The latter leaks internal info that's hard to understand. What's pk__upgrade_query_ids_d321bc27483a4d24a8a79f4e8a4ca920_id? What's that weird table name?

[aj-fuentes]

OK I see what you mean:

Traceback (most recent call last):
  File "/home/odoo/src/upgrade/migrations/util/pg.py", line 2018, in __del__
    self._close()
  File "/home/odoo/src/upgrade/migrations/util/pg.py", line 1990, in _close
    self._cr.execute(format_query(self._cr, "DROP TABLE IF EXISTS {}", self._tmp_tbl))
  File "/home/odoo/src/odoo/16.0/odoo/sql_db.py", line 327, in execute
    _logger.error("bad query: %s\nERROR: %s", tools.ustr(self._obj.query or query), e)
  File "/home/odoo/src/odoo/16.0/odoo/sql_db.py", line 485, in __getattr__
    raise psycopg2.InterfaceError("Cursor already closed")
psycopg2.InterfaceError: Cursor already closed

That can be solved by checking in __del__ if the cursor was closed

if not self._cr.closed:
   # drop table...

[aj-fuentes]

I don't have a decisive opinion against the rollback call... it looks odd but I'll let @KangOl provide further advice.