entrouvert/wcs
entrouvert
/
wcs
14
1
Fork 0
Code Issues Pull Requests 34 Releases Activity

sql: switch to autocommit (#81133) #667

Merged
fpeters merged 4 commits from wip/81133-psycopg-autocommit into main 2023-11-24 15:05:03 +01:00
Conversation 6 Commits 4 Files Changed 12 +444 -509
pducroquet commented 2023-09-13 10:26:33 +02:00
Owner
Copy Link
  • passage de autocommit=False à autocommit=True, pour que psycopg ne fasse plus de transactions implicites
  • retrait de tous les conn.commit() répartis dans tout le code
  • introduction d'un décorateur @atomic pour gérer les transactions et sous-transactions

Le gain en perfs sera négligeable, mais au moins on n'aura plus le risque d'un .commit() oublié, et pgbouncer devrait mieux faire son travail...

- passage de autocommit=False à autocommit=True, pour que psycopg ne fasse plus de transactions implicites - retrait de tous les conn.commit() répartis dans tout le code - introduction d'un décorateur @atomic pour gérer les transactions et sous-transactions Le gain en perfs sera négligeable, mais au moins on n'aura plus le risque d'un .commit() oublié, et pgbouncer devrait mieux faire son travail...
pducroquet added 1 commit 2023-09-13 10:26:34 +02:00
gitea/wcs/pipeline/head There was a failure building this commit Details
3aa61a31b9 PoC: first pass for switching to autocommit
pducroquet force-pushed wip/81133-psycopg-autocommit from 3aa61a31b9 to 52d8590d3a 2023-09-13 10:31:16 +02:00 Compare
pducroquet added 1 commit 2023-09-13 10:52:44 +02:00
gitea/wcs/pipeline/head There was a failure building this commit Details
3bbca041d2 workaround
pducroquet added 1 commit 2023-09-13 11:18:19 +02:00
gitea/wcs/pipeline/head There was a failure building this commit Details
57ef9c1bc5 temp workaround for another lock
pducroquet added 1 commit 2023-09-13 12:06:13 +02:00
gitea/wcs/pipeline/head There was a failure building this commit Details
103795643d fix one order error
pducroquet force-pushed wip/81133-psycopg-autocommit from 103795643d to 9643572884 2023-10-03 10:48:33 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 9ff33c766b to f1ff6a9548 2023-10-04 13:43:38 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from f1ff6a9548 to f81abc4d6d 2023-10-04 14:21:48 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from f81abc4d6d to 66be798871 2023-10-04 14:31:41 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 66be798871 to 420b0e428b 2023-10-04 14:35:03 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 420b0e428b to 030dd64fa5 2023-10-04 15:10:58 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 030dd64fa5 to 1e56c8879e 2023-10-04 15:29:47 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 1e56c8879e to 3e0f038227 2023-10-04 15:45:02 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 3e0f038227 to 23a5be501d 2023-10-04 16:03:38 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 23a5be501d to 3f4136c10c 2023-10-04 16:20:12 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 3f4136c10c to 485138b5c0 2023-10-04 16:35:20 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 876c89c5a9 to aa8323561f 2023-10-04 17:28:20 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from eb8b72fd97 to 1872c9b03d 2023-10-04 19:43:16 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 1872c9b03d to 584b988cf9 2023-10-04 19:59:51 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 5637972826 to d853c73071 2023-10-04 22:01:40 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from ab8894a060 to 84a01f4ad4 2023-10-05 20:10:39 +02:00 Compare
pducroquet commented 2023-10-05 20:14:15 +02:00
Author
Owner
Copy Link

Après beaucoup de bagarre avec psycopg2, j'ai lu son code source et j'ai pu enfin comprendre ce qui ne marchait pas.
Du coup ça devrait passer les tests unitaires maintenant, j'ai introduit un @atomic pour pouvoir automatiser les transactions (et surtout les sous-transactions avec des savepoints)...

Après beaucoup de bagarre avec psycopg2, j'ai lu son code source et j'ai pu enfin comprendre ce qui ne marchait pas. Du coup ça devrait passer les tests unitaires maintenant, j'ai introduit un @atomic pour pouvoir automatiser les transactions (et surtout les sous-transactions avec des savepoints)...
pducroquet changed title from WIP: sql: switch to autocommit (#81133) to sql: switch to autocommit (#81133) 2023-10-05 20:14:24 +02:00
pducroquet force-pushed wip/81133-psycopg-autocommit from 84a01f4ad4 to 1e934b355d 2023-10-05 20:15:28 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 1e934b355d to 707611b6f0 2023-10-05 20:52:21 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 707611b6f0 to 92a8c72917 2023-10-05 21:26:38 +02:00 Compare
bdauvergne commented 2023-10-05 21:41:45 +02:00
Owner
Copy Link

J'aime toujours bien l'idée du atomic. Néanmoins tu as gardé guard_postgres or le fait de passer en autocommit casse complètement sa sémantique, le conn.rollback() en cas d'exception n'aura aucun effet sans conn.autocommit = False en entrée du décorateur. L'idéal ce serait de pouvoir faire guard_postgres = atomic et que tout marche toujours.

Il y a aussi la séquence conn.autocommit = False \ .. \ conn.commit() \ conn.autocommit = True dans rebuild_security qui je pense pourrait être remplacé par un décorateur @atomic en gardant le conn.commit() explicite au milieu (ça casse un peu la pureté de atomic() mais l'idée est la même que le code actuel).

J'aime toujours bien l'idée du atomic. Néanmoins tu as gardé guard_postgres or le fait de passer en autocommit casse complètement sa sémantique, le conn.rollback() en cas d'exception n'aura aucun effet sans `conn.autocommit = False` en entrée du décorateur. L'idéal ce serait de pouvoir faire `guard_postgres = atomic` et que tout marche toujours. Il y a aussi la séquence `conn.autocommit = False \ .. \ conn.commit() \ conn.autocommit = True` dans rebuild_security qui je pense pourrait être remplacé par un décorateur `@atomic` en gardant le `conn.commit()` explicite au milieu (ça casse un peu la pureté de atomic() mais l'idée est la même que le code actuel).
pducroquet force-pushed wip/81133-psycopg-autocommit from 92a8c72917 to 4f6fc63fb0 2023-10-05 21:42:12 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 4f6fc63fb0 to 8c9918af13 2023-10-05 21:54:42 +02:00 Compare
pducroquet commented 2023-10-05 22:50:31 +02:00
Author
Owner
Copy Link

J'aime toujours bien l'idée du atomic. Néanmoins tu as gardé guard_postgres or le fait de passer en autocommit casse complètement sa sémantique, le conn.rollback() en cas d'exception n'aura aucun effet sans conn.autocommit = False en entrée du décorateur. L'idéal ce serait de pouvoir faire guard_postgres = atomic et que tout marche toujours.

Pas super fan puisqu'on se retrouverait dans ce cas à balancer plein de transactions et de savepoints sans vraies raisons. Le seul usage qu'il restait à guard_postgres était un appel à capture_exception. Je l'ai retiré du coup en supposant que c'était pris par ailleurs, si ça ne convient pas on peut revoir différemment (intercepter dans WcsPgConnection par exemple)

Il y a aussi la séquence conn.autocommit = False \ .. \ conn.commit() \ conn.autocommit = True dans rebuild_security qui je pense pourrait être remplacé par un décorateur @atomic en gardant le conn.commit() explicite au milieu (ça casse un peu la pureté de atomic() mais l'idée est la même que le code actuel).

Je suis d'accord, l'actuel est malheureux, je compte sur le week-end pour trouver une solution sans le .commit()

> J'aime toujours bien l'idée du atomic. Néanmoins tu as gardé guard_postgres or le fait de passer en autocommit casse complètement sa sémantique, le conn.rollback() en cas d'exception n'aura aucun effet sans `conn.autocommit = False` en entrée du décorateur. L'idéal ce serait de pouvoir faire `guard_postgres = atomic` et que tout marche toujours. Pas super fan puisqu'on se retrouverait dans ce cas à balancer plein de transactions et de savepoints sans vraies raisons. Le seul usage qu'il restait à guard_postgres était un appel à capture_exception. Je l'ai retiré du coup en supposant que c'était pris par ailleurs, si ça ne convient pas on peut revoir différemment (intercepter dans WcsPgConnection par exemple) > Il y a aussi la séquence `conn.autocommit = False \ .. \ conn.commit() \ conn.autocommit = True` dans rebuild_security qui je pense pourrait être remplacé par un décorateur `@atomic` en gardant le `conn.commit()` explicite au milieu (ça casse un peu la pureté de atomic() mais l'idée est la même que le code actuel). Je suis d'accord, l'actuel est malheureux, je compte sur le week-end pour trouver une solution sans le .commit()
pducroquet force-pushed wip/81133-psycopg-autocommit from a0e22e1ff1 to 3eddf5f4e4 2023-10-05 22:59:01 +02:00 Compare
bdauvergne commented 2023-10-06 07:11:49 +02:00
Owner
Copy Link

J'aime toujours bien l'idée du atomic. Néanmoins tu as gardé guard_postgres or le fait de passer en autocommit casse complètement sa sémantique, le conn.rollback() en cas d'exception n'aura aucun effet sans conn.autocommit = False en entrée du décorateur. L'idéal ce serait de pouvoir faire guard_postgres = atomic et que tout marche toujours.

Pas super fan puisqu'on se retrouverait dans ce cas à balancer plein de transactions et de savepoints sans vraies raisons. Le seul usage qu'il restait à guard_postgres était un appel à capture_exception. Je l'ai retiré du coup en supposant que c'était pris par ailleurs, si ça ne convient pas on peut revoir différemment (intercepter dans WcsPgConnection par exemple)

J'ai toujours pensé que le guard_postgres était aussi là pour donner un comportement transactionnel à certaines de ces fonctions si ce n'est pas le cas, soit.

> > J'aime toujours bien l'idée du atomic. Néanmoins tu as gardé guard_postgres or le fait de passer en autocommit casse complètement sa sémantique, le conn.rollback() en cas d'exception n'aura aucun effet sans `conn.autocommit = False` en entrée du décorateur. L'idéal ce serait de pouvoir faire `guard_postgres = atomic` et que tout marche toujours. > > Pas super fan puisqu'on se retrouverait dans ce cas à balancer plein de transactions et de savepoints sans vraies raisons. Le seul usage qu'il restait à guard_postgres était un appel à capture_exception. Je l'ai retiré du coup en supposant que c'était pris par ailleurs, si ça ne convient pas on peut revoir différemment (intercepter dans WcsPgConnection par exemple) J'ai toujours pensé que le guard_postgres était aussi là pour donner un comportement transactionnel à certaines de ces fonctions si ce n'est pas le cas, soit.
fpeters requested changes 2023-10-06 09:41:43 +02:00
wcs/sql.py Outdated
@ -97,0 +140,4 @@
last_savepoint = conn._wcs_savepoints.pop()
cursor.execute("RELEASE SAVEPOINT \"%s\";" % last_savepoint)
else:
# rollback transaction, or rollback savepoint (and release the savepoint, it won't be used anymore)
fpeters commented 2023-10-06 09:41:41 +02:00
Owner
Copy Link

Cette partie n'est pas couverte par les tests, c'est quelque chose qui serait possible ?

Cette partie n'est pas couverte par les tests, c'est quelque chose qui serait possible ?
pducroquet commented 2023-10-06 12:50:20 +02:00
Author
Owner
Copy Link

Boarf, ça compile, c'est déjà ça non ? :)
Oui bien sûr, je pensais qu'on passait dans le rollback sur certains des tests, mais je pense que c'était une version intermédiaire où j'avais ajouté beaucoup trop d'appels à atomic dans le code.

Boarf, ça compile, c'est déjà ça non ? :) Oui bien sûr, je pensais qu'on passait dans le rollback sur certains des tests, mais je pense que c'était une version intermédiaire où j'avais ajouté beaucoup trop d'appels à atomic dans le code.
pducroquet marked this conversation as resolved
pducroquet force-pushed wip/81133-psycopg-autocommit from 6e82a22c77 to db0d7c58e2 2023-10-06 13:29:10 +02:00 Compare
pducroquet commented 2023-10-06 13:43:08 +02:00
Author
Owner
Copy Link

Voilà, tous les chemins du nouveau code Atomic sont testés, avec validation que ça fasse bien des rollback/commit dans le bon sens.

Voilà, tous les chemins du nouveau code Atomic sont testés, avec validation que ça fasse bien des rollback/commit dans le bon sens.
pducroquet requested review from fpeters 2023-10-06 13:43:36 +02:00
pducroquet force-pushed wip/81133-psycopg-autocommit from db0d7c58e2 to 05673c7c31 2023-10-08 07:54:52 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 05673c7c31 to 92b24a04ac 2023-10-13 10:03:09 +02:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 92b24a04ac to 2f4e86d679 2023-10-13 10:07:22 +02:00 Compare
fpeters requested changes 2023-10-26 09:36:41 +02:00
fpeters left a comment
Owner
Copy Link

(le build jenkins est actuellement en erreur)

(le build jenkins est actuellement en erreur)
pducroquet force-pushed wip/81133-psycopg-autocommit from 2f4e86d679 to 4fb129598f 2023-11-14 14:46:08 +01:00 Compare
pducroquet force-pushed wip/81133-psycopg-autocommit from 9c2245ea66 to 3129abd862 2023-11-14 16:43:09 +01:00 Compare
pducroquet commented 2023-11-14 16:43:45 +01:00
Author
Owner
Copy Link

(le build jenkins est actuellement en erreur)

C'est réparé, et je pense que le code est intégrable dans le projet en l'état.

> (le build jenkins est actuellement en erreur) C'est réparé, et je pense que le code est intégrable dans le projet en l'état.
pducroquet requested review from fpeters 2023-11-14 16:43:54 +01:00
fpeters force-pushed wip/81133-psycopg-autocommit from 3129abd862 to cabc133a77 2023-11-24 14:26:53 +01:00 Compare
fpeters approved these changes 2023-11-24 15:04:46 +01:00
fpeters merged commit cabc133a77 into main 2023-11-24 15:05:03 +01:00
fpeters deleted branch wip/81133-psycopg-autocommit 2023-11-24 15:05:03 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
fpeters
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/wcs#667
No description provided.
Delete Branch "wip/81133-psycopg-autocommit"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?