Re: [Patch] Update view-database only if the transaction succeeded

Hi Saurabh,

Given that you have expressed interest in submitting future
improvements to CLSQL, I have some general and specific suggestions so
that you can best contribute to the project with this bug fix and
future improvements.

Your identification of the bug is an excellent find. I'd like to
commit your final patch using git-am which automatically notes
authorship and commits the changes into the repository. Please take a
look at the git-am manual page for more info about on formatting of
your submission, especially about how the body of the message becomes
the commit message (see below). 

Add a ChangeLog entry so that can be in the same commit as your
patch. As you may have seen looking at the repository, I typically use
the ChangeLog addition as the actual commit message. See [1] below --
disregard the subversion revision "r11761: " prefix which was added by
git-svnimport would not be part of the ChangeLog or future commit
messages.

To preserve backward compatibility with applications that expect the
database slot to be immediately updated after a store, as well as be
more consistent with the semantics of a SQL transaction, I think the
right thing to do is to fill the database slot of objects immediately
after a store. However, if a transaction ends before it is committed,
then at that time the database slot of the modified objects should be
returned to their initial states.

> --- a/sql/database.lisp
> +++ b/sql/database.lisp
> @@ -332,3 +332,5 @@ from the body. MAKE-DEFAULT has a default value of NIL."
> [...]
> +;;;
> +(defvar *transaction-insert-list*)

This variable declaration should be at the top of the
transactions.lisp file as it has nothing to do with what's in the
database.lisp file. Note in clsql.asd that the functional module is
loaded before the object module. Remove the empty comment line ";;;"
as it provides no value. However, please add a documentation string to
the defvar form. It would be better to change the variale name to
something more descriptive and to leave the type out of the name. I
think *view-objects-transaction-rollbacks* is a better name than
*transaction-insert-list*. This variable should hold a stack of lists
to properly handle nested transactions.

The commit should come with an addition to the test suite in
tests/test-oodml.lisp that demonstrates the correctness of your
patch. Single and nested transactions with commits and rollbacks
should cover theq possibilities.

The "Side Effects" section of the ROLLBACK documentation should note
what happens to view objects that have had their database slot
modified during a transaction. If you need help generating the html
and pdf documentation after modifying the DocBook XML, please let me
know.

Thanks again for spotting the problem and coming up with an initial
idea on a fix. If the above seems like more work that you can do, let
me know and I'll make an addition to the BUGS file until someone has
the time to extend your idea. If you do have time to fix the bug
optimally, that'd be great!

-- 
Kevin Rosenberg
[email protected]

[1]
commit c41d81c0d1233372012a1de93fcdfd6b2a6e5618
Author: Kevin M. Rosenberg <kevin-HJRc7zDS/[email protected]>
Date:   Fri Jul 20 23:19:02 2007 +0000

    r11761: 20 Jul 2007  Kevin Rosenberg <kevin-HJRc7zDS/[email protected]>
        * Version 3.8.6
        * db-oracle/oracle-loader.lisp: Rework use of ORACLE_HOME
        directory
        (problem noted on clsql-devel by icardo Boccato Alves)
        * sql/pool.lisp: Remove incorrect keyword
        * sql/database.lisp: Rework WITH-DATABASE to not make the
        database the
        default database (reported by Saurabh Nanda and Chaitanya
        Gupta)
        * doc/ref-connect.lisp: Update the documentation to
        WITH-DATABASE to emphasis
        that make-default has a default value of nil.
        * sql/transaction.lisp: Adjust commit/rollback messages for
        Microsoft SQL Server.
        (patch from Nathan Bird)
        * sql/metaclasses.lisp: Use finalize-inheritance hack on SBCL
        because of trouble with
        def-view-class compilations (patch from Nathan Bird)