ODBC interface

[Fergus Henderson]

Simon TAYLOR <stayl at students.cs.mu.OZ.AU> writes:

>This is the result of my hacking at Mission Critical's ODBC interface.

Ah.  I see.  What do Mission Critical think of this?

To distribute this, we would need their permission.

>Is it OK to distribute a patch for these header files, given that they
>aren't in the section of the ODBC SDK which is redistributable?

Probably not.
(A non-context diff might be OK, but a context one wouldn't be.)

>Also I should probably get permission from the Mission Critical people 
>before this goes in the release, but that shouldn't be a problem.

Please do that.

>=======
>Part 1:
>=======
>
>Estimated hours taken: 3
>
>runtime/engine.h
>runtime/engine.mod
>	Added macros MR_setjmp and MR_longjmp which in save and restore
>	the jmp_buf used to return from call_engine_inner. This is needed 
>	to make longjmp work across C->Mercury calls.
>
>runtime/mercury_string.h
>	Added a macro make_aligned_string_copy, which is the same as
>	make_aligned_string except that it is guaranteed to do the copy.
>	This is useful for copying C strings out of buffers onto the 
>	Mercury heap.
>
>runtime/misc.c
>	Avoid a segfault in debug grades when printing information about
>	the bottom nondetstack frame.

These changes look fine, except for the stuff you noted about
the use of setjmp and longjmp in MR_setjmp and MR_longjmp.

>+	if (s - nondetstack_zone->min > 0) {

How about just

	if (s > nondetstack_zone->min)

?

>	# gcc aborts with optimization enabled.

Could you be more specific here, e.g.

	# gcc 2.7.2.1 for sparc-sun-solaris2.5 aborts when compiling
	# odbc.c if optimization is enabled.

It might be a good idea to figure out why gcc aborts...


>%---------------------------------------------------------------------------%
>% XXX what should go here?
>% Copyright (C) 1997 Mission Critical
>% Copyright (C) 1997 University of Melbourne.
>% This file may only be copied under the terms of the GNU Library General
>% Public License - see the file COPYING.LIB in the Mercury distribution.

I think what you have there is fine, so long as you get Mission Critical's
permission.

>%---------------------------------------------------------------------------%
>% File: odbc.m
>% Authors: Renaud Paquay (rpa at miscrit.be), stayl
>% ODBC version: 2.0
>%
>% The transaction interface used here is described in
>%	Kemp, Conway, Harris, Henderson, Ramamohanarao and Somogyi,
>% 	"Database transactions in a purely declarative 
>%		logic programming language", 
>%	Technical Report 96/45, Department of Computer Science, 
>% 	University of Melbourne, December 1996.

Is there a URL for that report?
There should be.

>	% odbc__tables(QualifierPattern, OwnerPattern, 
>	% 	TableNamePattern, Result)
>	% 
>	% Get a list of database tables matching the given description.
>	% Note that wildcards are not allowed in the CatalogPattern.
>	% This is fixed in ODBC 3.0.

What's CatalogPattern??

> :- type odbc__state == unit.

A brief explanation here would be helpful.

>#define ODBC_VER 0x0250

Ditto.

>#undef DEBUG

Ditto.

>static Word odbc_message_list = list_empty();

There is no guarantee that list_empty() will be a macro suitable
for inclusion in a static initializer.  For some configurations, it is not.

>%-----------------------------------------------------------------------------%
>
>	% Call the transaction closure.
> :- pred odbc__do_transaction(odbc__transaction(T), T,
>		odbc__state, odbc__state).
> :- mode odbc__do_transaction(odbc__transaction, out, di, uo) is det.
>
> :- pragma export(odbc__do_transaction(odbc__transaction, out, di, uo), 
>		"ML_odbc__do_transaction").

Since this isn't part of the Mercury standard library, `ML_' isn't the
right prefix to use here.  I suggest perhaps MODBC_do_transaction.

> :- pred odbc__throw(odbc__state, odbc__state).
> :- mode odbc__throw(di, uo) is erroneous.
>
> :- pragma c_code(odbc__throw(DB0::di, DB::uo),
>		will_not_call_mercury,
>"
>{
>	save_transient_registers();
>	odbc_ret_code = SQL_ERROR;
>	odbc_throw();
>	DB = DB0;
>}

The `DB = DB0;' is unreachable, so change it `/* DB = DB0; (not reached) */'.

>	statement = (MODBC_Statement *) newmem(sizeof(MODBC_Statement));

I would prefer

	statement = make(MODBC_Statement);

>	switch ((int) Type) {

Why is this cast to `(int)' here?

>	    case MODBC_INT:
>
>		Int = (Integer) *(MODBC_SQL_INT *)(col->data);
>#ifdef DEBUG
>			printf(""got integer %ld\\n"", Int);
>#endif /* DEBUG */
>		break;

I think it would be worth checking for overflow here.

		MODBC_SQL_INT data = *(MODBC_SQL_INT *)(col->data);
		Int = (Integer) data;
		if (Int != data) {
			/* handle overflow error */
		}

>	    case MODBC_FLOAT:	
>
>		Flt = *(Float *)(col->data);

Ah, is that a bug?  It sure looks different to the code for ints.

>	} /* switch (Type) */

s/switch/end switch/

>} /* odbc__get_data */

s/odbc__get_data/end odbc__get_data()/

>
> :- pragma c_code("
>
>void
>odbc_do_get_data(MODBC_Statement *stat, int column_id)
>{
>	MODBC_Column 	*column;
>	SQLRETURN 	rc;
>	SDWORD		column_info;
>	char 		dummy_buffer[1]; /*
>					 ** Room for the NULL termination 
>					 ** byte and nothing else.
>					 */

s/NULL/NUL/

>		if (rc != SQL_SUCCESS_WITH_INFO &&
>				! odbc_check(odbc_env_handle, 
>					odbc_connection, 
>					stat->stat_handle, rc)) {
>			odbc_do_cleanup_statement(stat);	
>			odbc_throw();
>		}

In cases like this, where the condition of an if statement wraps lines,  
it is IMHO clearer if the { is on a line of its own.
This indentation convention is described in our C coding guidelines.

>		}
>		else if (column->value_info == SQL_NO_TOTAL) {

Our C coding guidelines also say that the else should go on the
same line as the }.  (Several occurrences in this function.)

XXX you might need to change some of the pragma C code
from `will_not_call_mercury' to `may_call_mercury'.

>
>#ifdef MODBC_MYSQL
>		case SQL_LONGVARBINARY:		return MODBC_STRING;
>		case SQL_LONGVARCHAR:		return MODBC_STRING;
>#else /* ! MODBC_MYSQL */
>		case SQL_LONGVARBINARY:		return MODBC_VAR_STRING;
>		case SQL_LONGVARCHAR:		return MODBC_VAR_STRING;
>#endif /* ! MODBC_MYSQL */

Please explain the #ifdef here.

>	MR_ASSERT_IMPLY(connection_handle == SQL_NULL_HDBC, 
>			statement_handle == SQL_NULL_HSTMT);
>	MR_ASSERT_IMPLY(statement_handle != SQL_NULL_HSTMT, 
>			connection_handle != SQL_NULL_HDBC);

These two assertions both assert the same thing.
Are you sure that's what you intended?
I suggest you delete one of them.

If you want to assert 

	MR_ASSERT_IMPLY(statement_handle != SQL_NULL_HSTMT, 
			connection_handle != SQL_NULL_HDBC);