[Cubicweb] NativeSQLSource.doexec auto-rollback

Christophe de Vienne christophe at unlish.com
Sat Dec 27 18:36:37 CET 2014


Hi everyone,

We are running into a behavior that feels wrong to me, while using the
worker cube.

The worker cube uses the task workflow to make sure only one worker gets
to execute it. Hence, to acquire a task before executing it, it simply
attempt to change its state.

Doing so, it actually write a new entry in the database: a relation
between the task and the 'executing' state. If the task is already being
executed, ie if its state it already 'executing', the update fails with
a duplicate key violation, and the worker simply pass to the next task.

So far so good.

The problem is that when the duplicate key violation occur, which is
expected to happen sometimes with several workers, here is what we get
in the logs:

cubicweb.sources.system 2014-12-27 16:57:07,780 CRITICAL sql many:
'INSERT INTO in_state_relation ( eid_from, eid_to ) VALUES (
%(eid_from)s, %(eid_to)s )'
 args: [{'eid_to': 2240, 'eid_from': 1250592}]
dbms message: 'duplicate key value violates unique constraint
"in_state_relation_p_key"\nDETAIL:  Key (eid_from, eid_to)=(1250592,
2240) already exists.\n'
cubicweb.sources.system 2014-12-27 16:57:10,072 CRITICAL transaction has
been rolled back


I consider this a problem because it is in no way a critical error: it
is expected to happen sometimes, and if it does it is perfectly fine. It
should not even be a warning.


Digging a little in the source and looking for the 'transaction has been
rolled back', I found the NativeSQLSource.doexec function, which is
basically:

    def doexec(self, cnx, query, args=None, rollback=True):
        cursor = cnx.blablabla
        try:
            cursor.execute()
        except Exception as ex:
            self.critical('sql: xxx' % ex.xxx)
            if rollback:
                cnx.rollback()
                self.critical('transaction has been rolled back')
            # some clever code to translate the exception in a CW one
            # if possible

Several things are bothering me:

- The 'rollback' feature, if anyhow useful (I do not think it is),
  should not have such a default: doexec is, in my understanding, a low
  level function. And rolling back a transaction is a high-level
  decision. But since the doexec function is called by many intermediate
  functions without passing any 'rollback' parameter, we end up with a
  high-level decision made in a low-level function.

- If an error occur at the sql level, it will be transmitted or
  translated to a CW error, which is fine. But if the exception is
  raised and handled in a higher level function, why do we make it a
  critical error?

- Admitting that rolling-back automatically on errors is wanted in some
  cases, how can it be considered 'critical'? The caller will receive
  the original exception anyway, it is its job to decide if it is
  critical or not.


So basically, I would like to propose 2 things:

- Remove the 'critical' logs, and change them into into 'debug', or
  'info'.

- Remove the 'rollback' option, or at least change its default value.
  I really thing this feature has nothing to do in this place.


I am willing to provide patches, but since I expect some disagreements I
prefer we talk about it before writing code.


Thanks for making it so far, and happy end of the year festivities.

Christophe

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.cubicweb.org/pipermail/cubicweb/attachments/20141227/52fedd72/attachment-0213.sig>


More information about the Cubicweb mailing list