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

Hi Jeremy,

I saw the PR. Thanks

What tests fail in 1.5 ?

Dave Cramer

dave.cramer(at)credativ(dot)ca
http://www.credativ.ca

On Wed, Jan 22, 2014 at 2:11 PM, Jeremy Whiting <jwhiting(at)redhat(dot)com> wrote:

> 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>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> 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
>>
>>> [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)
>>> 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)
>>
>>
>
>