[elephant-devel] Re: Updated version of last Postmodern patch bundle

[Robert L. Read]

Dear Alex,

	Thank you very much for taking the time to deal with this.  I
personally don't have much of an opinion about how it should be
done---except that it sure seems like a good idea to make it
configurable, which ought to keep everybody happy, and prevents me
having to make a decision I am poorly-qualified to make.

	Could you make this configurable?  I would be happy to document it and
commit it if it were configurable (and if we can get to a proven green
state, which might be a little tricky right at the moment---but we can
save the patch for when we get it ironed out.)

On Sun, 2008-03-16 at 23:26 +0200, Alex Mizrahi wrote:
>  LPP> It's a performance decision...
> 
> as far as i know with MVCC higher isolation level does not cause performance 
> penalties unless you really have conflicts.
> at least some postgresql experts said me this.
> 
>  LPP> go, but the rest of the code should try hard not to expose any
>  LPP> avoidable errors if the user decides to switch to another isolation
>  LPP> level...
> 
> switching to another isolation level user gets concurrent conflicts. why 
> should we try to detect them in our code when DB can handle them in 
> centralized fashion?
> this sounds like an "ugly workaround" for me.
> 
>  LPP> In any case, can you send a patch for the change of the default
>  LPP> isolation level, and probably a public convenience function/sc arg to
>  LPP> change it?
> 
> changing isolation level is dead simple, but it requires dealing with 
> consequences. this was pretty hard for application i'm working with, so i 
> had to make extensions like retry-cleanup-fn.
> and stuff is not configurable because i thought serializable would be good 
> for everybody, but if you insist we should have "read committed" too, i'll 
> make it for you.
> below is work-in-progress patch:
> 
> --- old-elephant/src/db-postmodern/pm-transaction.lisp Wed Jan 16 23:07:43 
> 2008
> +++ new-elephant/src/db-postmodern/pm-transaction.lisp Fri Mar 14 15:00:02 
> 2008
> @@ -26,30 +26,70 @@
>  (defun txn-cache-clear-value (bt key)
>    (cache-clear-value *txn-value-cache* bt key))
> 
> +(defun execute-transaction-one-try (sc txn-fn always-rollback)
> +  (let (tran commited
> + (*txn-value-cache* (make-value-cache sc)))
> +    (incf (tran-count-of sc))
> +    (setf tran (controller-start-transaction sc))
> +    (unwind-protect
> +  (multiple-value-prog1
> +      (funcall txn-fn)
> +    (unless always-rollback ; automatically commit unless always-rollback 
> is on
> +      (controller-commit-transaction sc tran)
> +      (setf commited t)))
> +      (unless commited (controller-abort-transaction sc tran))
> +      (decf (tran-count-of sc)))))
> +
> +(defmacro with-concurrency-errors-handler (&body body)
> +  "execute body with a handler catching postgres concurrency errors
> +   and invoking restart-transaction restart automatically"
> +  `(handler-bind
> +    ((cl-postgres:database-error
> +      (lambda (c)
> + (let ((err-code (cl-postgres:database-error-code c)))
> +   (when (or (string= err-code "40001") ; SERIALIZATION FAILURE
> +      (string= err-code "40P01")); DEADLOCK DETECTED
> +     (invoke-restart 'retry-transaction c))))))
> +    , at body))
> +
>  (defmethod execute-transaction ((sc postmodern-store-controller) txn-fn
> -    &key (always-rollback nil) &allow-other-keys)
> -  ;; SQL doesn't support nested transaction
> +    &key (always-rollback nil)
> +    (retry-cleanup-fn nil)
> +    (retries 10) &allow-other-keys)
>    (with-postmodern-conn ((controller-connection-for-thread sc))
>      (if (> (tran-count-of sc) 0)
> -        (funcall txn-fn)
> -        (let (tran
> -       commited
> -       (*txn-value-cache* (make-value-cache sc)))
> -          (incf (tran-count-of sc))
> -          (unwind-protect
> -        (prog2
> -     (setf tran (controller-start-transaction sc))
> -     (funcall txn-fn) ;;this gets returned
> -   (unless always-rollback ;;automatically commit unless always rollback
> -     (controller-commit-transaction sc tran)
> -     (setf commited t)))
> -     (unless commited (controller-abort-transaction sc tran))
> -     (decf (tran-count-of sc)))))))
> +
> + ;; SQL doesn't support nested transaction
> + ;; TODO: perhaps it's worth detecting abnormal exit here
> + ;; and abort parent transaction too.
> + (with-concurrency-errors-handler (funcall txn-fn))
> +
> + (loop named txn-retry-loop
> +   ;; NB: it does (1+ retries) attempts, 1 try + retries.
> +   for try from retries downto 0
> +   do (block txn-block
> +        (restart-bind ((retry-transaction
> +          (lambda (&optional condition)
> +     (when (and retry-cleanup-fn
> +         (not (= try 0))) ; cleanup is skipped when we are exiting
> +       (funcall retry-cleanup-fn condition sc))
> +     (return-from txn-block))
> +          :report-function (lambda (s) (princ "retry db-postmodern 
> transaction" s)))
> +         (abort-transaction
> +          (lambda () (return-from txn-retry-loop))))
> +        (with-concurrency-errors-handler
> +          (return-from txn-retry-loop
> +     (execute-transaction-one-try sc txn-fn always-rollback)))))
> +   finally (error 'transaction-retry-count-exceeded
> +    :format-control "Transaction exceeded the ~A retries limit"
> +    :format-arguments (list retries)
> +    :count retries)))))
> +
> 
>  (defmethod controller-start-transaction ((sc postmodern-store-controller) 
> &key &allow-other-keys)
>    (with-postmodern-conn ((controller-connection-for-thread sc))
>      (let ((transaction (make-instance 'postmodern::transaction-handle)))
> -      (postmodern:execute "BEGIN")
> +      (postmodern:execute "BEGIN ISOLATION LEVEL SERIALIZABLE")
>        transaction)))
> 
>  (defmethod controller-commit-transaction ((sc postmodern-store-controller) 
> transaction &key &allow-other-keys)
> 
> 
> 
> 
> 
> 
> _______________________________________________
> elephant-devel site list
> elephant-devel at common-lisp.net
> http://common-lisp.net/mailman/listinfo/elephant-devel