On Thu, Mar 04, 2021 at 08:46:44PM +0000, John Rouillard wrote:
> 
> In the initial post, you make a case for the connection name as a
> static value.  Does that info have to be included as a comment in the
> code somewhere so others won't be confused by a static name?

No the static value is hard coded (for filter, filter_iter and
_materialize_multilink I'm using different hard-coded values)

> In back_postgresql.py the sql_new_cursor method has an argument
> name='default'. But there is no code to make sure that it was called
> with a real cursor name. Should there be a check and exception thrown
> if name == 'default'?

The cursor is dynamically created by sql_new_cursor.
So 'default' would be a valid name.

> Do we need code in configuration.py that enforces serverside_cursor =
> no if you are not using postgres and if so, is 'yes' the right
> default?

The code is written in a way that doesn't use that config variable for
other sql backends, see the implementation of sql_new_cursor in
backends/rdbms_common.py vs. backends/back_postgresql.py, the latter
ignores the cursor name in the default implementation.

> I think the call to self.db.sql_new_cursor(name='filter') in
> rdbms_common.py is called for all backends: sqlite mysql and postgres
> right? Why is there no need for:
> 
>    sql_new_cursor(self, name, conn, *args, *(kw))
> 
> for other rdbms backends besides postgres?

It is called with the name for all backends but the default implementation
ignores the parameter in the sql_new_cursor method in
backends/rdbms_common.py.

I've looked into mysql, there server-side cursors without "buffering"
seem to be the default. There are named cursors in mysql but they do not
seem to be related with client-side vs. server-side cursors as in
psycopg. Note that I think making client-side cursors the default in
psycopg is a bad design decision. It certainly surprised me that the
roundup-server process grew by 1GB just after issuing the SQL statement
:-) I haven't looked into what sqlite does in this regard. Since sqlite
cannot do parallel queries (!) I would think that the first performance
optimization would be to replace sqlite with another backend anyway.

> I see you are testing and comparing both server side and client side
> cursors. Is it possible we could get different answers based on the
> amount of data returned? That is the server says "hey I only have 4
> rows returned, I'll push the 4 rows and kill the serve side
> cursor?". So that server side cursor on a small data set is the same
> as client side cursor?

The results should never be different. By default a server-side cursor
in Postgres fetches 2000 results in one go, the parameter of the cursor
is called "itersize" and is configurable. So for small query results
server-side vs. client-side cursors are identical in behaviour. Only if
the result-set is larger than the itersize will the client repeatedly
fetch from the server-side cursor.

> I am pushing to CI now. Pleakse keep an eye on
> https://travis-ci.org/github/roundup-tracker/roundup in case there are
> issues.

Thanks!
Looks like everything passes. (I *did* run the tests on python2.7 and
3.7 before pushing).

> Thanks for identify fixing this. Sadly bpo could probably use this but
> since they are moving away from roundup ...
Yes, I still think it's an error to move to a commercially hosted
tracker where it is questionable if it won't become a vendor lockin data
silo when the company behind it changes policies.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com