Обсуждение: Re: SQLState Implementation
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
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
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>
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