Re: Performance improvement proposal. Removal of toLowerCase calls. PR

Hi Dave,
A pull request has been created. See [1].

In the commit are:

Changes to the Driver and DataSource configuration. To recognise the
new property called "disableColumnSanitiser". The default value for this
property is false. Comments to rename the property are welcome.

3 additional test case classes added to the jdbc2 test suite. Each class
groups tests for:

a) ColumnSanitiserEnabledTest. Tests to check existing and default
behaviour still works as expected.
b) ColumnSanitiserDisabledTest. Tests to check new behaviour works as
expected.
c) CaseOptimiserDataSourceTest. Checking a datasource configured with
the sanitiser disabled will recognise the property configuration.

I've attempted to conform to the coding guidelines where possible. The
test-suite passes for 1.7 and 1.6 spec. 1.5 fails due to code in master
[1] branch.

Regards,
Jeremy

[1] https://github.com/pgjdbc/pgjdbc/pull/113
[2] http://www.postgresql.org/message-id/52DFA5D0.7000901@redhat.com

On 17/01/14 16:55, Dave Cramer wrote:
> Jeremy,
>
> At this point send a PR and we can test it. If you are proposing
> having an optimization switch which defaults to off I can't see why I
> would not merge it.
>
> Dave Cramer
>
> dave.cramer(at)credativ(dot)ca
> http://www.credativ.ca
>
>
> On Fri, Jan 17, 2014 at 11:48 AM, Jeremy Whiting <jwhiting(at)redhat(dot)com
> <mailto:jwhiting(at)redhat(dot)com>> wrote:
>
> Hi Dave and danap,
>
>
>
> On 16/01/14 19:33, Dave Cramer wrote:
>>
>>
>> On Thu, Jan 16, 2014 at 2:21 PM, dmp <danap(at)ttc-cmc(dot)net
>> <mailto:danap(at)ttc-cmc(dot)net>> wrote:
>>
>> Jeremy Whiting wrote:
>>
>> Hi,
>> I would like to propose an optimization to improve
>> performance in the
>> jdbc driver. The performance improvement has been tested
>> on commodity
>> hardware using an industry standard Java benchmark. The
>> overall
>> benchmark metric reports an improvement in performance.
>> Profiling using
>> sampling showed calls reduced from 1100 to 0 when the
>> benchmark workload
>> is running.
>>
>> The optimization will eliminate calls to the method
>> toLowerCase(java.util.Locale). This in the pg-jdbc
>> org.postgresql.jdbc2.AbstractJdbc2ResultSet.findColumnIndex(java.lang.String)
>> method when setting up the column index registry.
>>
>> For the optimization to be enabled I suggest relying on
>> a new system
>> property. Making the existing functionality the default
>> behaviour to
>> ensure existing applications do not break when the driver
>> is upgraded.
>>
>> The change removes the call toLowerCase when putting
>> items in the
>> registry [1]. Essentially what's being proposed is
>> removing sanitizing
>> the key names. For best performance application code
>> should pass SQL to
>> the driver with column names already folded to lower case.
>>
> I have found this to be incorrect. The column registry is built
> using fields/columns that have come back from the db.
>
>> But upper
>> case names will still be matched in the second lookup [2]
>> in the method.
>>
>>
>> ****************
>>
>> For this optimization to work this feature introduces a
>> requirement on
>> applications. To use all lower or upper case column names.
>>
>> ****************
>>
>> This is going to break existing applications, if the
>> requirement is to have
>> either upper or lowercase.
>>
> Yes it's incompatible. Mixed case situations should use the
> existing behaviour in 9.3-1100 which is proposed to be the default.
>
>> I test explicitly with the MyJSQLView application
>> the use of mixed case column, key index names because there
>> are applications
>> that used mixed case. The proper way to handle is quote to
>> handle the mixed
>> case so it is store as such.
>>
>> Perhaps the optimization could check for quoting then do no
>> addition process
>> and store directly intact. Other wise the optimization could
>> initiate as
>> proposed.
>>
>> danap.
>>
>>
>>
>> Are you sure ? This is in the resultset, so any column names
>> should have come back from the db.
>> Which means that they should come back in lower case anyway.
>>
>> I would think the current code would break apps that having real
>> mixed case columns in the database ?
>>
> Column names coming back from the database are sanitized. Before
> putting into the column registry.
>
>> Jeremy's proposal would leave the case alone and store it in the
>> map with mixed case.
>>
> The proposal has a hard requirement of either all upper or lower
> case columns defined in the database.
>
> If a user attempts to enable the optimization with an application
> that uses mixed case the driver does throw an exception of type
> org.postgresql.util.PSQLException. With this message "The column
> name <column name> was not found in this ResultSet.".
>
> Jeremy
>
>> Dave Cramer
>>
>> dave.cramer(at)credativ(dot)ca
>> http://www.credativ.ca <http://www.credativ.ca/>
>>
>> [2]
>> https://github.com/pgjdbc/pgjdbc/blob/REL9_3_STABLE/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java?#L2752
>>
>>
>>
>>
>> --
>> Sent via pgsql-jdbc mailing list (pgsql-jdbc(at)postgresql(dot)org
>> <mailto:pgsql-jdbc(at)postgresql(dot)org>)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-jdbc
>>
>>
>
>
> --
> Jeremy Whiting
> Senior Software Engineer, Performance Team
> Red Hat
>
> ------------------------------------------------------------
> Registered Address: Red Hat UK Ltd, 64 Baker Street, 4th Floor, London. W1U 7DF. United Kingdom.
> Registered in England and Wales under Company Registration No. 03798903. Directors: Michael Cunningham (USA), Charlie Peters (USA), Matt Parson (USA), Paul Hickey (Ireland)
>
>