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

Hi Dave,
It's not tests that fail but compilation errors.

The compile phase output is attached to an email in a new thread I
started earlier. Subject is "Master branch compile error using 1.5 spec.".

Jeremy

On 22/01/14 19:38, Dave Cramer wrote:
> 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
> <mailto: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 <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)
>>
>>
>
>