Thanks!

> A few questions:
>
> 1) Am I right in understanding that the only purpose of
> isc_arg_sql_state being added to the status vector is to remove an
> ambiguity for some GDSCODEs for a proper mapping into SQLSTATEs?

In

> other words, we generally rely on the mapping table, but sometimes

force

> the necessary SQLSTATE via isc_arg_sql_state?
>

Yes. We generally rely on the mapping table. We only use the
constant isc_arg_sql_state in DYN module and to disambiguate a
message that could have 2 possible SQLSTATES.

> While I would accept that as an intermediate solution, I'd really

would

> like to avoid the engine internals being SQLSTATE aware, as it

requires

> us to permanently keep SQLSTATEs in mind when throwing GDS errors.

When the developer adds a new GDSCODE, the datafill for SQLSTATE and
SQLCODE has to be added to message database. I don't see a way
around this.

>
> Could we think of any alternative solution? For example, implement

a

> multi-level mapping table and solve the ambiguity issues using

secondary

> GDS codes?
>
> - "String data, right-truncated"
> ERR_post(isc_arith_except, isc_arg_gds, isc_string_truncation, 0);
> mapping: SQLSTATE 22001 <-> {isc_arith_except,

isc_string_truncation}

>
> - "Division by zero"
> ERR_post(isc_arith_except, isc_arg_gds, isc_division_by_zero, 0);
> mapping: SQLSTATE 22012 <-> {isc_arith_except,

isc_division_by_zero}

>

Actually, I only found 1 case where we have an ambiguous message -
the one cited. I thought there would be more. Since there is only 1,
I prefer to disambiguate this message and keep code very simple. I
see there is already a divide by 0 message, perhaps it could be
recycled.

> This doesn't solve the DYN problem, so those errors should be
> de-randomized (i.e. redefined using proper GDS codes in the DYN

facility

> group). But you seem to suggesting that anyway (defining symbols

for DYN

> errors).
>

Yes, the right approach is to remove the use of isc_random and
replace with proper isc constants. The playpen I have today doesn't
do that. I will play around with this and see if I can make it
better.

> Comments?
>
> 2) The standard declares the list of SQLSTATEs separated by class

and

> subclass. PGSQL also documents its error the same way. Thinking

about

> that, does it really make sense to replace columns SQL_CLASS and
> SQL_SUBCLASS with a single one SQL_STATE? Also, with these columns

being

> separated, we could allow subclass values to be omitted (and

implicitly

> treated as '000' -- exactly how the standard defines that).

I can see this might add value. We have SQLSTATE as a computed
column, so the user could use whichever they prefer. I will change
it back.

>
> 3) Is the SQLSTATE value 'HY000' widely used as a generic state

code

> among other RDBMS? The only place where it's mentioned in the SQL
> specification is related to CLI_SPECIFIC_CONDITION_NO_SUBCLASS.

But I

> see 'HY000' being used by MySQL and DB2. Also, I cannot find other
> subclasses of the 'HY' class in the SQL:2003 document.

I asked coworker Jim about this. He said:

HY000 is the generic "something went wrong" message.

It should almost never be used, but almost everyone gets lazy and
uses it as a general purpose error - the problem is that it isn't
helpful to the user.

The HY block is defined as API errors - maybe the other HY values
were defined by Microsoft, but the ODBC doc clearly lists them in
the appendix, and since ODBC was adopted as the SQL CLI standard,
they are defacto official SQLSTATEs.

>
> 4) Please prefix the new API call with "fb_" rather then

with "isc_" :-)

> Also, the "gds__" counterpart is not required, as there's no

legacy

> interface to care about in this case.

ok

>
> 5) I don't think there's a real need to hide SQLSTATE in ISQL by
> default, so perhaps we could live without the SET SQLSTATE

command.

> Although I'd expect the guys maintaining the TCS and QMDB scripts

to

> argue here :-)
>

As a part-time tester these days, that change was driven by my
desire not to rebench all of my tests. :) I think I'll leave it in
for now - when you do the FB3 merge, you can take it out - it is a
small fix.