Re: SQLState Implementation - Mailing list pgsql-jdbc

Fernando Nasser wrote:
> Hi Barry,
>
> Barry Lind wrote:
>
>> Kim, Fernando,
>>
>> I have been studying this patch.  I would like to bounce some of my
>> concerns off of the two of you to see if we (or anyone else on the
>> list) can come up with a better solution.
>>
>> My concerns are:
>>
>> 1) There is no way for anyone else who is adding code to the driver to
>> understand what all these SQL_STATE values mean.  For example if I
>> were to add code that threw a sql exception now, I would have no idea
>> what (if any) sql state value should be included.  To me they are
>> random five digit strings that have no meaning.  To be maintainable
>> there needs to be a way for all contributors to understand what each
>> value means and what the list of values the driver uses are.
>>
>
> The SQLState values are defined in the SQL Standard.  They are not
> RANDOM values, they are standard values.
>
> Whenever the condition was in the implementation define range we used
> the "de facto" standard values as assigned by the ODBC (also used by
> both DB2 and Oracle).
>
> All these are public documents.

I should have put random in quotes.  I know that they are not truely
random, but if one doesn't know what the values mean, they appear that
way.  While it may be true that these are all documented somewhere, the
problem is that I don't know where that is.  So at a minimum there needs
to be comments in the relevant code that point myself (and any others
who will ever add code to the driver that throws a sql exception) to the
correct location, so that we can do the right thing when it comes to the
sql state values.

>
>> 2) This is related to the first, but I don't like the use of String
>> values in the constructor to SqlState.  They all look the same and
>> from a code review standpoint, even if I knew what all the values
>> were, it would be too easy to either mistype or otherwise pass in the
>> wrong value.  In something like this I think some sort of constants
>> with descriptive names would be much better than the five digit codes
>> (get some compile time checking).
>>
>
> Good suggestion.  Kim is adding constants to the PSQLState class so the
> 5 character strings will become something like PSQLState.SomeCondition.
>

I think that will go a long way to address issue one above as well.

>
>> 3) There are now too many constructors (IMHO) for PSQLException.  It
>> isn't obvious what constructor does what, and under what circumstances
>> you would use one or the other.  Perhaps the structure of this class
>> needs some thought.
>>
>
> This is not our doing.  This is the current state of the driver.  Kim is
> actually proposing a considerably smaller number of signatures just to
> improve this situation.
>

In part it is your doing, since the patch as it stands only makes things
worse by adding yet more.

>
>> 4) Related to 3 above, is it necessary to still have the old
>> constructors for PSQLException that don't take a SqlState object, or
>> should all PSQLExceptions have a SqlState, even if much of the time it
>> is some generic standard value?
>>
>
> As Kim and I mentioned in some of the messages we've sent to you, we are
> leaving the old signatures in place so that the conversion can be done
> in steps.  They will go away after all calls are replaced.  We thought
> this was better than sending a too pervasive mega-patch.
>

Then the old signatures should be marked deprecated.

I appreciate the thought about not sending a mega-patch, but at the same
time I don't like seeing things half done.  I guess I just want it both
ways :-).  As long as I have your commitment that the rest is coming
this is fine for now.

Thanks for the work on this feature.  This feature is a long standing
user requested enhancement to the driver.  It is good to see that it is
getting done in 7.4.

thanks,
--Barry