Parameterized execution #11

bpintea · 2018-07-10T22:13:38Z

This PR adds support for binding parameters to statements.

One statement is typically first prepared, then executed. However, if the statement is to be executed multiple times with different data or if this data is not available at preparation time (i.e. it is obtained dynamically), the application can bind parameters to the statement and the and feed the data just before, or during execution.

Just as in the case of retrieving data from it, sending data to the data server involves data conversions: the application binds an SQL C buffer containing the data and specifies an SQL data type for this data to be converted to.

This PR adds the logic for binding parameters to a statement, as well as the support for data conversions (SQL_C_XXX => SQL_XXX).

Indicate lack of support for arrays and statement batching.

- also, convert some option values to the data type of the var collecting them.

(access rights will get later a utest)

…execution

As well as: - refactored types convertability check (now using a matrix); - small changes on record type setting (needed since the verbose SQL_DATETIME type overlaps SQL_C_DATE value); - split JSON body building from request dispatching; - timeout statement attribute now taken into account; (- data conversion - C -> SQL - not yet implemented)

- catalog functions can leave string pointers unset, but only set their lengths to 0; - remove all SQL[_C]_DATE/TIME/TIMESTAMP references: the DM will do the mapping to the new _TYPE values; - METATYPE_MAX is now used for SQL_C_DEFAULT in app descriptors and for ES/SQL's SQL_NULL in implementation ones; - boolean (and BIT) conversions to strings changed to "0"/"1" (as per spec);

edsavage

I've raised a few minor points/questions but other than that, this looks good to me Bogdan

edsavage · 2018-07-12T09:18:05Z

driver/connect.c

@@ -1002,7 +878,7 @@ static SQLRETURN process_config(esodbc_dbc_st *dbc, config_attrs_st *attrs)
 	/*
 	 * request timeout for liburl: negative reset to 0
 	 */
-	if (! wstr2llong(&attrs->timeout, &timeout)) {
+	if (! str2bigint(&attrs->timeout, /*wide?*/TRUE, (SQLBIGINT *)&timeout)) {
 		ERRH(dbc, "failed to convert '" LWPDL "' to long long.",


nitpick: The error message is now not strictly appropriate for the conversion failure. Is it worth updating?

I've updated it (BIGINT is a long long, but this way is clearer).

edsavage · 2018-07-12T09:38:43Z

driver/handles.c

 */
-static void set_defaults_from_type(esodbc_rec_st *rec)
+static void set_defaults_from_meta_type(esodbc_rec_st *rec)
 {
 	DBGH(rec->desc, "(re)setting record@0x%p lengt/precision/scale to "


typo: lengt -> length

Thanks, fixed.

edsavage · 2018-07-12T09:40:50Z

driver/handles.c

@@ -1738,15 +1764,32 @@ static void set_defaults_from_type(esodbc_rec_st *rec)
 				rec->precision = 6;


Might be worth using named constants for the precision values?

Thanks, fixed.
These were forgotten (as API mandated vs. "implementation defaults" which had already been named.)

edsavage · 2018-07-12T09:52:46Z

driver/odbc.c

@@ -509,9 +509,17 @@ SQLRETURN SQL_API SQLBindParameter(
 	SQLLEN             cbValueMax,
 	SQLLEN             *pcbValue)
 {
-	RET_NOT_IMPLEMENTED;
+	SQLRETURN ret;
+	TRACE10(_IN, "pudddudpdp", hstmt, ipar, fParamType, fCType,


Are these trace statements still required at this stage?

Yes, they would help, since I don't log within the driver all the action taken by the Driver Manager.
And should something go wrong, Microsoft's DM is offering some logging support, but only for the north side of the API (app <-> DM), leaving the south one (DM <-> driver) uncovered. The two sides are not 1:1 mappings.

edsavage · 2018-07-12T11:02:20Z

driver/queries.c

+		}
+	}
+
+	if (! dest) {


I'm curious why the 'cnt' field is updated regardless of whether dest exists.

Also should we not log an error and return FALSE if dest is null?

All the SQL C to ES/SQL data conversions go through two steps to avoid extra copies (or limitations for local buffers) and/or allocations:

a size estimation step, where dest - the string buffer to copy the converted data into - is NULL (and this is the indicator that the functions are called just for size estimation); and

a conversion step: once the needed room for serialisation is calculated and buffer allocated, dest will point to it and the functions will convert the data.

edsavage · 2018-07-12T12:09:26Z

driver/queries.c

+		trim_ws(&xstr.c);
+	}
+
+	ts_buff.str = dest;


Should there be a check for dest existing?

The check is done in the calling function. But I've added an assert.

edsavage · 2018-07-12T12:12:23Z

driver/queries.c

+			RET_HDIAG(stmt, SQL_STATE_HY000, "param conversion bug", 0);
+	}
+
+	cnt = snprintf(dest, sizeof(ESODBC_ISO8601_TEMPLATE) - 1,


again, should dest be checked before use?

Added another assert here too.

- replaced constants with named ones; - ~s/leng*/length; - more accurate logging messages; - added some more asserts.

bpintea added 9 commits June 26, 2018 13:16

no support for params arrays or statement batching

1401503

Indicate lack of support for arrays and statement batching.

small fix: indicate catalog support

f4e9aab

- also, convert some option values to the data type of the var collecting them.

b/f: fix c&p error

a7ec890

(access rights will get later a utest)

Merge branch 'feature/string_conversions' into feature/parameterized_…

0a45965

…execution

add parameter conversions to SQL numeric types

e295e5d

added param conversions to (ES) DATE (SQL TIMESTAMP)

1880312

add param conversion for varchar

92d8d10

bpintea mentioned this pull request Jul 10, 2018

TODOs #2

Closed

bpintea requested review from droberts195 and edsavage July 10, 2018 22:14

edsavage approved these changes Jul 12, 2018

View reviewed changes

coding style improvements

6fe2865

- replaced constants with named ones; - ~s/leng*/length; - more accurate logging messages; - added some more asserts.

bpintea merged commit 4425c09 into elastic:master Jul 19, 2018

bpintea deleted the feature/parameterized_execution branch July 19, 2018 17:44

bpintea added >feature Applicable to PRs adding new functionality v6.5.0 labels May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Parameterized execution #11

Parameterized execution #11

Uh oh!

bpintea commented Jul 10, 2018

Uh oh!

edsavage left a comment

Uh oh!

edsavage Jul 12, 2018

Uh oh!

bpintea Jul 19, 2018

Uh oh!

edsavage Jul 12, 2018

Uh oh!

bpintea Jul 19, 2018

Uh oh!

edsavage Jul 12, 2018

Uh oh!

bpintea Jul 19, 2018

Uh oh!

edsavage Jul 12, 2018

Uh oh!

bpintea Jul 19, 2018

Uh oh!

edsavage Jul 12, 2018

Uh oh!

bpintea Jul 19, 2018

Uh oh!

edsavage Jul 12, 2018

Uh oh!

bpintea Jul 19, 2018

Uh oh!

edsavage Jul 12, 2018

Uh oh!

bpintea Jul 19, 2018

Uh oh!

Uh oh!

		@@ -1738,15 +1764,32 @@ static void set_defaults_from_type(esodbc_rec_st *rec)
		rec->precision = 6;

Parameterized execution #11

Parameterized execution #11

Uh oh!

Conversation

bpintea commented Jul 10, 2018

Uh oh!

edsavage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!