Menu ▾ ▴

#103 Time conversion, data truncation, etc.

closed
momo
None
8
2012-08-15
2009-04-04
Rainer Schwarze
No

Hi,

the attached patch addresses various issues in the 1.2.2 release of jTDS:

1) Time conversion with user supplied Calendars:

With default TimeZone "America/New_York", setting Timestamp 2009-03-08 02:00:00 with a UTC Calendar is converted to 2009-03-08 03:00:00 (note 03:00 instead of 02:00). This is because at that time the DST switch occurs and 02:00 in the zone America/New_York doesn't exist (01:59 -> 03:00).

The patch addresses this problem by creating DateTime instances directly upon calls such as PreparedStatement.setTimestamp(int, Timestamp, Calendar) and reading from DateTime instances directly in calls such as ResultSet.getTimestamp(int, Calendar).

2) PreparedStatement.executeBatch() ... ..executeUpdate() cause data truncation at 4000 or 8000 bytes for big sized columns under certain circumstances.

I attach the DataTruncTest.java in addition to the patch (it is included in the patch) to look at the problem more easily.

We use subversion internally. The diff has been created using the appropriate subversion command. In my environment I can apply the patch to a CVS based project. If this doesn't work here, please tell me and I'll fix it.

This patch changes some parts heavily and it is probably worth to discuss some details.
Please advice where the best place is to discuss the changes (comments here or forum or e-mail?).

Thanks and best wishes,
Rainer Schwarze

Discussion

  • Rainer Schwarze

    Rainer Schwarze - 2009-04-04

    The patch file

     
    jtds-1.2.2-misc.diff

  • Rainer Schwarze

    Rainer Schwarze - 2009-04-04

    Test case for data truncation

     
    DataTruncTest.java

  • momo

    momo - 2009-07-27

    Thank you for reporting those issues and proposing solutions! The data truncation problem is now addressed in CVS. I'll take a look at the time conversion issue later.

    Just for the records:
    The problem was caused by the fact, that jTDS may choose different native data types for certain JDBC data types, depending on the data actually provided. This, in turn, may cause different procedures being created for a single PreparedStatement object. Because of an assignment problem, a normal executeXYZ() operation then used the last created procedure of a previously executed batch operation. This potentially caused usage of incorrect data types and various conversion problems, including data truncation.

     

  • grimlock81

    grimlock81 - 2009-11-06

    What is the current status of this proposed patch?

     

  • momo

    momo - 2009-11-09

    Unfortunately, I did not yet find the time to look into the time conversion issue. If I got you right, this patch should fix bug [2887672]. Did you already have the time to give it a try?

     

  • Nobody/Anonymous

    Nobody/Anonymous - 2009-11-10

    Yes I applied the patch to 1.2.2 and ran all unit tests (the ones supplied with 1.2.2 and the ones introduced in the patch). In addition, I also ran the test case I submitted for bug [2887672].

    They all passed except for one test. The test that failed was DatabaseMetaDataTest.testProcedureColumns() but this also failed before the patch was applied.

    I've also had a look at the code, and I'm happy with the changes. The only point of contention is the change in the usage of Calendar objects. They were previously associated to a ThreadLocal, one in Support and one in DateTime.

    After the patch, there is one Calendar object per ConnectionJDBC2 object and synchronization is done when the Calendar is used.

    I am unsure of the implications of this change, performance or otherwise, given I'm not too familiar with the threading model used in jtds. Perhaps someone else can shed some light on this.

    I'm also prepared to put in the work to integrate this patch into the current version if needed. Please let me know.

     

  • grimlock81

    grimlock81 - 2009-11-10

    Sorry I didn't realise I wasn't logged in. The previous comment was made by me.

     

  • momo

    momo - 2009-11-10

    I just took a look at the patch. I currently see two problems here. First, the patch (as you already pointed out) moves from ThreadLocal objects to synchronizing local Calendar instances, thus (maybe heavily, depending on usage) degrading performance. Second, I'm still unsure whether the patch really provides completely correct behavior.

    I'll have to dig a bit deeper into this and maybe create a number of additional tests. Nevertheless, if you are willing to adopt the patch to the current code base in advance, your efforts are highly appreciated.

    Cheers,
    momo

     

  • grimlock81

    grimlock81 - 2009-11-11

    We could always switch the Calendar instance in ConnectionJDBC2.java to a private static ThreadLocal which should mirror the behaviour in 1.2.4. 1.2.2 did not use ThreadLocal, which was only introduced in 1.2.3 to fix issue 1955499.

    Is there any particular facet of the patch that you are concerned about, or are you just not 100% confident until more tests are passed?

    I'll try and get started soon.

     

  • momo

    momo - 2009-11-13

    No, there is nothing particular. I just have some feeling to go round in circles so I want to avoid to introduce any new/old errors in the date/time handling under all circumstances.

     

Anonymous
Anonymous

Add attachments
Cancel