Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad performance for SQLAlchemy >= 1.4 due to disabled caching #436

Open
lonvia opened this issue Mar 31, 2023 · 1 comment
Open

Bad performance for SQLAlchemy >= 1.4 due to disabled caching #436

lonvia opened this issue Mar 31, 2023 · 1 comment

Comments

@lonvia
Copy link

lonvia commented Mar 31, 2023

#338 has disable caching for all Geometry and Geography types. This effectively disables SQLAlchemy's caching mechanism for anything where geometry columns are involved.

Take this simple table:

import sqlalchemy as sa
from geoalchemy2 import Geometry

t = sa.Table('test', sa.MetaData(),
             sa.Column('id', sa.BigInteger, nullable=False, unique=True),
             sa.Column('centroid', Geometry(srid=4326, spatial_index=False)))

Selecting the full table will create a cache key:

>>> sa.select(t)._generate_cache_key()
CacheKey(key=(
  '0', 
  <class 'sqlalchemy.sql.selectable.Select'>, 
  '_raw_columns', 
  (
    (
      <Table object at 0x7fb2689d2390>, 
    ),
  ),
  '_label_style', 
  symbol('LABEL_STYLE_DISAMBIGUATE_ONLY'), 
  '_compile_options', 
  (
    <class 'sqlalchemy.sql.selectable.SelectState.default_select_compile_options'>, 
    ()
  ),
),)

while only selecting the geometry column does not:

>>> sa.select(t.c.centroid)._generate_cache_key()
None

If I change Geometry to cache_ok=True, then I measure about 20% performance gain in my application, so the difference is fairly significant.

@adrien-berchet
Copy link
Member

Hi @lonvia
Thanks for this report!
Indeed, I was not sure how the new cache mechanism would behave so I preferred to disable it (and I did not benchmarked this, 20% is indeed a lot 😕). And re-enabling it actually breaks a test, so I think we should keep it disabled for now. But at a first glance, it seems to me that the failure is caused by an almost impossible state which consists in passing a WKB fetched from a Postgres DB to a MySQL DB. I would have liked to be able to switch from one dialect to another with the same WKB/WKT elements but maybe it's a bit too ambitious. I will think about this...
In the mean time, if you want to propose a patch for this I will be happy to review it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants