Thread: Fix for fetchone() and fetchmany() in Python interface
This patch fixes the well-known but unfixed bug that fetchone() always returns the first result in the DB-API compliant wrapper. It turned out that the bug was way down in the C code. Gerhard -- mail: gerhard <at> bigfoot <dot> de registered Linux user #64239 web: http://www.cs.fhm.edu/~ifw00065/ public key at homepage public key fingerprint: DEC1 1D02 5743 1159 CD20 A4B6 7B22 6575 86AB 43C0 reduce(lambda x,y:x+y,map(lambda x:chr(ord(x)^42),tuple('zS^BED\nX_FOY\x0b')))
Attachment
Gerhard Häring wrote: > > This patch fixes the well-known but unfixed bug that fetchone() always returns > the first result in the DB-API compliant wrapper. It turned out that the bug > was way down in the C code. > > Gerhard This is great. It solves a problem we were having. Bruce: this should go into 7.1.3 (if possible). It applies cleanly and, without it, we basically can't use Python (cursors don't work, basically). -- Fernando Nasser Red Hat - Toronto E-Mail: fnasser@redhat.com 2323 Yonge Street, Suite #300 Toronto, Ontario M4P 2C9
> Gerhard H?ring wrote: > > > > This patch fixes the well-known but unfixed bug that fetchone() always returns > > the first result in the DB-API compliant wrapper. It turned out that the bug > > was way down in the C code. > > > > Gerhard > > This is great. It solves a problem we were having. > > Bruce: this should go into 7.1.3 (if possible). It applies cleanly and, > without it, we basically can't use Python (cursors don't work, basically). We only put major lowrisk fixes in 7.1.X releases, and this one is too minor for the risk. Sorry. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
> This patch fixes the well-known but unfixed bug that fetchone() always returns
> the first result in the DB-API compliant wrapper. It turned out that the bug
> was way down in the C code.
>
> Gerhard
> --
> mail: gerhard <at> bigfoot <dot> de registered Linux user #64239
> web: http://www.cs.fhm.edu/~ifw00065/ public key at homepage
> public key fingerprint: DEC1 1D02 5743 1159 CD20 A4B6 7B22 6575 86AB 43C0
> reduce(lambda x,y:x+y,map(lambda x:chr(ord(x)^42),tuple('zS^BED\nX_FOY\x0b')))
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> > We only put major lowrisk fixes in 7.1.X releases, and this one is too > > minor for the risk. Sorry. > > > > I think this patch is important enough -- it fixes an annoying broken > call, with a two line patch that is quite localized. So, the risk level > seems quite low. AFAICT, the worst thing this patch could do is keep > fetch as broken, though perhaps in a different way. But, even then, I'm > fairly confident that the patch is good, or at least better than what > we've got, since we've already tried it out here, and found it to > apparently fix the issue. I am not actually making the decision myself. Patching only major problems with low-risk patches has always been our policy. If you can get others to agree, I will gladly apply it. See the HISTORY file for a description of what is in 7.1.3. You can see they all follow that policy. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
On Wed, Aug 15, 2001 at 04:59:37PM -0400, Bruce Momjian wrote: > > This is great. It solves a problem we were having. > > > > Bruce: this should go into 7.1.3 (if possible). It applies cleanly and, > > without it, we basically can't use Python (cursors don't work, basically). > > We only put major lowrisk fixes in 7.1.X releases, and this one is too > minor for the risk. Sorry. First I'm glad that it's in the patch queue for a future version at all. But: I don't see any risk in this patch. A source code review of the function would show that this is an obvious fix for a stupid coding error. Gerhard -- mail: gerhard <at> bigfoot <dot> de registered Linux user #64239 web: http://www.cs.fhm.edu/~ifw00065/ public key at homepage public key fingerprint: DEC1 1D02 5743 1159 CD20 A4B6 7B22 6575 86AB 43C0 reduce(lambda x,y:x+y,map(lambda x:chr(ord(x)^42),tuple('zS^BED\nX_FOY\x0b')))
Bruce Momjian wrote: > > > Gerhard H?ring wrote: > > > > > > This patch fixes the well-known but unfixed bug that fetchone() always returns > > > the first result in the DB-API compliant wrapper. It turned out that the bug > > > was way down in the C code. > > > > > > Gerhard > > > > This is great. It solves a problem we were having. > > > > Bruce: this should go into 7.1.3 (if possible). It applies cleanly and, > > without it, we basically can't use Python (cursors don't work, basically). > > We only put major lowrisk fixes in 7.1.X releases, and this one is too > minor for the risk. Sorry. > I think this patch is important enough -- it fixes an annoying broken call, with a two line patch that is quite localized. So, the risk level seems quite low. AFAICT, the worst thing this patch could do is keep fetch as broken, though perhaps in a different way. But, even then, I'm fairly confident that the patch is good, or at least better than what we've got, since we've already tried it out here, and found it to apparently fix the issue. Neil -- Neil Padgett Red Hat Canada Ltd. E-Mail: npadgett@redhat.com 2323 Yonge Street, Suite #300, Toronto, ON M4P 2C9
Bruce Momjian wrote: > > > Gerhard H?ring wrote: > > > > > > This patch fixes the well-known but unfixed bug that fetchone() always returns > > > the first result in the DB-API compliant wrapper. It turned out that the bug > > > was way down in the C code. > > > > > > Gerhard > > > > This is great. It solves a problem we were having. > > > > Bruce: this should go into 7.1.3 (if possible). It applies cleanly and, > > without it, we basically can't use Python (cursors don't work, basically). > > We only put major lowrisk fixes in 7.1.X releases, and this one is too > minor for the risk. Sorry. > Bruce, It may be too late for including it, but it would not be for the reasons above. 1) low risk It is low risk. It is a one line patch that only affects the python interface code. 2) major It is major. Without it we can consider that 7.1 does not support Python (at least, not the 2.0 API). What use is an interface that keeps returning the same row (the first one) every time you call the cursor trying to get the next row? It happens with both functions fetchone() and fetchmany(), so there is no workaround besides reading the whole table (which sometimes is not feasible). So, it is both low risk and major. Cheers, Fernando -- Fernando Nasser Red Hat - Toronto E-Mail: fnasser@redhat.com 2323 Yonge Street, Suite #300 Toronto, Ontario M4P 2C9
> Bruce, > > It may be too late for including it, but it would not be for the reasons above. > > 1) low risk > > It is low risk. It is a one line patch that only affects the python interface code. > > 2) major > > It is major. Without it we can consider that 7.1 does not support Python > (at least, not the 2.0 API). > > What use is an interface that keeps returning the same row (the first one) > every time you call the cursor trying to get the next row? > > It happens with both functions fetchone() and fetchmany(), so there is no > workaround besides reading the whole table (which sometimes is not feasible). > > > So, it is both low risk and major. If it has taken months for someone report report it and fix it, it can't be that major. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian wrote:
>
> > Bruce,
> >
> > It may be too late for including it, but it would not be for the reasons above.
> >
> > 1) low risk
> >
> > It is low risk. It is a one line patch that only affects the python interface code.
> >
> > 2) major
> >
> > It is major. Without it we can consider that 7.1 does not support Python
> > (at least, not the 2.0 API).
> >
> > What use is an interface that keeps returning the same row (the first one)
> > every time you call the cursor trying to get the next row?
> >
> > It happens with both functions fetchone() and fetchmany(), so there is no
> > workaround besides reading the whole table (which sometimes is not feasible).
> >
> >
> > So, it is both low risk and major.
>
> If it has taken months for someone report report it and fix it, it can't
> be that major.
>
Maybe people are using the old interface pg.py and that one is not broken?
Or people tried to use the new interface (pgdb.py) and gave up because it
didn't work (and went on to use some other DBMS)?
It took us a couple weeks to get the first customer complaint, which is a
very short time. We sent them Gerhard's patch and they are quite happy now
(thanks Gerhard!).
Regardless of the conflicting bug report statistics, just think how you would
use a binding that fails on the following (hint: fetchmany() does not work either).
row = c.fetchone()
count = 0
while row != None:
print count, str(row)
count = count + 1
row = c.fetchone()
You can only do:
count = 0
for row in c.fetchall():
print count, str(row)
count = count + 1
which will not work if your table is big.
So, with 7.1.2 you can only use the old interface or forget about Python
(or use Gerhard's patch).
--
Fernando Nasser
Red Hat - Toronto E-Mail: fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario M4P 2C9
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> If it has taken months for someone report report it and fix it, it can't
> be that major.
Good point --- but the whole Python interface is not very heavily used
AFAICT.
My initial reaction was the same as yours: put it in current sources,
but don't backpatch. But with both Fernando and Neil vouching for the
correctness of the patch, I'm willing to assume it's okay.
regards, tom lane
On Wed, Aug 15, 2001 at 06:01:58PM -0400, Fernando Nasser wrote: > > If it has taken months for someone report report it and fix it, it can't > > be that major. Many PyGreSQL users aren't aware of the fact that it's part of PostgreSQL. I sent a message to the mailing list recommending that they report bugs w/ the PostgreSQL bug tracker in the future, not only on the mailing list. > Maybe people are using the old interface pg.py and that one is not broken? It's broken as well. > Or people tried to use the new interface (pgdb.py) and gave up because it > didn't work (and went on to use some other DBMS)? I personally (and many others) switched to using different PostgreSQL/Python interface. There are three alternatives to the one include in PG. I prefer http://sourceforge.net/projects/pypgsql which is decidedly less buggy. > It took us a couple weeks to get the first customer complaint, which is a > very short time. We sent them Gerhard's patch and they are quite happy now > (thanks Gerhard!). You're welcome :-) I also think that the bug is a major one. There were at least three complaints at the mailing list, but it seems the PyGreSQL maintainer doesn't have much time. Gerhard -- mail: gerhard <at> bigfoot <dot> de registered Linux user #64239 web: http://www.cs.fhm.edu/~ifw00065/ public key at homepage public key fingerprint: DEC1 1D02 5743 1159 CD20 A4B6 7B22 6575 86AB 43C0 reduce(lambda x,y:x+y,map(lambda x:chr(ord(x)^42),tuple('zS^BED\nX_FOY\x0b')))
> My initial reaction was the same as yours: put it in current sources,
> but don't backpatch. But with both Fernando and Neil vouching for the
> correctness of the patch, I'm willing to assume it's okay.
I'll second that. (For me) the criteria for backpatching is that more
than one person is willing to *really* verify that the patch is Correct.
- Thomas
> > My initial reaction was the same as yours: put it in current sources, > > but don't backpatch. But with both Fernando and Neil vouching for the > > correctness of the patch, I'm willing to assume it's okay. > > I'll second that. (For me) the criteria for backpatching is that more > than one person is willing to *really* verify that the patch is Correct. OK, this is what I needed to hear. Patch applied to both trees. Let me reiterate my hesitation to apply this. First, 7.1.3 was supposed to be packaged up Tuesday night (two days ago), meaning I thought it was closed already. Second, I don't trust a patch to be 100% safe no matter who submits it. We had patches created and applied to 7.1 by core members that we thought were safe but in fact they were not and caused problems in 7.1.1 that were only fixed in 7.1.2, so "I don't trust no one." :-) Third, the evaluation of whether a patch is even safe takes time, and the farther we get from a major release, 7.1 in this case, the less likely we are to even take the time to evaluate it. Having a bug fix is not enough justification to get it backpatched. Most people want their patch backpatched, but if you look at the CVS HISTORY for 7.1.3 you will see that very few items are backpatched. We have over a dozen jdbc bugs fixed in CVS, but we aren't backpatching those because we are focused on 7.2, which is pretty close now. People who report JDBC problems are encouraged to use the CVS version or the one on http://jdbc.fastcrypt.com. I know it is not fun to ship bugs in 7.1.3 that we know we could fix, but with limited resources and testing, we are doing the best we can. If someone would like to take the responsibility of backpatching more aggressively, we should discuss that. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
...
> If someone would like to take the responsibility of backpatching more
> aggressively, we should discuss that.
...
Actually, I think that the process worked very well in this case. The
authors of the patch had to sell pretty hard to get it included, and the
reasons (and code) were reviewed by several people in the process. An
initial "no" is a good start for something like this!
It's OK for it to be hard to get something backpatched at the last
minute, and it is good that it is not impossible to do so. And if there
is a problem with the patch, then we all can be even more conservative
next time (and no one can say that we didn't all have a chance to have
input this time).
As we get older and wiser, there is a tendency to accumulate enough
experience that we end up saying "no, because we had trouble doing this
before..." more often than might be desirable.
All imho of course.
- Thomas
Bruce Momjian wrote: > > > > My initial reaction was the same as yours: put it in current sources, > > > but don't backpatch. But with both Fernando and Neil vouching for the > > > correctness of the patch, I'm willing to assume it's okay. > > > > I'll second that. (For me) the criteria for backpatching is that more > > than one person is willing to *really* verify that the patch is Correct. > > OK, this is what I needed to hear. Patch applied to both trees. > Thank you! > Let me reiterate my hesitation to apply this. First, 7.1.3 was supposed > to be packaged up Tuesday night (two days ago), meaning I thought it was > closed already. Second, I don't trust a patch to be 100% safe no matter > who submits it. We had patches created and applied to 7.1 by core > members that we thought were safe but in fact they were not and caused > problems in 7.1.1 that were only fixed in 7.1.2, so "I don't trust no > one." :-) Third, the evaluation of whether a patch is even safe takes > time, and the farther we get from a major release, 7.1 in this case, the > less likely we are to even take the time to evaluate it. > I believe we all agree with that. It just happened that this one was a win-win situation. The options were: 1) totally broken (without the patch) 2) Probably fixed (with the patch). There was no chance of affecting anything else that was not broken before due to the scope of the patch. The customer who had the problem applied the patch and is now very happy with his Python interface. Add this to Gerhards experience and ours, to the limited scope of the change and the fact that was completely broken before, and you see that it is a special case. > Having a bug fix is not enough justification to get it backpatched. > Most people want their patch backpatched, but if you look at the CVS > HISTORY for 7.1.3 you will see that very few items are backpatched. We > have over a dozen jdbc bugs fixed in CVS, but we aren't backpatching > those because we are focused on 7.2, which is pretty close now. People > who report JDBC problems are encouraged to use the CVS version or the > one on http://jdbc.fastcrypt.com. > > I know it is not fun to ship bugs in 7.1.3 that we know we could fix, > but with limited resources and testing, we are doing the best we can. > But JDBC for 7.1 was a lost case, while the one line change saves us from having to adopt the same criteria for Python folks. I rather see they use the version that is shipped from the pgsql tree and help fix it than using the alternative ones that Gerhard mentioned. (For many, using the development version is not an option). > If someone would like to take the responsibility of backpatching more > aggressively, we should discuss that. > I personally agree with the current rules. This was an exceptional case where there was much to gain and little to lose. The only bad thing was to give you extra work to repack things, for which I apologize. But you can rest assured that it was worthy of your effort: Python seems to have a growing number of adepts. Best regards, Fernando -- Fernando Nasser Red Hat - Toronto E-Mail: fnasser@redhat.com 2323 Yonge Street, Suite #300 Toronto, Ontario M4P 2C9
> As we get older and wiser, there is a tendency to accumulate enough > experience that we end up saying "no, because we had trouble doing this > before..." more often than might be desirable. All's well that ends well. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> > Let me reiterate my hesitation to apply this. First, 7.1.3 was supposed > > to be packaged up Tuesday night (two days ago), meaning I thought it was > > closed already. Second, I don't trust a patch to be 100% safe no matter > > who submits it. We had patches created and applied to 7.1 by core > > members that we thought were safe but in fact they were not and caused > > problems in 7.1.1 that were only fixed in 7.1.2, so "I don't trust no > > one." :-) Third, the evaluation of whether a patch is even safe takes > > time, and the farther we get from a major release, 7.1 in this case, the > > less likely we are to even take the time to evaluate it. > > > > I believe we all agree with that. It just happened that this one was a > win-win situation. The options were: 1) totally broken (without the patch) > 2) Probably fixed (with the patch). There was no chance of affecting > anything else that was not broken before due to the scope of the patch. > > The customer who had the problem applied the patch and is now very happy > with his Python interface. Add this to Gerhards experience and ours, > to the limited scope of the change and the fact that was completely > broken before, and you see that it is a special case. Nice you could test the patch in a live situation. We don't normally get that. > > Having a bug fix is not enough justification to get it backpatched. > > Most people want their patch backpatched, but if you look at the CVS > > HISTORY for 7.1.3 you will see that very few items are backpatched. We > > have over a dozen jdbc bugs fixed in CVS, but we aren't backpatching > > those because we are focused on 7.2, which is pretty close now. People > > who report JDBC problems are encouraged to use the CVS version or the > > one on http://jdbc.fastcrypt.com. > > > > I know it is not fun to ship bugs in 7.1.3 that we know we could fix, > > but with limited resources and testing, we are doing the best we can. > > > > But JDBC for 7.1 was a lost case, while the one line change saves us from Agreed, a lost cause. It was frozen in February, 2001. > having to adopt the same criteria for Python folks. I rather see they use > the version that is shipped from the pgsql tree and help fix it than using > the alternative ones that Gerhard mentioned. (For many, using the > development version is not an option). Yep. > > If someone would like to take the responsibility of backpatching more > > aggressively, we should discuss that. > > > > I personally agree with the current rules. This was an exceptional case > where there was much to gain and little to lose. The only bad thing was > to give you extra work to repack things, for which I apologize. > But you can rest assured that it was worthy of your effort: Python seems to > have a growing number of adepts. Seems it has not been packaged yet, or at least I haven't heard about it, so we should be OK. Sounds good. As long as you don't mind the extra effort you had to make to sell the patch, I think we have a good system. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian wrote: > > Sounds good. As long as you don't mind the extra effort you had to make > to sell the patch, I think we have a good system. > Not at all. This is an important part of the process -- that is how we help each other and prevent bad things to happen. Thanks again. -- Fernando Nasser Red Hat - Toronto E-Mail: fnasser@redhat.com 2323 Yonge Street, Suite #300 Toronto, Ontario M4P 2C9