Thread: Bug with caching SQLTypes in Connection:getSQLType(oid)
Folks- I've found a possible bug but I'm trying to located a fix for it. In some cases, when get getSQLType(oid) method is called, it doesn't cache the result, so it gets called alot. This is seen in Castor in a few cases. Anyone seen this before or am I the first? -- Virtually, Ned Wolpert <ned.wolpert@knowledgenet.com> D08C2F45: 28E7 56CB 58AC C622 5A51 3C42 8B2B 2739 D08C 2F45
Attachment
On 07 Dec 2001 15:09:51 -0700, you wrote: >In some cases, when get getSQLType(oid) method is called, it doesn't >cache the result, so it gets called alot. This is seen in Castor in >a few cases. Anyone seen this before or am I the first? We've discussed this before: http://archives2.us.postgresql.org/pgsql-jdbc/2001-09/msg00275.php. It's not entirely clear from the rest of the thread if the problem disappeared when the OP upgraded to the latest development version, but he did say that the improvement seemed to be already implemented. Regards, René Pijlman <rene@lab.applinet.nl>
I was the one that started the stated thread. Sorry about not following up, but I really do not know whether it was fixed or not. I cannot remember why, but I know that I compiled a new driver, and it did not get on very well with the backend (I think). Anyway, the development was nearly finished, and when I moved to the production server, not caching the requests was not critical, as the client and the server are on two machines on the same subnet. The development machine was in a different country than the server. So, I "forgot" about the problem. I think the interoperation issues were due to the fact we are on a 7.0.3 server, or something like that. We may be changing our server from a 7.0.3 to a 7.1 or 7.2 by January so I may test that again (new developments using similar techniques will take place). BTW, which one do you recommend, 7.1 or 7.2? The server is a RH6.2 2xPIII (800) 512Mb RAM, upgrading to 1Gb soon. The database size is about 3Gb, and growing every day. Servlet operation against the database is the main performance concern, and the server is most of the time doing many INSERTs on different tables, and precalculating statistics (SELECT - INSERT). So, I recall: I was about to test more, but I could not. I may soon have a second opportunity. Sorry for not clarifying that before. Antonio Fiol Rene Pijlman wrote: >On 07 Dec 2001 15:09:51 -0700, you wrote: > >>In some cases, when get getSQLType(oid) method is called, it doesn't >>cache the result, so it gets called alot. This is seen in Castor in >>a few cases. Anyone seen this before or am I the first? >> > >We've discussed this before: >http://archives2.us.postgresql.org/pgsql-jdbc/2001-09/msg00275.php. > >It's not entirely clear from the rest of the thread if the >problem disappeared when the OP upgraded to the latest >development version, but he did say that the improvement seemed >to be already implemented. > >Regards, >René Pijlman <rene@lab.applinet.nl> > >---------------------------(end of broadcast)--------------------------- >TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > >. >
--- Antonio Fiol Bonn�n <fiol@w3ping.com> wrote: > Well, It is *certain* that the existing code is somehow broken. And it > is visible even at first sight. How to correct it is probably harder, Yes, I noticed that too. The 'quick/easy' fix that I'm using is to update sqlTypeCache hashtable which mapped 'oid -> SQLType'. That was the patch that I sent in. It doesn't break anything 'more' since a) if its not in cache it puts the value in the sqlTypeCache hashtable to begin with and b) if it is found in the check, then a potentially bogus result is returned. (Odds are it won't, since it currently checks typeOidCache, which maps 'PGType -> oid' instead. I doubt one would ever have an oid they send into the function, which would then check the typeOidCache. The oid key wouldn't match the PGType key to find the oid value. Ugh... my brain hurts... ;-) > but if you look into the following non abstract method, getOID(String), > you will see that it is looking up in the same hashtable, but using a > different class. Yes. in getOID(String), it uses the typeOidCache correctly. (If the comments defining the variable typeOidCache are correct.) It maps the string of the PGType -> the Integer oid. > In getSQLType(int), it is looking up an entry for an > Integer. In getOID(String), it is looking up in the same hashtable, but > using a String. This is obviously wrong or a very bad programming > practice. (I vote wrong). Yeah, I think it was just a mistake made that didn't give incorrect results since the item is never cached. (Or, can never be found.) > I can't go much deeper by now. Sorry. Thanks, you've given me the info I need on the original discussion. I think we should see if we can put this fix in before 7.2 is released. Perchance in the 7.2rc1? Folks? Can someone else verify that our analysis is correct before the rc1 release? ===== Virtually, | "Must you shout too?" Ned Wolpert | -Dante wolpert@yahoo.com | _________________/ "Who watches the watchmen?" 4e75 -Juvenal, 120 AD -- Place your commercial here -- fnord __________________________________________________ Do You Yahoo!? Send your FREE holiday greetings online! http://greetings.yahoo.com
If we are going to try to get this in as a bug fix for 7.2RC1 then I would like to see something more conclusive. Can we verify that the patch works, or fixes something? Dave -----Original Message----- From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Ned Wolpert Sent: Sunday, December 09, 2001 4:49 PM To: pgsql-jdbc@postgresql.org Subject: Re: [JDBC] Bug with caching SQLTypes in Connection:getSQLType(oid) --- Antonio Fiol Bonnín <fiol@w3ping.com> wrote: > Well, It is *certain* that the existing code is somehow broken. And it > is visible even at first sight. How to correct it is probably harder, Yes, I noticed that too. The 'quick/easy' fix that I'm using is to update sqlTypeCache hashtable which mapped 'oid -> SQLType'. That was the patch that I sent in. It doesn't break anything 'more' since a) if its not in cache it puts the value in the sqlTypeCache hashtable to begin with and b) if it is found in the check, then a potentially bogus result is returned. (Odds are it won't, since it currently checks typeOidCache, which maps 'PGType -> oid' instead. I doubt one would ever have an oid they send into the function, which would then check the typeOidCache. The oid key wouldn't match the PGType key to find the oid value. Ugh... my brain hurts... ;-) > but if you look into the following non abstract method, > getOID(String), > you will see that it is looking up in the same hashtable, but using a > different class. Yes. in getOID(String), it uses the typeOidCache correctly. (If the comments defining the variable typeOidCache are correct.) It maps the string of the PGType -> the Integer oid. > In getSQLType(int), it is looking up an entry for an > Integer. In getOID(String), it is looking up in the same hashtable, but > using a String. This is obviously wrong or a very bad programming > practice. (I vote wrong). Yeah, I think it was just a mistake made that didn't give incorrect results since the item is never cached. (Or, can never be found.) > I can't go much deeper by now. Sorry. Thanks, you've given me the info I need on the original discussion. I think we should see if we can put this fix in before 7.2 is released. Perchance in the 7.2rc1? Folks? Can someone else verify that our analysis is correct before the rc1 release? ===== Virtually, | "Must you shout too?" Ned Wolpert | -Dante wolpert@yahoo.com | _________________/ "Who watches the watchmen?" 4e75 -Juvenal, 120 AD -- Place your commercial here -- fnord __________________________________________________ Do You Yahoo!? Send your FREE holiday greetings online! http://greetings.yahoo.com ---------------------------(end of broadcast)--------------------------- TIP 6: Have you searched our list archives? http://archives.postgresql.org
I included a pretty chessy test case for this in the email. You'll have to watch the log output with debug_query set to true in the postgresql conf file. Look for the number of times the query is executed. With the current cvs version, the query "select typname from pg_type where oid = 1043" occurs four times. With my fix, it occurs once. (args are url, username and password) On Mon, 2001-12-10 at 12:43, Dave Cramer wrote: > If we are going to try to get this in as a bug fix for 7.2RC1 then I > would like to see something more conclusive. > > Can we verify that the patch works, or fixes something? > > Dave > > -----Original Message----- > From: pgsql-jdbc-owner@postgresql.org > [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Ned Wolpert > Sent: Sunday, December 09, 2001 4:49 PM > To: pgsql-jdbc@postgresql.org > Subject: Re: [JDBC] Bug with caching SQLTypes in > Connection:getSQLType(oid) > > > --- Antonio Fiol Bonnín <fiol@w3ping.com> wrote: > > Well, It is *certain* that the existing code is somehow broken. And it > > is visible even at first sight. How to correct it is probably harder, > > Yes, I noticed that too. The 'quick/easy' fix that I'm using is to > update sqlTypeCache hashtable which mapped 'oid -> SQLType'. That was > the patch that I sent in. It doesn't break anything 'more' since a) if > its not in cache it puts the value in the sqlTypeCache > hashtable to begin with and b) if it is found in the check, then a > potentially bogus result is returned. (Odds are it won't, since it > currently checks typeOidCache, which maps 'PGType -> oid' instead. I > doubt one would ever have an oid they send into the function, which > would then check the typeOidCache. The oid key wouldn't match the > PGType key to find the oid value. Ugh... my brain hurts... ;-) > > > but if you look into the following non abstract method, > > getOID(String), > > you will see that it is looking up in the same hashtable, but using a > > different class. > > Yes. in getOID(String), it uses the typeOidCache correctly. (If the > comments defining the variable typeOidCache are correct.) It maps the > string of the PGType -> the Integer oid. > > > In getSQLType(int), it is looking up an entry for an > > Integer. In getOID(String), it is looking up in the same hashtable, > but > > using a String. This is obviously wrong or a very bad programming > > practice. (I vote wrong). > > Yeah, I think it was just a mistake made that didn't give incorrect > results since the item is never cached. (Or, can never be found.) > > > I can't go much deeper by now. Sorry. > > Thanks, you've given me the info I need on the original discussion. I > think we should see if we can put this fix in before 7.2 is released. > Perchance in the 7.2rc1? Folks? Can someone else verify that our > analysis is correct before the rc1 release? > > ===== > Virtually, | "Must you shout too?" > Ned Wolpert | -Dante > wolpert@yahoo.com | > _________________/ "Who watches the watchmen?" > 4e75 -Juvenal, 120 AD > > -- Place your commercial here -- fnord > > __________________________________________________ > Do You Yahoo!? > Send your FREE holiday greetings online! http://greetings.yahoo.com > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org > > > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) -- Virtually, Ned Wolpert <ned.wolpert@knowledgenet.com> D08C2F45: 28E7 56CB 58AC C622 5A51 3C42 8B2B 2739 D08C 2F45
Attachment
OK, I finally got a few hours to look into this and I agree with Ned's findings. It is just a one line fix, which I have committed and built new jar files and placed up on the website. --Barry Ned Wolpert wrote: > I included a pretty chessy test case for this in the email. You'll have > to watch the log output with debug_query set to true in the postgresql > conf file. Look for the number of times the query is executed. With > the current cvs version, the query "select typname from pg_type where > oid = 1043" occurs four times. With my fix, it occurs once. > (args are url, username and password) > > > On Mon, 2001-12-10 at 12:43, Dave Cramer wrote: > >>If we are going to try to get this in as a bug fix for 7.2RC1 then I >>would like to see something more conclusive. >> >>Can we verify that the patch works, or fixes something? >> >>Dave >> >>-----Original Message----- >>From: pgsql-jdbc-owner@postgresql.org >>[mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Ned Wolpert >>Sent: Sunday, December 09, 2001 4:49 PM >>To: pgsql-jdbc@postgresql.org >>Subject: Re: [JDBC] Bug with caching SQLTypes in >>Connection:getSQLType(oid) >> >> >>--- Antonio Fiol Bonnín <fiol@w3ping.com> wrote: >> >>>Well, It is *certain* that the existing code is somehow broken. And it >>>is visible even at first sight. How to correct it is probably harder, >>> >>Yes, I noticed that too. The 'quick/easy' fix that I'm using is to >>update sqlTypeCache hashtable which mapped 'oid -> SQLType'. That was >>the patch that I sent in. It doesn't break anything 'more' since a) if >>its not in cache it puts the value in the sqlTypeCache >>hashtable to begin with and b) if it is found in the check, then a >>potentially bogus result is returned. (Odds are it won't, since it >>currently checks typeOidCache, which maps 'PGType -> oid' instead. I >>doubt one would ever have an oid they send into the function, which >>would then check the typeOidCache. The oid key wouldn't match the >>PGType key to find the oid value. Ugh... my brain hurts... ;-) >> >> >>>but if you look into the following non abstract method, >>>getOID(String), >>>you will see that it is looking up in the same hashtable, but using a >>>different class. >>> >>Yes. in getOID(String), it uses the typeOidCache correctly. (If the >>comments defining the variable typeOidCache are correct.) It maps the >>string of the PGType -> the Integer oid. >> >> >>>In getSQLType(int), it is looking up an entry for an >>>Integer. In getOID(String), it is looking up in the same hashtable, >>> >>but >> >>>using a String. This is obviously wrong or a very bad programming >>>practice. (I vote wrong). >>> >>Yeah, I think it was just a mistake made that didn't give incorrect >>results since the item is never cached. (Or, can never be found.) >> >> >>>I can't go much deeper by now. Sorry. >>> >>Thanks, you've given me the info I need on the original discussion. I >>think we should see if we can put this fix in before 7.2 is released. >>Perchance in the 7.2rc1? Folks? Can someone else verify that our >>analysis is correct before the rc1 release? >> >>===== >>Virtually, | "Must you shout too?" >>Ned Wolpert | -Dante >>wolpert@yahoo.com | >>_________________/ "Who watches the watchmen?" >>4e75 -Juvenal, 120 AD >> >>-- Place your commercial here -- fnord >> >>__________________________________________________ >>Do You Yahoo!? >>Send your FREE holiday greetings online! http://greetings.yahoo.com >> >>---------------------------(end of broadcast)--------------------------- >>TIP 6: Have you searched our list archives? >> >>http://archives.postgresql.org >> >> >> >>---------------------------(end of broadcast)--------------------------- >>TIP 2: you can get off all lists at once with the unregister command >> (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) >> >> >>------------------------------------------------------------------------ >> >>import java.sql.*; >>import org.postgresql.Connection; >> >>public class PostgreSQLTest { >> >> public void runTest (String []str) throws Exception { >> String url = str[0]; >> String user = str[1]; >> String password = str[2]; >> >> Class.forName("org.postgresql.Driver").newInstance(); >> org.postgresql.Connection c = (org.postgresql.Connection) DriverManager.getConnection(url, user,password); >> // look up a void oid >> for ( int i=0;i<5;i++) { >> c.getSQLType(1043); >> } >> } >> >> public static void main(String[] args) { >> try { >> PostgreSQLTest test = new PostgreSQLTest(); >> test.runTest(args); >> } catch (Exception e) { >> e.printStackTrace(); >> } // end of try-catch >> } // end of main() >>} >>