pydal

[Massimo DiPierro]

Should we expect more changes to DAL in the near future? we would like to tag a new version.

Massimo

[Paolo Valleri]

I've done with https://github.com/web2py/pydal/pull/105

 Paolo

2015-03-23 17:15 GMT+01:00 Massimo DiPierro <massimo....@gmail.com>:

Should we expect more changes to DAL in the near future? we would like to tag a new version.

Massimo

--
-- mail from:GoogleGroups "web2py-developers" mailing list
make speech: web2py-d...@googlegroups.com
unsubscribe: web2py-develop...@googlegroups.com
details    : http://groups.google.com/group/web2py-developers
the project: http://code.google.com/p/web2py/
official    : http://www.web2py.com/
---
You received this message because you are subscribed to the Google Groups "web2py-developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to web2py-develop...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Niphlod]

I still need to figure out a way to run unit tests on mssql.
It's not a matter of "it's impossible to do it" rather an "I need to figure out how"

[Paolo Valleri]

@niphold, if needed tomorrow I can run tests using windows/pypyodbc/python2.7.9/mssql(2008)

 Paolo

[Niphlod]

doing right now. running tests became a real PITa and I spotted yet a few bugs.

[Niphlod]

ahem... can somebody explain

https://github.com/web2py/pydal/blob/master/tests/sql.py#L469

?

in T-SQL, a condition should/can be resolved only if there are two parts compared with an operator. I don't think it's standard to evaluate (1=1) to a simple "AND 1", and in fact mssql complains about needing a condition............

[Massimo DiPierro]

I cannot explain. I think the intention was that

(True && query) should be replaced with (query)

(False || query) should also be replaced with (query)

The fact that DAL may be able to do this, does not mean it should be able to do it.

[Niphlod]

BTW: reviewing tests I noticed a few test cases with a return to the bottom (I guess they could be easily eliminated) but another thing too:

def testcase1():
    db = DAL('blablabla')

def testcase2():
    db = DAL('blablabla2')

the connections are kept and never dropped.... should we end every testcase with a db.close() to ensure that connections are dropped ?

[Giovanni Barillari]

I'll wait niphlod to build the new release.

/Giovanni

[Giovanni Barillari]

* I'll wait niphlod changes and consideration before I build the new release.

/Giovanni

[Niphlod]

uhm. reiterating on the "funny" thing of opened connections.

uri = 'mssql4://blablabla'
for a in range(1000):
    db = DAL(uri)

opens (and keeps open) 1000 connections

uri = 'mssql4://blablabla'
for a in range(1000):
    db = DAL(uri)
    db.close()

opens (and keeps open) 1000 connections

uri = 'mssql4://blablabla'
for a in range(1000):
    db = DAL(uri, pool_size=5)
    db.close()

only one connection results alive (could be fine with that, but feels strange anyway)

does anyone see in other adapters ?

[Massimo DiPierro]

can you try revert this patch?

https://github.com/web2py/pydal/commit/f08df5e2bc317c096874d0c97c2743d8044d82b3

[Giovanni Barillari]

Seems we really still have connection issues. I think we should slow down a bit and try to analyze the flow and see what's going on.

I will try on mongo.

[Niphlod]

confirmed, it's that patch.

+1 for reviewing the flow.
Comparing one to the other ....

def close(self,action='commit',really=True):
        if action:
            if callable(action):
                action(self)
            else:
                getattr(self, action)()
        # ## if you want pools, recycle this connection
        if self.pool_size:
            GLOBAL_LOCKER.acquire()
            pool = ConnectionPool.POOLS[self.uri]
            if len(pool) < self.pool_size:
                pool.append(self.connection)
                really = False
            GLOBAL_LOCKER.release()
        if really:
            self.close_connection()
        self.connection = None

vs

def close(self, action='commit', really=False):
        if action:
            try:
                if callable(action):
                    action(self)
                else:
                    getattr(self, action)()
            except:
                really = True

        # If you want pools, recycle this connection
        if self.pool_size and really == False:
            GLOBAL_LOCKER.acquire()
            pool = ConnectionPool.POOLS[self.uri]
            if len(pool) < self.pool_size:
                pool.append(self.connection)
            else:
                really = True
            GLOBAL_LOCKER.release()
        if really:
            try:
                self.close_connection()
            except:
                pass
        self.connection = None

"really" changed from true to false, with obvious issues...
What was the rationale behind it ?

[Massimo DiPierro]

The first version you show is not the one before the patch. These lines had three revision. You show version 0 (original) and version 3 (broken). Version 1 was also broken. Version 2 was the one before the patch was applied and the one I tested. It introduced the “success” variable. It prevents recycling of a connection of the an action (commit, rollback) fails. I am not sure about the rationale for revision 3, perhaps @Paolo can explain.

[Niphlod]

groan, sorry, late night.

keeps open if no pool_size              
               https://raw.githubusercontent.com/web2py/pydal/master/pydal/connection.py
closes as advertised, no matter of pool_size
               https://raw.githubusercontent.com/web2py/pydal/7db1d1a638802a62389cf79a771728dc2e1cffdb/pydal/connection.py

"version 0" still closed connections as advertised.

[Massimo DiPierro]

I suggest we revert to version 0 then. It always worked for me. You in favor?

[Giovanni Barillari]

Please, use commits hashes when you refers to some "version". I didn't understand which is the "version 0" we're talking about.

/Giovanni

To unsubscribe from this group and stop receiving emails from it, send an email to web2py-developers+unsub...@googlegroups.com.

[Niphlod]

Sorry, maybe I was misleading with that "no matter".....if there was a rationale behind 1, I'd promote it over 0.
 
As far as db.close() actually terminating the connection, it behaves exactly the same as version 0, i.e. db.close() actually terminates the connection independently of the pool_size, and that's both fine for me and straight from a "logic" perspective.

it's the current trunk that has issues (more obvious without the pool_size argument), but maybe there's a rationale there too and
 
db = DAL('...')
db.close()
it's not supposed to terminate the connection anymore, but only committing/rollbacking and put back the connection in the pool.
That being said, current trunk leaves a few open connections when there is a pool_size (even with repeated calls to close()), while without a pool_size it never closes the connection. That's what makes me thing there has been some idea behind it that may be worth reassessing.


BTW: in all of this, I still feel that every test case should be terminated with a db.close() (if in the test case there's a db = DAL('...'))

[Niphlod]

sorry @giovanni, we're going through pydal/connections.py history

0 : 19d1022
1: 7db1d1a
2: 30566f8

Basically, here https://github.com/web2py/pydal/commits/master/pydal/connection.py, from 22 Feb to 11 March

[Giovanni Barillari]

Anyway, inspecting 'master' code, the deal is that the "really" parameter is default to False, and when we don't have a connection pool this lead to not closing the current connection. I can work on a patch.

[Niphlod]

BTW, once sorted out

https://github.com/web2py/pydal/blob/da21aafba19b81ccacf631a785fe5e437e516192/pydal/connection.py#L115

holds tab values instead of spaces.

BTW3: most of the code here is not covered by tests.

[Niphlod]

On Tuesday, March 24, 2015 at 12:11:40 AM UTC+1, Giovanni Barillari wrote:

Anyway, inspecting 'master' code, the deal is that the "really" parameter is default to False, and when we don't have a connection pool this lead to not closing the current connection. I can work on a patch.

This was what I was pointing out before: the change on the default value of really is the bit causing issues when no pool_size is around. But I really don't see why that had to be changed.

[Massimo DiPierro]

Will you be able to add these tests?

On Mar 23, 2015, at 4:31 PM, Niphlod <nip...@gmail.com> wrote:

--
-- mail from:GoogleGroups "web2py-developers" mailing list
make speech: web2py-d...@googlegroups.com
unsubscribe: web2py-develop...@googlegroups.com
details : http://groups.google.com/group/web2py-developers
the project: http://code.google.com/p/web2py/
official : http://www.web2py.com/
---
You received this message because you are subscribed to the Google Groups "web2py-developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to web2py-develop...@googlegroups.com.

[Giovanni Barillari]

I think we have 2 choices:

1) connection.close has really default to True to close connections without pools

2) db.close has really set to True

I think is a "political" choice.

[Massimo DiPierro]

I think both should default really=True

--
-- mail from:GoogleGroups "web2py-developers" mailing list
make speech: web2py-d...@googlegroups.com
unsubscribe: web2py-develop...@googlegroups.com
details : http://groups.google.com/group/web2py-developers
the project: http://code.google.com/p/web2py/
official : http://www.web2py.com/
---
You received this message because you are subscribed to the Google Groups "web2py-developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to web2py-develop...@googlegroups.com.

[Giovanni Barillari]

https://github.com/web2py/pydal/pull/107

I think this is the behavior we want..

/Giovanni 

[Massimo DiPierro]

Looks good to me. If it passes Simone’s test, I will approve it.

[Niphlod]

it's ok : connections are correctly terminated when you explicitely invoke db.close()

[Paolo Valleri]

Sorry for the troubles of the mentioned commit :(

That part is not covered by tests, I'll write a few somehow

 Paolo

[Niphlod]

don't mind, the important is not have shipped it as stable :-P

[Massimo DiPierro]

true. thank you both for the hard work!