Thread: Result Set Cursor Patch
I submitted an updated patch to this list on yesterday, but I never saw my post come through. Did I just miss it or was it lost in transit somewhere? -Andy
Attachment
On Tue, 4 May 2004, Andy Zeneski wrote: > I submitted an updated patch to this list on yesterday, but I never saw > my post come through. Did I just miss it or was it lost in transit > somewhere? > I haven't seen it and it's not in the archives. If it was large it could have gotten killed. The list has a size limit somewhere in the 30-50k range. Kris Jurka
Kris Jurka <books@ejurka.com> writes:
> I haven't seen it and it's not in the archives. If it was large it could
> have gotten killed. The list has a size limit somewhere in the 30-50k
> range.
I believe pgsql-patches has a larger size limit. Also, consider
compressing the attachment if that will get you under 30K ...
BTW, you can set majordomo to notify you if it delays or drops a
message. I think this is not on by default for some reason.
regards, tom lane
Kris, Here it is again, I think my IDE reformatted a few classes which caused this patch to be bigger then it needed to be. Here it is in its current state. I took the suggestions from the list and implemented the movement of position using the MOVE command. This made finding the result set size MUCH easier. In addition, I moved all the code related to chunks into a new class called ResultChunk which lives in core. Attached is a tarball which contains the new file as well as the patch. This patch uses diff -c rather then diff -u as it was brought to my attention as being the preferred format for this project. Let me know if this is not correct. If the formatting is major problem, I can attempt to reformat it to desired settings. I will need to know what the "preferred" way is. I've noticed there are several different formats used within these classes, I could easily run the tree though Jacobe and clean it up. However, that may be too big for the list. :) There is partial support for reverse fetching in this patch, but it does not work yet. In this version it is completely disabled so even if reverse is requested it will ignore the request and use forward fetching instead. I will fix this when I have some extra time. I don't think the current code supports reverse fetching either, so I don't think any current functionality is lost. The code attached does pass the entire test suite without problem. Please let me know if there are any problems. -Andy On May 4, 2004, at 1:35 PM, Kris Jurka wrote: > > > On Tue, 4 May 2004, Andy Zeneski wrote: > >> I submitted an updated patch to this list on yesterday, but I never >> saw >> my post come through. Did I just miss it or was it lost in transit >> somewhere? >> > > I haven't seen it and it's not in the archives. If it was large it > could > have gotten killed. The list has a size limit somewhere in the 30-50k > range. > > Kris Jurka > > ---------------------------(end of > broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if > your > joining column's datatypes do not match
Attachment
On Wed, 5 May 2004, Andy Zeneski wrote: > Kris, > > Here it is again, I think my IDE reformatted a few classes which caused > this patch to be bigger then it needed to be. Here it is in its current > state. > > If the formatting is major problem, I can attempt to reformat it to > desired settings. I will need to know what the "preferred" way is. I've > noticed there are several different formats used within these classes, > I could easily run the tree though Jacobe and clean it up. However, > that may be too big for the list. :) The formatting is an issue. Yes, the code is formatted differently all over the driver, but now is not the time to fix that. There are too many outstanding patches that will conflict with these whitespace changes to try and do something about that now. I aim to reindent/reformat at the start of 7.5 beta when there shouldn't be many outstanding patches or active development. Also just trying to read your patch is difficult because it is impossible to tell if a real change is mixed in with a whitespace change. Could you resubmit this patch as just your changes. Kris Jurka
Or add a -w to your diff to ignore whitespace differences. --Barry Kris Jurka wrote: > > On Wed, 5 May 2004, Andy Zeneski wrote: > > >>Kris, >> >>Here it is again, I think my IDE reformatted a few classes which caused >>this patch to be bigger then it needed to be. Here it is in its current >>state. >> >>If the formatting is major problem, I can attempt to reformat it to >>desired settings. I will need to know what the "preferred" way is. I've >>noticed there are several different formats used within these classes, >>I could easily run the tree though Jacobe and clean it up. However, >>that may be too big for the list. :) > > > The formatting is an issue. Yes, the code is formatted differently all > over the driver, but now is not the time to fix that. There are too many > outstanding patches that will conflict with these whitespace changes to > try and do something about that now. I aim to reindent/reformat at the > start of 7.5 beta when there shouldn't be many outstanding patches or > active development. > > Also just trying to read your patch is difficult because it is impossible > to tell if a real change is mixed in with a whitespace change. Could you > resubmit this patch as just your changes. > > Kris Jurka > > > ---------------------------(end of broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if your > joining column's datatypes do not match
No when the the formatting got real messy, I just told IDEA to reformat the entire class. Ended up doing that for a few of them. That changed a lot of lines. I'll need to figure out what is different. This will probably take a while to do. -Andy On May 5, 2004, at 4:15 PM, Barry Lind wrote: > Or add a -w to your diff to ignore whitespace differences. > > --Barry > > Kris Jurka wrote: >> On Wed, 5 May 2004, Andy Zeneski wrote: >>> Kris, >>> >>> Here it is again, I think my IDE reformatted a few classes which >>> caused this patch to be bigger then it needed to be. Here it is in >>> its current state. >>> >>> If the formatting is major problem, I can attempt to reformat it to >>> desired settings. I will need to know what the "preferred" way is. >>> I've noticed there are several different formats used within these >>> classes, I could easily run the tree though Jacobe and clean it up. >>> However, that may be too big for the list. :) >> The formatting is an issue. Yes, the code is formatted differently >> all over the driver, but now is not the time to fix that. There are >> too many outstanding patches that will conflict with these whitespace >> changes to try and do something about that now. I aim to >> reindent/reformat at the start of 7.5 beta when there shouldn't be >> many outstanding patches or active development. >> Also just trying to read your patch is difficult because it is >> impossible to tell if a real change is mixed in with a whitespace >> change. Could you resubmit this patch as just your changes. >> Kris Jurka >> ---------------------------(end of >> broadcast)--------------------------- >> TIP 9: the planner will ignore your desire to choose an index scan if >> your >> joining column's datatypes do not match
Attachment
Kris Jurka wrote: > The formatting is an issue. Yes, the code is formatted differently all > over the driver, but now is not the time to fix that. There are too many > outstanding patches that will conflict with these whitespace changes to > try and do something about that now. I aim to reindent/reformat at the > start of 7.5 beta when there shouldn't be many outstanding patches or > active development. Are we still tracking the main tree's release cycle then? I thought part of the reason for moving to gborg was because the JDBC driver's release cycle didn't match the main tree's release cycle well? I have a large stack of not-yet-ready-for-prime-time v3 protocol changes that I'd like to get in at some point -- but I don't want to get bitten by "we're in beta, this is too big to apply". Are we planning on a code/feature freeze or similar, tracking whatever 7.5 is doing? Also, now that the driver is separate from the main code tree, maybe it'd be a good idea when reindenting to use more "normal" (for java anyway) indentation rules -- e.g. use either all spaces or spaces and 8-character tabs when indenting. The current 4-character tabs rule is fairly unusual. That'd make driver editing easier across a range of IDEs; sure, I have my xemacs settings that play nicely with the current scheme, but it's a barrier to entry for anyone who hasn't worked on the driver before. -O
Oliver Jowett wrote: > Are we still tracking the main tree's release cycle then? I thought part > of the reason for moving to gborg was because the JDBC driver's release > cycle didn't match the main tree's release cycle well? > > I have a large stack of not-yet-ready-for-prime-time v3 protocol changes > that I'd like to get in at some point -- but I don't want to get bitten > by "we're in beta, this is too big to apply". Are we planning on a > code/feature freeze or similar, tracking whatever 7.5 is doing? > I suspect we are trying to track the server release schedule. 7.5 looks like it might be the first server release in a while that didn't require changes to the jdbc driver to remain compatible with it. So I don't know of a reason right now that would force a new jdbc version at the same time as 7.5 of the server, however I think it is a good idea. As for the move to gborg, the rational of different release cycles was one argument made by those forcing the jdbc driver out of the base source tree. Jdbc was moved to gborg not by choice but by mandate. --Barry
This outsider/new-guy-on-the-list thinks "4 space" tabs are great. I think it's very portable because if a file is indented with all tabs then someone can set their editor to render that however they like: 2, 4, 8, e, Pi, etc. "spaces". 4 space + 8-space-tabs is the least portable. Everyone has to configure their editor exactly alike. all spaces is portable, everyone gets the same thing, but on the down side, everyone gets the same thing. Of course, that's just my opinion, I could be wrong. Brian Olson http://bolson.org/ On Thu, 6 May 2004, Oliver Jowett wrote: > Also, now that the driver is separate from the main code tree, maybe > it'd be a good idea when reindenting to use more "normal" (for java > anyway) indentation rules -- e.g. use either all spaces or spaces and > 8-character tabs when indenting. The current 4-character tabs rule is > fairly unusual. That'd make driver editing easier across a range of > IDEs; sure, I have my xemacs settings that play nicely with the current > scheme, but it's a barrier to entry for anyone who hasn't worked on the > driver before.
Brian Olson wrote:
> This outsider/new-guy-on-the-list thinks "4 space" tabs are great. I think
> it's very portable because if a file is indented with all tabs then
> someone can set their editor to render that however they like: 2, 4, 8, e,
> Pi, etc. "spaces".
This only works if you only ever indent in strict multiples of a
tabwidth and are consistent about only ever using tabs. In practice,
this doesn't happen -- you'll get spaces appearing pretty fast. Witness
the current driver, where in theory only tabs should be being used, but
in practice there are lots of spaces used to indent. Also consider cases
like this, assuming 2-space tabs on the lefthand side:
if (condition) { // Comment 1
code; // Comment 2
} // Comment 3
With 4 space tabs we get:
if (condition) { // Comment 1
code; // Comment 2
} // Comment 3
Oops, those comments don't nicely line up any more. You can't fix it
using tabs to align the comments either, since the indenting you need on
a particular line depends on the line length and isn't necessarily a
multiple of the tab width.
> 4 space + 8-space-tabs is the least portable. Everyone has to configure
> their editor exactly alike.
Yes, mixing spaces and tabs makes life difficult (especially if you're
editing python code!). This is essentially the same case as the current
code (both spaces and 4-space-tabs are used, although tabs-only is
mandated).
> all spaces is portable, everyone gets the same thing, but on the down
> side, everyone gets the same thing.
This is what I prefer because it's hard to get wrong. And personally I
think that the *goal* is that everyone sees the same thing for a
particular bit of code.
And it makes 'diff' and 'less' produce more readable output, too :)
There's a pretty good summary of the issues at
http://www.jwz.org/doc/tabs-vs-spaces.html. Some conclusions from that doc:
> My opinion is that the best way to solve the technical issues is to
> mandate that the ASCII #9 TAB character never appear in disk files:
> program your editor to expand TABs to an appropriate number of spaces
> before writing the lines to disk. That simplifies matters greatly, by
> separating the technical issues of #2 and #3 from the religious issue of
> #1.
> I just care that two people editing the same file use the same
> interpretations, and that it's possible to look at a file and know what
> interpretation of the TAB character was used, because otherwise it's
> just impossible to read.
-O
On Wed, 5 May 2004, Barry Lind wrote: > > > Oliver Jowett wrote: > > Are we still tracking the main tree's release cycle then? I thought part > > of the reason for moving to gborg was because the JDBC driver's release > > cycle didn't match the main tree's release cycle well? > > > > I suspect we are trying to track the server release schedule. 7.5 looks > like it might be the first server release in a while that didn't require > changes to the jdbc driver to remain compatible with it. So I don't > know of a reason right now that would force a new jdbc version at the > same time as 7.5 of the server, however I think it is a good idea. > Yes, continuing to track the server releases is the plan as I understand it. I would venture that we don't need to stick to their feature freeze date which is June 1st as best as I can tell from a long and complicated hackers discussion. Server betas tend to take forever and I don't think we need that kind of time to smooth out any JDBC issues. Assuming a minimum two month server freeze->release time frame, I would be comfortable with a July 1st freeze for the JDBC driver. Thoughts? Kris Jurka
Kris, Here is that patch again, correct without any formatting or whitespace changes (white space was ignored in the diff). Let me know if this is better then the last one. -Andy On May 5, 2004, at 3:46 PM, Kris Jurka wrote: > > > On Wed, 5 May 2004, Andy Zeneski wrote: > >> Kris, >> >> Here it is again, I think my IDE reformatted a few classes which >> caused >> this patch to be bigger then it needed to be. Here it is in its >> current >> state. >> >> If the formatting is major problem, I can attempt to reformat it to >> desired settings. I will need to know what the "preferred" way is. >> I've >> noticed there are several different formats used within these classes, >> I could easily run the tree though Jacobe and clean it up. However, >> that may be too big for the list. :) > > The formatting is an issue. Yes, the code is formatted differently all > over the driver, but now is not the time to fix that. There are too > many > outstanding patches that will conflict with these whitespace changes to > try and do something about that now. I aim to reindent/reformat at the > start of 7.5 beta when there shouldn't be many outstanding patches or > active development. > > Also just trying to read your patch is difficult because it is > impossible > to tell if a real change is mixed in with a whitespace change. Could > you > resubmit this patch as just your changes. > > Kris Jurka > > > ---------------------------(end of > broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if > your > joining column's datatypes do not match
Attachment
On Tue, 11 May 2004, Andy Zeneski wrote: > Here is that patch again, correct without any formatting or whitespace > changes (white space was ignored in the diff). Let me know if this is > better then the last one. > This is much better, but there are still a fair number of inconsequential changes like renaming this_row and other variables. If we had some naming standards that you were matching to, or you were making everything in one localized area match I wouldn't object, but I can't say I understand the name changes. Anyway that's a minor issue at this point, so lets get to the heart of the matter. The JDBC driver's goal is compatibility for the current version and the previous two releases. Two key features this depends on were introduced in 7.4: ABSOLUTE cursor positioning and the SCROLL keyword actually guaranteeing a scrollable cursor in all situations. Ideally we could retain the current FORWARD_ONLY requirement for 7.3 servers. I notice ResultSetChunk has get/setFetchSize, but they are unused and lack the checking provided in the AbstractJdbc2ResultSet versions. I tried creating three 100 row tables which in an unconstrained join produce a million rows and testing some things. rs.absolute(44000000); // past the end of the result LOG: statement: DECLARE JDBC_CURS_1 SCROLL CURSOR FOR SELECT a,b,c FROM t1,t2,t3 ; FETCH FORWARD 3 FROM JDBC_CURS_1 LOG: duration: 1.425 ms LOG: statement: MOVE ABSOLUTE 43999999 IN JDBC_CURS_1 LOG: duration: 3565.519 ms LOG: statement: FETCH FORWARD 3 FROM JDBC_CURS_1 LOG: duration: 2.236 ms LOG: statement: MOVE FORWARD ALL IN JDBC_CURS_1 LOG: duration: 2.134 ms LOG: statement: MOVE ABSOLUTE 0 IN JDBC_CURS_1 LOG: duration: 2.139 ms LOG: statement: MOVE FORWARD ALL IN JDBC_CURS_1 LOG: duration: 1312.637 ms LOG: statement: MOVE ABSOLUTE 43999999 IN JDBC_CURS_1 LOG: duration: 2.135 ms This jumps back to zero and then to the end again for some reason costing us 1.3 seconds in this test. In general something seems wrong. Testing using previous() with a forward fetch direction I see: rs.absolute(); while (rs.previous()) ; LOG: statement: DECLARE JDBC_CURS_1 SCROLL CURSOR FOR SELECT a,b,c FROM t1,t2,t3 ; FETCH FORWARD 3 FROM JDBC_CURS_1 LOG: duration: 1.441 ms LOG: statement: MOVE ABSOLUTE 19 IN JDBC_CURS_1 LOG: duration: 0.394 ms LOG: statement: FETCH FORWARD 3 FROM JDBC_CURS_1 LOG: duration: 0.349 ms LOG: statement: MOVE ABSOLUTE 0 IN JDBC_CURS_1 LOG: duration: 0.225 ms LOG: statement: MOVE FORWARD ALL IN JDBC_CURS_1 LOG: duration: 3550.779 ms LOG: statement: MOVE ABSOLUTE 22 IN JDBC_CURS_1 LOG: duration: 2.287 ms LOG: statement: MOVE ABSOLUTE 18 IN JDBC_CURS_1 LOG: duration: 2.164 ms LOG: statement: FETCH FORWARD 3 FROM JDBC_CURS_1 LOG: duration: 2.231 ms LOG: statement: MOVE ABSOLUTE 17 IN JDBC_CURS_1 LOG: duration: 2.156 ms LOG: statement: FETCH FORWARD 3 FROM JDBC_CURS_1 LOG: duration: 2.268 ms This strangely jumps back and forward after the first fetch and kills performance for this query. The way that previous() currently works will be a real downer for many people, especially with a reasonably large fetch size. While FETCH BACKWARD is ideal, a stopgap solution would be to MOVE back and fetch forward which looks like it might be easier with your current code setup. For this particular case (especially because people probably aren't setting fetch direction) I would disagree with your defaulting of fetchSize to 1000 instead of it's previous 0 meaning no cursor fetching. Finally I notice that the cursor is not closed when either the ResultSet or Statement are closed. This wasn't really a problem with non-scrolling cursors, but now with the possibility of consuming significant server side resources this is a necessity. Kris Jurka