Project

General

Profile

Development #57017

Locker quelque chose pendant les modifications de schéma

Added by Benjamin Dauvergne about 1 month ago. Updated 28 days ago.

Status:
Solution proposée
Priority:
Normal
Target version:
-
Start date:
16 Sep 2021
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

Au début de chaque point d'entré1 dans les méthodes de migration (migrate, do_formdef_tables appelé par FormDef.data_class()) il faudrait locker un table globale, pour s'assurer qu'on ne migre pas dans plusieurs processus à la fois, peut-être pg_catalog.pg_class.

1

$ git grep 'sql\.[a-z]\w*(' wcs  | grep -v get_conn | grep -v 'totals\|count\|get_field_id\|get_carddef_new\|wipe\|new_id\|cleanu'
wcs/admin/settings.py:            sql.do_user_table()
wcs/carddef.py:            table_name = sql.get_formdef_table_name(self)
wcs/carddef.py:            actions = sql.do_formdef_tables(self)
wcs/ctl/management/commands/convert_to_sql.py:            sql.do_formdef_tables(formdef, rebuild_views=True, rebuild_global_views=True)
wcs/formdef.py:            self.table_name = sql.get_formdef_table_name(self)
wcs/formdef.py:            sql.do_global_views(conn, cur)
wcs/formdef.py:            table_name = sql.get_formdef_table_name(self)
wcs/formdef.py:            actions = sql.do_formdef_tables(self)
wcs/formdef.py:            sql.do_formdef_tables(self, rebuild_views=True, rebuild_global_views=True)
wcs/formdef.py:            sql.do_formdef_tables(self, rebuild_views=True, rebuild_global_views=True)
wcs/publisher.py:        sql.do_session_table()
wcs/publisher.py:        sql.do_user_table()
wcs/publisher.py:        sql.do_role_table()
wcs/publisher.py:        sql.do_tracking_code_table()
wcs/publisher.py:        sql.do_custom_views_table()
wcs/publisher.py:        sql.do_snapshots_table()
wcs/publisher.py:        sql.do_loggederrors_table()
wcs/publisher.py:        sql.do_meta_table()
wcs/publisher.py:        sql.drop_views(None, conn, cur)
wcs/publisher.py:            sql.do_formdef_tables(_formdef)
wcs/publisher.py:        sql.migrate_global_views(conn, cur)
wcs/publisher.py:        sql.migrate()
wcs/publisher.py:        sql.reindex()


Files


Related issues

Related to w.c.s. - Development #57118: faire les modifications de schéma à la sauvegarde de formdef/etc. plutôt qu'au moment du data_class()Solution déployée20 Sep 2021

Actions

History

#2

Updated by Benjamin Dauvergne about 1 month ago

  • Assignee set to Benjamin Dauvergne
#3

Updated by Benjamin Dauvergne about 1 month ago

#4

Updated by Benjamin Dauvergne about 1 month ago

C'est pylint qui fait des siennes.

#5

Updated by Frédéric Péters about 1 month ago

do_formdef_tables (particulièrement) est le plus souvent appelé sans action de modification derrière; y aurait-il possibilité de limiter l'envoi du LOCK uniquement en cas de modification ? (cela écrit sans aucune idée du coût de l'appel).

#7

Updated by Frédéric Péters about 1 month ago

Je ne capte pas comment on passe du premier patch, qui lance juste "LOCK wcs_meta" peut-être un peu trop souvent (c'est mon commentaire en tout cas) au deuxième patch, avec décorateur et un truc de profondeur et de réinitialisation de lock.

Je ne sais pas si en soit le premier patch était trop naïf et ne marchait en fait pas, ou si le deuxième a une complexité inexpliquée inutile.

~~

À tester de mon côté, déjà le comportement : le second LOCK il fait attendre, il ne lève pas d'erreur etc. ça me laisse penser que le premier patch serait fonctionnellement ok.

Sur la forme par contre, je n'étais donc pas fan, mais peut-être encoe moins fan du deuxième.

Mon approche alternative (poussée dans une branche pas testée plus que ça, pour au moins avoir l'idée), ce serait de détecter les moments de modifications de schéma au niveau du curseur psycopg2, qqch comme ça :

+class LockableCursor(psycopg2.extensions.cursor):
+    locked = False
+
+    def execute(self, sql, args=None):
+        if not self.locked and (
+            sql.startswith('ALTER') or sql.startswith('CREATE') or sql.startswith('DROP')
+        ):
+            # hold a lock (on wcs_meta as it will be the first table we create) during schema
+            # changes; this should avoid concurrent operations and potential deadlocks.
+            super().execute('LOCK TABLE wcs_meta')
+            self.locked = True
+        super().execute(sql, args)
+
+
 def get_connection_and_cursor(new=False):
     conn = get_connection(new=new)
     try:
-        cur = conn.cursor()
+        cur = conn.cursor(cursor_factory=LockableCursor)

C'est isolé, ça évite d'avoir à penser à poser des lock_schemas() ou @guard_lock et lock().

#8

Updated by Benjamin Dauvergne about 1 month ago

  • Status changed from Solution proposée to Information nécessaire

Frédéric Péters a écrit :

Je ne capte pas comment on passe du premier patch, qui lance juste "LOCK wcs_meta" peut-être un peu trop souvent (c'est mon commentaire en tout cas) au deuxième patch, avec décorateur et un truc de profondeur et de réinitialisation de lock.

Oui j'aurai du expliquer; si on lock en début de transaction, i.e. avant le code de lecture qui décide ou pas de modifier le schéma, on a pas de souci mais effectivement on perd en performance parce qu'on lock tout le temps et donc on sérialise tous chemins du code qui ont besoin d'appeler do_formdef_tables; mais donc si on décide de ne locker qu'au moment des modifications de schéma on se trouve avec une race condition entre les transactions, car une fois que le lock se libère on est peut-être plus dans une situation qui demande la modification du schéma. Exemple : j'ai deux do_formdef_tables qui se lancent en paralèlle pour ajouter la colonne f2, le premier obtient le lock immédiatement et ajoute la colonne le deuxième détecte qu'il faut ajouter la colonne arrive sur le lock_schema() attend que le premier finisse puis tente d'ajouter la colonnne une deuxième fois; ça lève une erreur.

Quand on lock au dernier moment on doit redémarrer la transaction complète au début (avec les vérifications dans information_schema du besoin de modifier le schéma) pour ne pas se retrouver dans cette situation.

Je ne sais pas si en soit le premier patch était trop naïf et ne marchait en fait pas, ou si le deuxième a une complexité inexpliquée inutile.

Si les deux marchent mais ça dépend si on cherche la simplicité ou la performance (pseudo-performance parce qu'on a pas mesuré l'impact réel de locker en début de transaction).

~~

À tester de mon côté, déjà le comportement : le second LOCK il fait attendre, il ne lève pas d'erreur etc. ça me laisse penser que le premier patch serait fonctionnellement ok.

Oui je pense qu'il l'était aussi, j'ai juste voulu répondre à ton inquiétude sur les performances de do_formdef_tables.

Sur la forme par contre, je n'étais donc pas fan, mais peut-être encore moins fan du deuxième.

Mon approche alternative (poussée dans une branche pas testée plus que ça, pour au moins avoir l'idée), ce serait de détecter les moments de modifications de schéma au niveau du curseur psycopg2, qqch comme ça :

[...]

C'est isolé, ça évite d'avoir à penser à poser des lock_schemas() ou @guard_lock et lock().

Oui mais ça a le problème dont je parle plus haut, ça ne résout pas les soucis de concurrence entre workers, mais par contre je peux combiner nos deux patchs pour éviter de saupoudrer le code de lock() un peu partout, voir garder l'approche lock en début de transaction pour la plupart des fonctions et n'utiliser guard_lock que pour do_formdef_tables et do_global_views qui semblent les deux seuls méthodes de modification des schémas qui sont appelées un peu n'importe quand.

#9

Updated by Frédéric Péters about 1 month ago

que pour do_formdef_tables et do_global_views qui semblent les deux seuls méthodes de modification des schémas qui sont appelées un peu n'importe quand.

Pour do_formdef_tables je vois assez, je vais faire un ticket dédié, je serais plutôt pour commencer par réduire ça. (et plutôt automatiquement ça devrait libérer ce besoin de locker, il me semble).

#10

Updated by Frédéric Péters about 1 month ago

  • Related to Development #57118: faire les modifications de schéma à la sauvegarde de formdef/etc. plutôt qu'au moment du data_class() added
#11

Updated by Frédéric Péters about 1 month ago

Pour do_formdef_tables je vois assez, je vais faire un ticket dédié, je serais plutôt pour commencer par réduire ça. (et plutôt automatiquement ça devrait libérer ce besoin de locker, il me semble).

#57118 et je pense qu'avec ça, si on veut locker, ça peut juste être un cur.execute('LOCK wcs_meta') exécuté en haut de do_formdef_tables.

#12

Updated by Benjamin Dauvergne 28 days ago

Voilà, lock posé uniquement sur migrate (pour éviter que ça se marche sur les pieds avec do_formdef_table sait-on jamais) et do_formdef_tables.

Also available in: Atom PDF