From 62738bacf3c0cf51d4c6c7adee8857b4b355ef17 Mon Sep 17 00:00:00 2001 From: David Puglielli Date: Fri, 12 Apr 2019 20:49:03 -0700 Subject: [PATCH] Addressed review comments --- source/pdo_sqlsrv/pdo_stmt.cpp | 36 +++++++++++-------- source/shared/core_sqlsrv.h | 6 +++- source/shared/core_stmt.cpp | 30 ++++++++-------- source/sqlsrv/stmt.cpp | 2 +- source/sqlsrv/util.cpp | 2 +- .../sqlsrv/sqlsrv_empty_result_error.phpt | 2 +- .../sqlsrv/test_warning_errors3.phpt | 4 +-- 7 files changed, 48 insertions(+), 34 deletions(-) diff --git a/source/pdo_sqlsrv/pdo_stmt.cpp b/source/pdo_sqlsrv/pdo_stmt.cpp index 575fc8c6..f8dd160c 100644 --- a/source/pdo_sqlsrv/pdo_stmt.cpp +++ b/source/pdo_sqlsrv/pdo_stmt.cpp @@ -595,23 +595,22 @@ int pdo_sqlsrv_stmt_execute( _Inout_ pdo_stmt_t *stmt TSRMLS_DC ) stmt->row_count = 0; driver_stmt->column_count = 0; driver_stmt->row_count = 0; - driver_stmt->columns_rows_obtained = true; } else { - if (driver_stmt->columns_rows_obtained == false) - { + if (driver_stmt->column_count == ACTIVE_NUM_COLS_INVALID) { stmt->column_count = core::SQLNumResultCols( driver_stmt TSRMLS_CC ); + driver_stmt->column_count = stmt->column_count; + } + else { + stmt->column_count = driver_stmt->column_count; + } + if (driver_stmt->row_count == ACTIVE_NUM_ROWS_INVALID) { // return the row count regardless if there are any rows or not stmt->row_count = core::SQLRowCount( driver_stmt TSRMLS_CC ); - - driver_stmt->column_count = stmt->column_count; driver_stmt->row_count = stmt->row_count; - driver_stmt->columns_rows_obtained = true; } - else - { - stmt->column_count = driver_stmt->column_count; + else { stmt->row_count = driver_stmt->row_count; } } @@ -707,11 +706,21 @@ int pdo_sqlsrv_stmt_fetch( _Inout_ pdo_stmt_t *stmt, _In_ enum pdo_fetch_orienta SQLSMALLINT odbc_fetch_ori = pdo_fetch_ori_to_odbc_fetch_ori( ori ); bool data = core_sqlsrv_fetch( driver_stmt, odbc_fetch_ori, offset TSRMLS_CC ); - // support for the PDO rowCount method. Since rowCount doesn't call a method, PDO relies on us to fill the - // pdo_stmt_t::row_count member - if( driver_stmt->past_fetch_end) { + // support for the PDO rowCount method. Since rowCount doesn't call a + // method, PDO relies on us to fill the pdo_stmt_t::row_count member + // The if condition was changed from + // `driver_stmt->past_fetch_end || driver_stmt->cursor_type != SQL_CURSOR_FORWARD_ONLY` + // because it caused SQLRowCount to be called at each fetch if using a non-forward cursor + // which is unnecessary and a performance hit + if( driver_stmt->past_fetch_end || driver_stmt->cursor_type == SQL_CURSOR_DYNAMIC) { - stmt->row_count = core::SQLRowCount( driver_stmt TSRMLS_CC ); + if (driver_stmt->row_count == ACTIVE_NUM_ROWS_INVALID) { + stmt->row_count = core::SQLRowCount( driver_stmt TSRMLS_CC ); + driver_stmt->row_count = stmt->row_count; + } + else { + stmt->row_count = driver_stmt->row_count; + } // a row_count of -1 means no rows, but we change it to 0 if( stmt->row_count == -1 ) { @@ -1164,7 +1173,6 @@ int pdo_sqlsrv_stmt_next_rowset( _Inout_ pdo_stmt_t *stmt TSRMLS_DC ) driver_stmt->column_count = stmt->column_count; driver_stmt->row_count = stmt->row_count; - driver_stmt->columns_rows_obtained = true; } catch( core::CoreException& ) { diff --git a/source/shared/core_sqlsrv.h b/source/shared/core_sqlsrv.h index 1c6cacad..347a89d7 100644 --- a/source/shared/core_sqlsrv.h +++ b/source/shared/core_sqlsrv.h @@ -182,6 +182,11 @@ const int SQL_MAX_ERROR_MESSAGE_LENGTH = SQL_MAX_MESSAGE_LENGTH * 2; // max size of a date time string when converting from a DateTime object to a string const int MAX_DATETIME_STRING_LEN = 256; +// identifier for whether or not we have obtained the number of rows and columns +// of a result +const short ACTIVE_NUM_COLS_INVALID = -99; +const long ACTIVE_NUM_ROWS_INVALID = -99; + // precision and scale for the date time types between servers const int SQL_SERVER_2005_DEFAULT_DATETIME_PRECISION = 23; const int SQL_SERVER_2005_DEFAULT_DATETIME_SCALE = 3; @@ -1439,7 +1444,6 @@ struct sqlsrv_stmt : public sqlsrv_context { bool fetch_called; // Used by core_sqlsrv_get_field to return an informative error if fetch not yet called int last_field_index; // last field retrieved by core_sqlsrv_get_field bool past_next_result_end; // core_sqlsrv_next_result sets this to true when the statement goes beyond the last results - bool columns_rows_obtained; // Whether or not SQLNumResultCols and SQLRowCount have been called for the active result short column_count; // Number of columns in the current result set obtained from SQLNumResultCols long row_count; // Number of rows in the current result set obtained from SQLRowCount unsigned long query_timeout; // maximum allowed statement execution time diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index 352d2ff0..9d7f6801 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -140,9 +140,8 @@ sqlsrv_stmt::sqlsrv_stmt( _In_ sqlsrv_conn* c, _In_ SQLHANDLE handle, _In_ error fetch_called( false ), last_field_index( -1 ), past_next_result_end( false ), - columns_rows_obtained( false ), - column_count( 0 ), - row_count( 0 ), + column_count( ACTIVE_NUM_COLS_INVALID ), + row_count( ACTIVE_NUM_ROWS_INVALID ), query_timeout( QUERY_TIMEOUT_INVALID ), date_as_string(false), format_decimals(false), // no formatting needed @@ -228,9 +227,8 @@ void sqlsrv_stmt::new_result_set( TSRMLS_D ) this->past_next_result_end = false; this->past_fetch_end = false; this->last_field_index = -1; - this->columns_rows_obtained = false; - this->column_count = 0; - this->row_count = 0; + this->column_count = ACTIVE_NUM_COLS_INVALID; + this->row_count = ACTIVE_NUM_ROWS_INVALID; // delete any current results if( current_results ) { @@ -828,7 +826,7 @@ bool core_sqlsrv_fetch( _Inout_ sqlsrv_stmt* stmt, _In_ SQLSMALLINT fetch_orient // First time only if ( !stmt->fetch_called ) { SQLSMALLINT has_fields; - if (stmt->columns_rows_obtained) { + if (stmt->column_count != ACTIVE_NUM_COLS_INVALID) { has_fields = stmt->column_count; } else { has_fields = core::SQLNumResultCols( stmt TSRMLS_CC ); @@ -1078,21 +1076,25 @@ void core_sqlsrv_get_field( _Inout_ sqlsrv_stmt* stmt, _In_ SQLUSMALLINT field_i bool core_sqlsrv_has_any_result( _Inout_ sqlsrv_stmt* stmt TSRMLS_DC ) { - SQLSMALLINT num_cols, rows_affected; - if (stmt->columns_rows_obtained) - { + SQLSMALLINT num_cols; + SQLLEN rows_affected; + + if (stmt->column_count != ACTIVE_NUM_COLS_INVALID) { num_cols = stmt->column_count; - rows_affected = stmt->row_count; } - else - { + else { // Use SQLNumResultCols to determine if we have rows or not num_cols = core::SQLNumResultCols( stmt TSRMLS_CC ); stmt->column_count = num_cols; + } + + if (stmt->row_count != ACTIVE_NUM_ROWS_INVALID) { + rows_affected = stmt->row_count; + } + else { // Use SQLRowCount to determine if there is a rows status waiting rows_affected = core::SQLRowCount( stmt TSRMLS_CC ); stmt->row_count = rows_affected; - stmt->columns_rows_obtained = true; } return (num_cols != 0) || (rows_affected > 0); diff --git a/source/sqlsrv/stmt.cpp b/source/sqlsrv/stmt.cpp index aa97231e..0765b7d4 100644 --- a/source/sqlsrv/stmt.cpp +++ b/source/sqlsrv/stmt.cpp @@ -1791,7 +1791,7 @@ SQLSMALLINT get_resultset_meta_data(_Inout_ sqlsrv_stmt * stmt) if (num_cols == 0) { getMetaData = true; - if (stmt->columns_rows_obtained == false) { + if (stmt->column_count == ACTIVE_NUM_COLS_INVALID) { num_cols = core::SQLNumResultCols(stmt TSRMLS_CC); } else { num_cols = stmt->column_count; diff --git a/source/sqlsrv/util.cpp b/source/sqlsrv/util.cpp index dcddcdcd..e4ea3472 100644 --- a/source/sqlsrv/util.cpp +++ b/source/sqlsrv/util.cpp @@ -196,7 +196,7 @@ ss_error SS_ERRORS[] = { { SQLSRV_ERROR_NO_FIELDS, - { IMSSP, (SQLCHAR*)"The active result for the query contains no fields, so no result set was created.", -28, false } + { IMSSP, (SQLCHAR*)"The active result for the query contains no fields.", -28, false } }, { diff --git a/test/functional/sqlsrv/sqlsrv_empty_result_error.phpt b/test/functional/sqlsrv/sqlsrv_empty_result_error.phpt index bfea07d9..92b7f0ae 100644 --- a/test/functional/sqlsrv/sqlsrv_empty_result_error.phpt +++ b/test/functional/sqlsrv/sqlsrv_empty_result_error.phpt @@ -12,7 +12,7 @@ require_once("MsCommon.inc"); // These are the error messages we expect at various points below $errorNoMoreResults = "There are no more results returned by the query."; $errorNoMoreRows = "There are no more rows in the active result set. Since this result set is not scrollable, no more data may be retrieved."; -$errorNoFields = "The active result for the query contains no fields, so no result set was created."; +$errorNoFields = "The active result for the query contains no fields."; // Variable function gets an error message that depends on the OS function getFuncSeqError() diff --git a/test/functional/sqlsrv/test_warning_errors3.phpt b/test/functional/sqlsrv/test_warning_errors3.phpt index f0e7848c..1b0bdc66 100644 --- a/test/functional/sqlsrv/test_warning_errors3.phpt +++ b/test/functional/sqlsrv/test_warning_errors3.phpt @@ -128,8 +128,8 @@ Array [SQLSTATE] => IMSSP [1] => -28 [code] => -28 - [2] => The active result for the query contains no fields, so no result set was created. - [message] => The active result for the query contains no fields, so no result set was created. + [2] => The active result for the query contains no fields. + [message] => The active result for the query contains no fields. ) )