pyodbc and tables with triggers

[polaar]

Hi,

I recently tried out sqlalchemy with mssql via pyodbc (after being
bitten by the adodbapi bug with the truncated parameters), and noticed
the following problem:

On inserting records into tables with triggers, pyodbc fails on the
'select @@identity as lastrowid' statement with an 'invalid cursor
state' error.

I've managed to work around this by doing "set nocount on" when
creating the connection, via a creator like this:
def create_connection():
conn = pyodbc.connect(connectionstring)
cur = conn.cursor()
cur.execute('set nocount on')
cur.close()
return conn

I don't know if there's a better way, but it seems to work for me. I
just mention it because it might be a useful tip, and also because it
seems like something that should be handled by the sqlalchemy mssql
driver?

greetings,

Steven

[Tim Golden]

polaar wrote:
> I recently tried out sqlalchemy with mssql via pyodbc (after being
> bitten by the adodbapi bug with the truncated parameters), and noticed
> the following problem:
>
> On inserting records into tables with triggers, pyodbc fails on the
> 'select @@identity as lastrowid' statement with an 'invalid cursor
> state' error.

OK, I can't reproduce this (and there's a follow-on issue which
I'll pick up later). Just to clarify, I have this structure
compiled on the database:

<db>
IF OBJECT_ID ('test_audit') IS NOT NULL
DROP TABLE test_audit
GO
IF OBJECT_ID ('test') IS NOT NULL
DROP TABLE test
GO

CREATE TABLE
test
(
id INT NOT NULL IDENTITY PRIMARY KEY,
code VARCHAR (10) NOT NULL UNIQUE
)
GO

CREATE TABLE
test_audit
(
test_id INT NOT NULL FOREIGN KEY REFERENCES test (id),
inserted_on DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
inserted_by VARCHAR (60) NOT NULL DEFAULT SYSTEM_USER
)
GO

CREATE TRIGGER tr_test_i ON test FOR INSERT AS
INSERT INTO test_audit (test_id) SELECT id FROM inserted
GO

</db>

That's a main table (test) an audit table (test_audit)
into which test-INSERTs are triggered. Now, in sqlalchemy:

<code>
from sqlalchemy import *
metadata = BoundMetaData ("mssql://VODEV1/TimHolding")
test = Table ("test", metadata, autoload=True)
result = test.insert ().execute (code = "ABC")
print result.last_inserted_ids ()
# => [1]
</code>

which is what I expected. If I explicitly set NOCOUNT OFF
for my session (in case it's on by default) using:

metadata.engine.raw_connection ().execute ("SET NOCOUNT OFF")

then it still works.

Is my case the situation you're describing? Or have I
misunderstood somthing?

TJG

[Tim Golden]

I've looked through the mailing list and can't see
this issue raised there so...

There is a known issue with retrieving the id of the
last inserted row under MSSQL where IDENTITY cols are
used and there are triggers involved. It's pretty easy
to demonstrate. If I have this construction:

<db>

CREATE TABLE
test
(
id INT NOT NULL IDENTITY PRIMARY KEY,
code VARCHAR (10) NOT NULL UNIQUE
)
GO

CREATE TABLE
test_audit
(
id INT NOT NULL IDENTITY (100, 1) PRIMARY KEY,

test_id INT NOT NULL FOREIGN KEY REFERENCES test (id),
inserted_on DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
inserted_by VARCHAR (60) NOT NULL DEFAULT SYSTEM_USER
)
GO

CREATE TRIGGER tr_test_i ON test FOR INSERT AS
INSERT INTO test_audit (test_id) SELECT id FROM inserted
GO

</db>

and insert something into test:

<db>
INSERT INTO test (code) VALUES ('ABC')
SELECT @@IDENTITY
SELECT * FROM test
</db>

The last id is 100 (the IDENTITY col from [test_audit])
while, as far as the user's concerned, [test] was the
only table affected. In sqlalchemy terms, this means
that the last_inserted_ids () could return misleading
values:

<python>
from sqlalchemy import *
db = BoundMetaData ("mssql://VODEV1/TimHolding")
test = Table ("test", db, autoload=True)
r = test.insert ().execute (code="DEF")
print r.last_inserted_ids ()
# => [101]
list (test.select ().execute (id=101))
# => []
</python>

What are the alternatives? Well, there are two:

IDENT_CURRENT ('tablename')

gives the last identity value assigned to this table
*in any session* (<alert>race condition</alert>)

or

SCOPE_IDENTITY ()

which seems to be what we're after here; it's like @@IDENTITY
but for the same "scope" (not a widely-used term in SQL
Server circles, as far as I know). The documentation
specifically gives this case as an example.

Looks to me like this would be the best bet for sqlalchemy's
purposes, but I'm sure there's a downside somewhere ;)

Comments?
TJG

[Rick Morrison]

If the trigger insets other rows on the same connection, that can foul things up for @@identity, as the value is connection-global.

There is an alternative (can't remember the name now, something like @@scope_identity?) that can be used in its stead that MIGHT fix this, but using it will break backward compatibility to the point where MS added the new variable (I think in MSSQL-2000).

Is anybody still using MSSQL-V7 or earlier with SA?

Rick

[polaar]

On Mar 7, 3:29 pm, Tim Golden <tjgol...@gmail.com> wrote:
> <code>
> from sqlalchemy import *
> metadata = BoundMetaData ("mssql://VODEV1/TimHolding")
> test = Table ("test", metadata, autoload=True)
> result = test.insert ().execute (code = "ABC")
> print result.last_inserted_ids ()
> # => [1]
> </code>
>
> which is what I expected. If I explicitly set NOCOUNT OFF
> for my session (in case it's on by default) using:
>
> metadata.engine.raw_connection ().execute ("SET NOCOUNT OFF")
>
> then it still works.
>
> Is my case the situation you're describing? Or have I
> misunderstood somthing?

My fault: I forgot to tell you that I was using a mapped class, and
it's the sqlalchemy-generated 'select @@identity' that causes the
problem. (you can see that it does that in the log output)

[Rick Morrison]

OK, I replied to the other thread already, which is really the same issue. See my response there about backward-compatibility.

At any rate, we could make it a connection variable like auto_identity_insert. Patches welcome.

Rick

On 3/7/07, Tim Golden <tjgo...@gmail.com> wrote:

[polaar]

Oops, guess I was lucky that in my case, the triggers only did updates
and no inserts. (I wasn't even aware of them before I ran into the
problem, as I did not set up the database)

> What are the alternatives? Well, there are two:
>
> IDENT_CURRENT ('tablename')
>
> gives the last identity value assigned to this table
> *in any session* (<alert>race condition</alert>)
>
> or
>
> SCOPE_IDENTITY ()
>
> which seems to be what we're after here; it's like @@IDENTITY
> but for the same "scope" (not a widely-used term in SQL
> Server circles, as far as I know). The documentation
> specifically gives this case as an example.
>
> Looks to me like this would be the best bet for sqlalchemy's
> purposes, but I'm sure there's a downside somewhere ;)
>
> Comments?
> TJG

SCOPE_IDENTITY() seems the right choice. I don't know if there are
hidden problems, but if there are, it looks like @@IDENTITY will have
them too. So it seems safer than @@IDENTITY in any case.

[Tim Golden]

Rick Morrison wrote:
> OK, I replied to the other thread already, which is really the same issue.
> See my response there about backward-compatibility.
>
> At any rate, we could make it a connection variable like
> auto_identity_insert. Patches welcome.

I'm happy to provide a patch. Not sure about the connection
variable. Ah, I see, you mean because of backwards compat.
But isn't the problem that if we just leave the @@IDENTITY
as now, it's a danger waiting to happen, especially if the
returned id happens to be a valid id for the table you
*think* it's for?

Not really sure what to offer here:

1) I can provide a patch, replacing @@IDENTITY by SCOPE_IDENTITY
throughout.

2) I can provide a patch allowing connection-level determination
of whether @@IDENTITY or SCOPE_IDENTITY is to be used. (Which
assumes the client module knows what that's about).

3) I can provide a patch which attempts to work out which
one would be allowed from some DB context (or just trying
it to see!)

The problem with (1) is that, if Rick's right, it won't
work with MSSQL <= 7. The problem with any of (2) or (3)
where @@IDENTITY ends up being used is that we might be
silently and dangerously returning wrong data.

TJG

[Rick Morrison]

I think option #2 with a default is the way to go (see the implementation of auto_identity_insert for how default behavior is done).

I would suggest to default to scope_identity(), which is as you say, safer and is available for most installations, but allow a connection level variable to allow MSSQL v7 users and earlier to still play.

Rick

On 3/7/07, Tim Golden <tjgo...@gmail.com> wrote:

[polaar]

Hmmm, seems the "set nocount on" trick now causes problems on deletes:
ConcurrentModificationError is thrown because "Updated rowcount -1
does not match number of objects updated 1".
Which seems strange because I thought rowcount -1 simply meant that
the count cannot be determined, not that there is something wrong.
There seems to be a supports_sane_rowcount check (for MySQL according
to the docs), wouldn't it make sense to treat a rowcount of -1 the
same?
Or should one just never use "set nocount on" when using the orm
(which would mean back to the original problem)?

Steven

[Rick Morrison]

This is still with pyodbc?  The MSSQL module should already set sane_rowcount to False for that dialect, as per the pyodbc site, they don't implement rowcount.

Rick

[polaar]

Yes, but I'm starting to think I'm doing something wrong ;-) I suppose
I should call create_engine with the module=pyodbc?
I was just using the creator argument (as I was doing already because
I needed to change the connectionstring to use integrated security
anyway), and just switched that from adodbapi to pyodbc. So maybe it's
still using the default adodbapi settngs...
Hmm, seems to make sense... oops... (well, it's not really clear from
the docs that this is used for anything else than determining which
module to use to create the connection, which seems unnecessary if you
create it yourself)

I'll try it on monday...

[Tim Golden]

> I needed to change the connectionstring to use integrated security
> anyway),

FWIW if someone were to be able to review / commit my patch
on ticket 488 (http://www.sqlalchemy.org/trac/ticket/488)
the integrated security would be there anyway. Haven't
got round to patching the SCOPE_IDENTITY stuff yet.

TJG

[Rick Morrison]

Module selection in MSSQL is a bit ugly right now. Mike has proposed a clean-up of the way that DB-API modules are loaded and used, so this will get better soon, I hope.

I'll have a look at the patch.

Rick 

On 3/9/07, polaar <steven.v...@gmail.com > wrote:

[Paul Johnston]

Hi,

I've had a go at implementing scope_identity, with mixed results.

It works fine with pymssql
With adodbapi, scope_identity() always returns None
Same problem with pyodbc, and I uncovered a worse problem lurking (it
exists with @@identity)
If you do an insert inside a transaction (i.e. autocommit doesn't
happen), when it tries to extract the ID, you get the dreaded "Invalid
cursor state" error.

So, for the time being I think we should hold fire on scope_identity. I
will see if I can figure out what's up with adodbapi/pyodbc.

Paul

[Rick Morrison]

Hi Paul,

Could this be one of those situations where MSSQL returns multiple result sets? If the select for @@identity / @scope_identity() generates another result set, MSSQL want a full fetch on the previous fetch before issuing a new one. Fix would be client-side cursors for adodbapi, pyodbc would need to implement nextset().

But it's hard for me to believe the problem is as simple as a select @@identity inside a transaction - surely some one (or one of the tests) would have hit that one before.

Rick

[polaar]

FYI, specifying module=pyodbc didn't seem to help wrt the
ConcurrentModificationError. Didn't have very much time, had a (very)
quick look at the code in mssql.py, and at first sight, it would seem
that sane_rowcount is a global variable that is only set in the
use_pyodbc() (resp. adodbapy/pymssql) function, which in turn is only
called from use-default(), this would seem to mean only when you don't
specify a module... Either I'm completely wrong (which is very well
possible ;-), as I said, I only took a quick look, and I'm not
familiar with the code), or this means that you may not have adodbapi
(or pymssql) installed in order to use pyodbc correctly???

[polaar]

On 12 mrt, 21:47, "polaar" <steven.vereec...@gmail.com> wrote:
> FYI, specifying module=pyodbc didn't seem to help wrt the
> ConcurrentModificationError. Didn't have very much time, had a (very)
> quick look at the code in mssql.py, and at first sight, it would seem
> that sane_rowcount is a global variable that is only set in the
> use_pyodbc() (resp. adodbapy/pymssql) function, which in turn is only
> called from use-default(), this would seem to mean only when you don't
> specify a module... Either I'm completely wrong (which is very well
> possible ;-), as I said, I only took a quick look, and I'm not
> familiar with the code), or this means that you may not have adodbapi
> (or pymssql) installed in order to use pyodbc correctly???
>

Update: it indeed seems to work like that. I tried changing the order
of preference in mssql.py so that it first tries pydobc, and that
seems to work: the ConcurrentModificationError no longer occurs. I now
also get a warning about using pyodbc that I didn't get before. (by
the way: I did have to keep the 'set nocount on' in order to prevent
the invalid cursor state problem)

I guess something could be done with changing the following line from
the __init__ method of class MSSQLDialect:
self.module = module or dbmodule or use_default()
to something that calls use_pyodbc/use_pymssql/use_adodbapi based on
module.__name__? (I'm not sure though: use_default seems to be called
already when the mssql is imported and it sets the global dbmodule, so
I'm not confident that this is where it should be done*)
Something like this?

{'pyodbc': use_pyodbc, 'adodbapi': use_adodbapi, 'pyodbc':
use_pyodbc}.get(module.__name__, use_default)()

Steven

* can't test it at home (using linux), and as using python at work is
mostly 'under the radar', I can't spend a lot of time on it there, so
sorry if I can't provide you with a well-tested patch ;-)

[polaar]

> {'pyodbc': use_pyodbc, 'adodbapi': use_adodbapi, 'pyodbc':
> use_pyodbc}.get(module.__name__, use_default)()

Sorry, should be pymssql instead of pyodbc twice, but I guess you got
that...

[Rick Morrison]

It's the second case, that is, it "sniffs" out what modules are installed. As I said before, this (along with other modules that effectively do the same thing), is up for a clean-up soon, see ticket #480.

Rick

[Rick Morrison]

Sorry, Stephen, I replied too early; your second email arrived before the first. A whole day before the first.

So until we get a real cleanup, you're looking to try modules in this order:

  ['pyodbc', 'adodbapi', 'pymssql']

Sounds OK to me -- any objections out there?

Rick

[Tim Golden]

Rick Morrison wrote:
> Sorry, Stephen, I replied too early; your second email arrived before the
> first. A whole day before the first.
>
> So until we get a real cleanup, you're looking to try modules in this order:
>
> ['pyodbc', 'adodbapi', 'pymssql']
>
> Sounds OK to me -- any objections out there?

Looks good to me.

I got slightly confused somewhere through this thread.
When I was putting a test together for the passthrough
patch, I ended up using an Import hook to force a
particular dbapi module to be used programatically
(given that I have all three installed).

Obviously there are variations on that (manually renaming
one etc) but have I missed anything more sophisticated
using SA itself? Didn't look like it to me from the code.

TJG

[Rick Morrison]

I don't think you missed anything; that "more sophisticated" approach is exactly what we're discussing. Seeing as how you have all three modules installed and switch between them, you probably have your own ideas about how it should work. Please pick up the discussion on ticket #480 and give us your input.

Rick

[Rick Morrison]

Tim, I committed a patch from ticket #480 today from that adds some improved module-switching code to the MSSQL interface. See how that works for you.

[Tim Golden]

Rick Morrison wrote:
> Tim, I committed a patch from ticket #480 today from
> that adds some improved module-switching code to the MSSQL interface. See how that works for you.

I'll have a look when I get near an MSSQL-connected machine
(tomorrow earliest). Thanks v. much.

TJG

[polaar]

Sorry for the delay ;-) No objections. I was just trying to figure out
what was happening. (didn't get at first that "module selection is a
bit ugly" also meant that it didn't work if you specified the module)
But you already have a patch it seems. I'll try and test it out.

Steven

On 15 mrt, 15:43, "Rick Morrison" <rickmorri...@gmail.com> wrote:
> Sorry, Stephen, I replied too early; your second email arrived before the
> first. A whole day before the first.
>
> So until we get a real cleanup, you're looking to try modules in this order:
>
> ['pyodbc', 'adodbapi', 'pymssql']
>
> Sounds OK to me -- any objections out there?
>
> Rick
>

> On 3/14/07, Rick Morrison <rickmorri...@gmail.com> wrote:
>
>
>
> > It's the second case, that is, it "sniffs" out what modules are installed.
> > As I said before, this
> > (along with other modules that effectively do the same thing), is up for a clean-up soon, see ticket #480.
>
> > Rick
>