Обсуждение: 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

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

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

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

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