Thread: Re: SQLState Implementation

Re: SQLState Implementation

From
Fernando Nasser
Date:
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.

> 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.


> 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.


> 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.


Regards,
Fernando



--
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9


Re: SQLState Implementation

From
Barry Lind
Date:

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



Re: SQLState Implementation

From
Dave Cramer
Date:
On Tue, 2003-08-26 at 03:10, Barry Lind wrote:
> 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.
I suggest you use the names from the server source code in the file
src/include/utils/errcodes.h, there may be some issue keeping them
synchronized, but this is a better start than creating our own names.
>
> >
> >> 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
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
>
--
Dave Cramer <dave@fastcrypt.com>
fastcrypt
--
Dave Cramer <Dave@micro-automation.net>


Re: SQLState Implementation

From
Dave Cramer
Date:
On Tue, 2003-08-26 at 03:10, Barry Lind wrote:
> 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.
I suggest you use the names from the server source code in the file
src/include/utils/errcodes.h, there may be some issue keeping them
synchronized, but this is a better start than creating our own names.
>
> >
> >> 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
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
>
--
Dave Cramer <dave@fastcrypt.com>
fastcrypt