Support Multi-DB-Backends for LibCache

[sgjzfzzf]

PR Category

Other

Type of Change

New Feature

Description

This PR supports multi-sql-backends for libcache. It's powered by sqlalchemy. The previous SQL backend is sqlite3, which doesn't support multi-writers at the same time, and may lead to some writing conflicts in the multi-workers cases. By introducing PostgreSQL as an available option for libcache, which supports several writings at the same time, it could solve the problem.

The details are recored in the document docs/libcache_db_backend.md. Please refer to it for more details.

Issue

Progress

• Change is properly reviewed (1 reviewer required, 2 recommended).
• Change is responded to an issue.
• Change is fully covered by a UT.

Performance

[sgjzfzzf]

Hi, could you please provide us with the test script you're using here? It looks like a database problem with duplicated primary keys. @ArashiFeng

[ArashiFeng]

Hi, could you please provide us with the test script you're using here? It looks like a database problem with duplicated primary keys. @ArashiFeng

Sorry that I can't provide the test script because it's for internal testing. In short, this problem is caused by this line https://github.com/FlagOpen/FlagGems/blob/11efecfcfa04c8c6d509adf04a4a067f1e6da524/src/flag_gems/utils/models/sql.py#L176.
INSERT OR REPLACES(https://github.com/FlagOpen/FlagGems/blob/master/src/flag_gems/utils/libentry.py#L258) can replace data when primary keys are detected duplicated. But session.add() cannot handle this.
Change to following code can deal with postgresql on conflict.

  from sqlalchemy.dialects.postgresql import insert
  obj: Base = ConfigCls(**key_dict, **config)
  cmd = insert(ConfigCls).values(**key_dict, **config)
  primary_keys = [col for col in ConfigCls.__table__.primary_key.columns]
  update_fields = {k: getattr(cmd.excluded, k) for k in key_dict.keys() if k not in [c.name for c in primary_keys]}
  update_fields.update({k: getattr(cmd.excluded, k) for k in config.keys() if k not in [c.name for c in primary_keys]})
  cmd = cmd.on_conflict_do_update(index_elements=primary_keys, set_=update_fields)
  session.execute(cmd)
  session.commit()

The following code may also work, but not validated.

  with Session(engine) as session:
    obj = session.get(ConfigCls, key_dict["xxx"])
    if obj is None:
        obj = ConfigCls(**key_dict, **config)
        session.add(obj)
    else:
        for k, v in config.items():
            setattr(obj, k, v)
    session.commit()

[sgjzfzzf]

Hi, could you please provide us with the test script you're using here? It looks like a database problem with duplicated primary keys. @ArashiFeng

Sorry that I can't provide the test script because it's for internal testing. In short, this problem is caused by this line

https://github.com/FlagOpen/FlagGems/blob/11efecfcfa04c8c6d509adf04a4a067f1e6da524/src/flag_gems/utils/models/sql.py#L176

.
INSERT OR REPLACES(https://github.com/FlagOpen/FlagGems/blob/master/src/flag_gems/utils/libentry.py#L258) can replace data when primary keys are detected duplicated. But session.add() cannot handle this.
Change to following code can deal with postgresql on conflict.

  from sqlalchemy.dialects.postgresql import insert
  obj: Base = ConfigCls(**key_dict, **config)
  cmd = insert(ConfigCls).values(**key_dict, **config)
  primary_keys = [col for col in ConfigCls.__table__.primary_key.columns]
  update_fields = {k: getattr(cmd.excluded, k) for k in key_dict.keys() if k not in [c.name for c in primary_keys]}
  update_fields.update({k: getattr(cmd.excluded, k) for k in config.keys() if k not in [c.name for c in primary_keys]})
  cmd = cmd.on_conflict_do_update(index_elements=primary_keys, set_=update_fields)
  session.execute(cmd)
  session.commit()

The following code may also work, but not validated.

  with Session(engine) as session:
    obj = session.get(ConfigCls, key_dict["xxx"])
    if obj is None:
        obj = ConfigCls(**key_dict, **config)
        session.add(obj)
    else:
        for k, v in config.items():
            setattr(obj, k, v)
    session.commit()

Hi thank you for your reply and comment! I'll try this solution later. My machine is broken so I need to wait for the repair😭. I'll let you know when there is any new update.

[sgjzfzzf]

@ArashiFeng Hi Arashi! Please take a look at the latest commit. We replaced sqlalchemy.orm.Session with RollbackSession, so it could handle the exception and rollback automatically when unique primary key constraints are broken. Please feel free to test it and let me know if there is any more problem!

[sgjzfzzf]

Btw, sqlalchemy seems to support sqlite3 in multi-connections case, so please also test it with sqlite3 if it's convenient for you. Thx!

[ArashiFeng]

Btw, sqlalchemy seems to support sqlite3 in multi-connections case, so please also test it with sqlite3 if it's convenient for you. Thx!

Thanks. I'll try the latest commit later.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[ArashiFeng]

Btw, sqlalchemy seems to support sqlite3 in multi-connections case, so please also test it with sqlite3 if it's convenient for you. Thx!

We tested sqlalchemy with sqlite3, seems it also resolve the multi-connections problems in our tests, while the old version will encounterring the sqllite3.OperationError(database is locked) at a very high possibilities.

[sgjzfzzf]

Btw, sqlalchemy seems to support sqlite3 in multi-connections case, so please also test it with sqlite3 if it's convenient for you. Thx!

We tested sqlalchemy with sqlite3, seems it also resolve the multi-connections problems in our tests, while the old version will encounterring the sqllite3.OperationError(database is locked) at a very high possibilities.

Sounds great! If it looks good to you we'll plan to merge it into the main stream after some tests. Please let us know if there is any other problem!