From a455f7c3e883fd8f46c12177e7732809b3267fae Mon Sep 17 00:00:00 2001 From: Mike Rylander Date: Tue, 22 Oct 2019 10:09:47 -0400 Subject: [PATCH] LP#1847805: Avoid using cursors for .retrieve calls There's no need to spend the overhead of cursors on pcrud requests that we know will only return one record, so we skip it in that case. Signed-off-by: Mike Rylander --- Open-ILS/src/c-apps/oils_sql.c | 158 +++++++++++++++++++++++++---------------- 1 file changed, 97 insertions(+), 61 deletions(-) diff --git a/Open-ILS/src/c-apps/oils_sql.c b/Open-ILS/src/c-apps/oils_sql.c index c4fc6aad8c..d8afb6dd38 100644 --- a/Open-ILS/src/c-apps/oils_sql.c +++ b/Open-ILS/src/c-apps/oils_sql.c @@ -93,7 +93,7 @@ static char* searchPredicate ( const ClassInfo*, osrfHash*, jsonObject*, osrfMet static char* searchJOIN ( const jsonObject*, const ClassInfo* left_info ); static char* searchWHERE ( const jsonObject* search_hash, const ClassInfo*, int, osrfMethodContext* ); static char* buildSELECT( const jsonObject*, jsonObject* rest_of_query, - osrfHash* meta, osrfMethodContext* ctx, char** cursor_name ); + osrfHash* meta, osrfMethodContext* ctx, char* cursor_name ); static char* buildOrderByFromArray( osrfMethodContext* ctx, const jsonObject* order_array ); char* buildQuery( osrfMethodContext* ctx, jsonObject* query, int flags ); @@ -5492,7 +5492,7 @@ static char* buildOrderByFromArray( osrfMethodContext* ctx, const jsonObject* or The SELECT statements built here are distinct from those built for the json_query method. */ static char* buildSELECT ( const jsonObject* search_hash, jsonObject* rest_of_query, - osrfHash* meta, osrfMethodContext* ctx, char** cursor_name ) { + osrfHash* meta, osrfMethodContext* ctx, char* cursor_name ) { const char* locale = osrf_message_get_last_locale(); @@ -5607,9 +5607,8 @@ static char* buildSELECT ( const jsonObject* search_hash, jsonObject* rest_of_qu if( !table ) table = strdup( "(null)" ); - *cursor_name = random_cursor_name(); - - buffer_fadd( sql_buf, "DECLARE \"%s\" NO SCROLL CURSOR WITH HOLD FOR SELECT %s FROM %s AS \"%s\"", *cursor_name, col_list, table, core_class ); + if (cursor_name) buffer_fadd( sql_buf, "DECLARE \"%s\" NO SCROLL CURSOR WITH HOLD FOR ", cursor_name ); + buffer_fadd( sql_buf, "SELECT %s FROM %s AS \"%s\"", col_list, table, core_class ); free( col_list ); free( table ); @@ -5820,6 +5819,25 @@ static char* buildSELECT ( const jsonObject* search_hash, jsonObject* rest_of_qu } free( order_by_list ); + + } + + if (!cursor_name) { + const jsonObject* limit = jsonObjectGetKeyConst( rest_of_query, "limit" ); + if( limit ) { + const char* str = jsonObjectGetString( limit ); + if (str) { + buffer_fadd( sql_buf, " LIMIT %d", atoi(str) ); + } + } + + const jsonObject* offset = jsonObjectGetKeyConst( rest_of_query, "offset" ); + if( offset ) { + const char* str = jsonObjectGetString( offset ); + if (str) { + buffer_fadd( sql_buf, " OFFSET %d", atoi(str) ); + } + } } } @@ -5945,9 +5963,11 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ int offset = 0; int limit = 0; int cursor_page_size = 5; - char* cursor_name; + char* cursor_name = NULL; + + if (*methodtype != 'r') cursor_name = random_cursor_name(); /* Use a cursor for all but .retrieve */ - char* sql = buildSELECT( where_hash, query_hash, class_meta, ctx, &cursor_name ); + char* sql = buildSELECT( where_hash, query_hash, class_meta, ctx, cursor_name ); if( !sql ) { osrfLogDebug( OSRF_LOG_MARK, "Problem building query, returning NULL" ); *err = -1; @@ -5990,25 +6010,27 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ } - const jsonObject* limit_o = jsonObjectGetKeyConst( query_hash, "limit" ); - if( limit_o ) { - limit = (int) jsonObjectGetNumber( limit_o ); - } else { - limit = -1; - } + if (cursor_name) { + const jsonObject* limit_o = jsonObjectGetKeyConst( query_hash, "limit" ); + if( limit_o ) { + limit = (int) jsonObjectGetNumber( limit_o ); + } else { + limit = -1; + } - const jsonObject* offset_o = jsonObjectGetKeyConst( query_hash, "offset" ); - if( offset_o ) { - offset = (int) jsonObjectGetNumber( offset_o ); - } else { - offset = -1; - } + const jsonObject* offset_o = jsonObjectGetKeyConst( query_hash, "offset" ); + if( offset_o ) { + offset = (int) jsonObjectGetNumber( offset_o ); + } else { + offset = -1; + } - // set cursor_page_size to the largest of the three - if (cursor_page_size < offset) cursor_page_size = offset; - if (cursor_page_size < limit) cursor_page_size = limit; + // set cursor_page_size to the largest of the three + if (cursor_page_size < offset) cursor_page_size = offset; + if (cursor_page_size < limit) cursor_page_size = limit; + } - // Declares a cursor + // Declares a cursor or runs a retrieve-select dbi_result result = dbi_conn_query( dbhandle, sql ); if( NULL == result ) { @@ -6058,34 +6080,38 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ rs_size = (int *) safe_malloc( sizeof(int) ); // will be freed by sessionDataFree() unsigned long long result_count = cursor_page_size; + if (!cursor_name) { + result_count = dbi_result_get_numrows( result ); + } *rs_size = (int) result_count * (flesh_depth + 1); // yes, we could lose some bits, but come on osrfHashSet( (osrfHash *) ctx->session->userData, rs_size, "rs_size_req_%d", ctx->request ); } + if (cursor_name) { + dbi_result_free( result ); // toss the declare result + result = dbi_conn_queryf( dbhandle, "FETCH %d FROM \"%s\";", cursor_page_size, cursor_name ); + + if( NULL == result ) { + const char* msg; + int errnum = dbi_conn_error( dbhandle, &msg ); + osrfLogError(OSRF_LOG_MARK, "%s: Error retrieving %s with query [%s]: %d %s", + modulename, osrfHashGet( class_meta, "fieldmapper" ), sql, errnum, + msg ? msg : "(No description available)" ); + if( !oilsIsDBConnected( dbhandle )) + osrfAppSessionPanic( ctx->session ); + osrfAppSessionStatus( + ctx->session, + OSRF_STATUS_INTERNALSERVERERROR, + "osrfMethodException", + ctx->request, + "Severe query error -- see error log for more details" + ); + *err = -1; + free( sql ); + return NULL; - dbi_result_free( result ); // toss the declare result - result = dbi_conn_queryf( dbhandle, "FETCH %d FROM \"%s\";", cursor_page_size, cursor_name ); - - if( NULL == result ) { - const char* msg; - int errnum = dbi_conn_error( dbhandle, &msg ); - osrfLogError(OSRF_LOG_MARK, "%s: Error retrieving %s with query [%s]: %d %s", - modulename, osrfHashGet( class_meta, "fieldmapper" ), sql, errnum, - msg ? msg : "(No description available)" ); - if( !oilsIsDBConnected( dbhandle )) - osrfAppSessionPanic( ctx->session ); - osrfAppSessionStatus( - ctx->session, - OSRF_STATUS_INTERNALSERVERERROR, - "osrfMethodException", - ctx->request, - "Severe query error -- see error log for more details" - ); - *err = -1; - free( sql ); - return NULL; - - } + } + } if( dbi_result_first_row( result )) { @@ -6094,9 +6120,9 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ // eliminate the duplicates. osrfLogDebug( OSRF_LOG_MARK, "Query returned at least one row" ); osrfHash* dedup = osrfNewHash(); - int cursor_page_remaining = cursor_page_size; + int cursor_page_remaining = cursor_page_size; do { - cursor_page_remaining--; + cursor_page_remaining--; row_obj = oilsMakeFieldmapperFromResult( result, class_meta ); char* pkey_val = oilsFMGetString( row_obj, pkey ); if( osrfHashGet( dedup, pkey_val ) ) { @@ -6105,16 +6131,21 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ } else { if( !enforce_pcrud || !need_to_verify || verifyObjectPCRUD( ctx, class_meta, row_obj, 0 /* means check user data for rs_size */ )) { - if (offset > -1 && offset) { - offset--; - } else { - if (limit > -1 && limit) limit--; - osrfHashSet( dedup, pkey_val, pkey_val ); - jsonObjectPush( res_list, row_obj ); - } + if (cursor_name) { + if (offset > -1 && offset) { + offset--; + } else { + if (limit > -1 && limit) limit--; + osrfHashSet( dedup, pkey_val, pkey_val ); + jsonObjectPush( res_list, row_obj ); + } + } else { + osrfHashSet( dedup, pkey_val, pkey_val ); + jsonObjectPush( res_list, row_obj ); + } } } - if (!cursor_page_remaining) { // fetch next page + if (cursor_name && !cursor_page_remaining) { // fetch next page cursor_page_remaining = cursor_page_size; dbi_result_free( result ); result = dbi_conn_queryf( dbhandle, "FETCH %d FROM \"%s\";", cursor_page_size, cursor_name ); @@ -6140,9 +6171,11 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ } } } while( - limit && ( // limit is -1 (none) or not yet decremented to 0 - (cursor_page_remaining == cursor_page_size && dbi_result_first_row(result)) || // on a new page, and there's a row - (cursor_page_remaining != cursor_page_size && dbi_result_next_row(result)) // on the same page, and there's a row + (!cursor_name && dbi_result_next_row(result)) || ( + cursor_name && limit && ( // limit is -1 (none) or not yet decremented to 0 + (cursor_page_remaining == cursor_page_size && dbi_result_first_row(result)) || // on a new page, and there's a row + (cursor_page_remaining != cursor_page_size && dbi_result_next_row(result)) // on the same page, and there's a row + ) ) ); osrfHashFree( dedup ); @@ -6154,8 +6187,11 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ /* clean up the query */ dbi_result_free( result ); - result = dbi_conn_queryf( dbhandle, "CLOSE \"%s\";", cursor_name ); - dbi_result_free( result ); + if (cursor_name) { + result = dbi_conn_queryf( dbhandle, "CLOSE \"%s\";", cursor_name ); + dbi_result_free( result ); + } + free( sql ); // If we're asked to flesh, and there's anything to flesh, then flesh it -- 2.11.0