Thread: Bug with caching SQLTypes in Connection:getSQLType(oid)

Bug with caching SQLTypes in Connection:getSQLType(oid)

From
Ned Wolpert
Date:
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

Re: Bug with caching SQLTypes in Connection:getSQLType(oid)

From
Rene Pijlman
Date:
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>

Re: Bug with caching SQLTypes in Connection:getSQLType(oid)

From
Antonio Fiol Bonnín
Date:
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)
>
>.
>




Re: Bug with caching SQLTypes in Connection:getSQLType(oid)

From
Ned Wolpert
Date:
--- 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

Re: Bug with caching SQLTypes in Connection:getSQLType(oid)

From
"Dave Cramer"
Date:
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



Re: Bug with caching SQLTypes in Connection:getSQLType(oid)

From
Ned Wolpert
Date:
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

Re: Bug with caching SQLTypes in Connection:getSQLType(oid)

From
Barry Lind
Date:
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()
>>}
>>