Development #66816
Race condition possible dans le stockage sql des "transient data"
0%
Description
En regardant les commits récemment apparus dans main, j'observe le code suivant (#66620):
@guard_postgres
def store(self):
sql_dict = {
'id': self.id,
'session_id': self.session_id,
'data': bytearray(pickle.dumps(self.data, protocol=2)),
'last_update_time': now(),
}
conn, cur = get_connection_and_cursor()
column_names = sql_dict.keys()
sql_statement = '''UPDATE %s SET %s WHERE id = %%(id)s RETURNING id''' % (
self._table_name,
', '.join(['%s = %%(%s)s' % (x, x) for x in column_names]),
)
cur.execute(sql_statement, sql_dict)
if cur.fetchone() is None:
sql_statement = '''INSERT INTO %s (%s) VALUES (%s)''' % (
self._table_name,
', '.join(column_names),
', '.join(['%%(%s)s' % x for x in column_names]),
)
cur.execute(sql_statement, sql_dict)
conn.commit()
cur.close()
Ce code souffre d'au moins une race condition, qu'on peut heureusement régler facilement tout en le simplifiant grâce à l'UPSERT introduit dans PostgreSQL 9.5. Si deux instances veulent stocker la donnée en même temps, les deux instances verront leur update échouer, et l'une d'elles aura un insert qui échouera.
Il suffit de remplacer le code SQL par un insert on conflict update pour éviter tout de suite ce bug, et en prime simplifier le code et réduire le nombre de requêtes.
Fichiers
Révisions associées
Historique
Mis à jour par Frédéric Péters il y a presque 2 ans
À noter que cette construction se trouve répandue, si une correction pouvait simplifier globalement plutôt qu'introduire une manière de faire particulière pour ces objets particuliers, ça serait top.
Mis à jour par Frédéric Péters il y a presque 2 ans
Je vois maintenant le patch de la branche et je me dis qu'on peut en fait y aller avec juste ça, cette classe a quand même la particularité d'un id random attribué côté python, plutôt que les séquences postgresql qu'on trouve ailleurs.
Mis à jour par Pierre Ducroquet il y a presque 2 ans
- Fichier 0001-sql-use-insert-on-conflict-to-store-transient-data-6.patch 0001-sql-use-insert-on-conflict-to-store-transient-data-6.patch ajouté
- Statut changé de Nouveau à Solution proposée
- Patch proposed changé de Non à Oui
Du coup je propose le patch. J'ai pas vu d'autres endroits évidents à changer, mais je garde ça en tête si je vois un schéma se répéter...
Mis à jour par Frédéric Péters il y a presque 2 ans
- Statut changé de Solution proposée à Solution validée
Mis à jour par Pierre Ducroquet il y a presque 2 ans
- Statut changé de Solution validée à Résolu (à déployer)
Mergé
commit fa129af6c994801b4057a3bde94e211c2580cd08 (HEAD -> main, origin/main, origin/HEAD, wip/66816-possible-racecondition)
Author: Pierre Ducroquet <pducroquet@entrouvert.com>
Date: Thu Jun 30 13:59:31 2022 +0200
sql: use insert on conflict to store transient data (#66816)
Mis à jour par Transition automatique il y a presque 2 ans
- Statut changé de Résolu (à déployer) à Solution déployée
sql: use insert on conflict to store transient data (#66816)