[storm] Oracle support review

[Gustavo Noronha Silva]

On Fri, 2008-08-15 at 19:02 -0300, Gustavo Niemeyer wrote:
> Gustavo,

Gustavo, =)

> I've just had a look at your branch implementing Oracle support for
> Storm, and it actually surprised me, since it feels like it's pretty close
> to being mergeable.

Not anymore, given the time I took to respond? =)

> It'd be awesome if more people who have hands on access to
> an Oracle database could give it a try too.

I agree!

> [1]

You suggestion seems to work, I'll commit the change, thanks! =)

> [2]
> Don't worry too much about this though.  We can even fix it up for you
> before integration if you want.

Sorry I was not very careful about it, but I'll take your offer of
(auto?)fixing it before integrating, if it's not a problem!

> [3]
> 
> +    # this is a simple hack to make sure all auto-generated aliases
> +    # get escaped; I added this if because oracle was complaining of
> +    # stuff such as money."value", that was being generated;
> 
> Do you know what exactly is the syntax that Oracle complains about?
> We can probably try to fix it without any hacks.

This is the error message I was trying to get fixed:

[15:43:35.047627] EXECUTE: 'SELECT money.id, money."value" FROM money WHERE money.id = :param_1 AND ROWNUM <= :param_2', [{':param_1': <storm.variables.IntVariable object at 0xa38704c>}, {':param_2': <storm.variables.IntVariable object at 0xa38728c>}]
[15:43:35.051330] ERROR: ORA-00904: "MONEY"."value": invalid identifier

Now, I'm a bit confused myself because what seems to be generated for
that specific case is this:

[16:06:54.650243] EXECUTE: 'SELECT money.id, money.value FROM money WHERE money.id = :1 AND ROWNUM <= :2', [10, 1]

> [4]
> 
> +        super(OracleResult, self).__init__(connection, raw_cursor)
> +        self.lastrowid = rowid
> 
> This should be private, since it's not part of the API offered by Storm itself.
> At least not yet.  We've been pondering about adding a standard way to
> access such entries.

You mean it should have a _ before its name? The main problem here is
that, differently from some other db2api modules, cx_Oracle doesn't
allow adding new attributes to the cursor object. 

> It might also be good to look at the mysql-insert-id branch from James
> which is up for review.  It has some optimizations to avoid one extra
> query on inserts which perhaps could be used here too.

Interesting. I'll take a look at that, but as far as I know, by
integrating Willi's work (even though it has been done in this fairly
hackish way) actually made getting the last row id not need the extra
query; the insert query itself gives us the id.

> [5]
> +    def execute(self, statement, params=None, noresult=False):
> +        """NOTE: this is being overriden completely because the
> +        original from the base class expects to receive only a
> +        raw_cursor from raw_execute, and we need to receive also the
> +        rowid, as we cannot set it in the cursor object
> 
> I don't understand all the details here, but this should be avoided if
> possible, since it's duplicating a lot of logic, which means that any
> changes in the top class will break it.
> 
> Would you mind to bring the topic up online so that we can figure it
> out together?

The problem is basically that I need to pass the rowid in, and I cannot
use the raw_cursor as a bearer, as is done by other backends.

> 
> [6]
> 
> +    def raw_execute(self, statement, params):
> +        """NOTE: this is being overriden completely because the original
> 
> Same case.

Same as above, and overriding inserts to get the row_id; this part is
basically the same as postgres' backend does.

> [7]
> 
> +        # FIXME (even more!): this except/sleep is here because the
> +        # storm test suite would have lots of failing tests because
> +        # cx_Oracle apparently is not always able to connect several
> +        # times in a row, for some reason
> 
> Argh!  We have to figure a fix for this somehow.  2 seconds per connection is
> a pretty bad limitation. :-(

It doesn't happen everytime, but yeah, it does happen; this is something
that we must take to cx_Oracle, or maybe it is a problem with oracle-xe.
That's why it would be a good thing to bring some more people who use
oracle in.

> [8]
> 
> +from storm.tracer import debug
> +
> +DEBUG = True
> +
> +debug(DEBUG)
> 
> All the DEBUG items should be removed from tests before integration
> to avoid polluting the natural test output.

OK, this was just too helpful for me while debugging. I will remove it
as soon as you are OK to integrate.

> Thanks for pushing this forward.

Thanks for the review, and sorry for the delay in answering!

See you,

-- 
Gustavo Noronha Silva <kov at alfaiati.net>
AlfaiaTI