Skip to content

Commit

Permalink
[AD-735] large dataset crash issue (#74)
Browse files Browse the repository at this point in the history
### Summary

ODBC driver will not run on large document tests

### Description
The performance test crashes for large document tests. 

1. The root cause is the wrong memory is passed to SQLBindCol(). The parameter TargetValuePtr is passed &cols[i][0].data_dat[i]. The i could exceed 255 for large table while its max valid value should be 254. The correct value should be 0.
2. The SQLExtendedFetch is deprecated. Replace it with SQLFetch.

### Related Issue

https://bitquill.atlassian.net/browse/AD-735

* [AD-509] Update mac developer environment setup

* Adjust formatting

* Add note about llvm

* Add note about iodbc

* Update README.md

change the install options for OpenSSL, since vcpkg is a preferred option

* Update README.md

remove the WiX Toolset v4 requirement

* Update README.md

add steps for the fix for ignite-odbc-tests.profraw error in README.

* [AD-517] Remove old properties + add new properties

* remove SSL settings and add TLS checkbox on the Configuration Window

* add comments for my own work

added some comments for my own convenience. Will remove later

* fix tls checkbox on Configuration Window

fixed by updating ChildId in tlsCheckBox = CreateCheckBox

* add TlsAllowInvalidHostnames checkbox and TLS CA edit to the Configuration Window

* Update configuration.cpp

add comment to illustrate plan on what to do for the configuration window

* Update dsn_configuration_window.cpp

bugfix to make tlsCheckBox and tlsAllowInvalidHostnamesCheckBox read into the saved values

* change defaultFetchSize to fetchSize in configuration.h

* change Default DSN name to DocumentDB DSN

* add App Name, Fetch Size, Read Preference, and login timeout to configuration window

* [AD-522] bugfix - change DefaultFetchSize to fetchSize in all related files to be consistent with configuration.h

Files that had the change connection_string_parser.cpp, connection_string_parser.h, dsn_config.cpp, batch_query.cpp, data_query.cpp.

* [AD-522] save read preference settings

* [AD-522] remove commented out code

* [AD-522] Implement replicaSet and retryReads on Configuration Window

* [AD-517] Re-enable unit tests - WIP -  still need to cleanup

* [AD-522] add space to code lines for format consistency in configuration_window.h

* Add more unit tests

* Remove extra file

* [AD-522] Add small letter case to keep format consistency in dsn_configuration_window.h

 Add small letter case to keep format consistency in dsn_configuration_window.h
fixed one typo (Create authentication settings group box)

* Remove redundant declaration

* [AD-522] add variable definitions for SSH tunnel settings for config UI

* [AD-522] add function definition for SSH Tunnel

added SSH User, SSH Host. SSH Strict Host Key Checking, and SSH Known Hosts File onto the UI. However, the code to save the written values would be done in later commits.

* [AD-522] code draft for configuration.cpp

add draft variable definition to Enable ssh tunnel boolean value

* Revert "change defaultFetchSize to fetchSize in configuration.h"

This reverts commit 8a04166b05161326e68c30d98ca17cdbec0df1dc.

* Revert "[AD-522] bugfix - change DefaultFetchSize to fetchSize in all related files to be consistent with configuration.h"

This reverts commit f89a7e9516c20924fbc6c3e6d3763f26309c5723.

* [AD-517] * Change the C++ version to use to compile.

* [AD-522] change fetchSize to defaultFetchSize on configuration window for consistency with JDBC driver

* [AD-517] * Changed checks.yml so that it doesn't fail on code review errors.

* Revert "[AD-522] bugfix - change DefaultFetchSize to fetchSize in all related files to be consistent with configuration.h""

This reverts commit 05eae8ade2c68607bd8a16bee8f60b5681ad74c6.

* [AD-522] make TLS allow invalid hostnames checkbox enabled only when tls Check box is checked

* Revert "Revert "[AD-522] bugfix - change DefaultFetchSize to fetchSize in all related files to be consistent with configuration.h"""

This reverts commit 301e04524c1bfcc89db76476171714c69ca1f582.

* Improvements from review

* Updated comments

* [AD-522] save SSH values for re-read, make settings window into 2 columns

Make ssl and additional settings groups to be on the right side of the column on the configuration window. This is subject to change later.
I created the function
void DsnConfigurationWindow::RetrieveSshParameters(config::Configuration& cfg) const
to save SSH values.
The width value in the DsnConfigurationWindow constructor is doubled from 360 to 720 and added the margin value. The margins currently look imbalanced on the settings box, and will be fixed in later commits.
Make Ok button and Cancel buttons aligned with the right column
bugfix - allow the SSH strict host checking checkbox to be checked on and off by adding the case ChildId::SSH_STRICT_HOST_KEY_CHECKING_CHECK_BOX definition.

* Removed includes no longer being used in configuration.h

* Some formatting improvements

* [Ad-522] add sshEnable variable in DSN config

- added sshEnable variable definition and getter/setter for sshEnable.
- added code for parsing sshEnable in the connection string parser

* [AD-522] change from GetSshEnable to IsSshEnable for consistency

* [AD-522] comment out deprecated readPreference variable

start on transitioning from readPreferenceEdit to readPreference combo box

* [AD-522] add SSH enable checkbox, SSH private key passphrase, and SSH private key file

* add SSH Enable variable in configuration.h and configuration.cpp
* add SSH checkbox in SSH group setting
* add SSH private key file edit in SSH group setting
* add SSH private key pass phrase in SSH group setting
** SSH private key passphrase label requires double the row height due to the label being long.
* make SSH setting items disabled/enabled as SSH enable checkbox is unchecked/checked
* save SSH variables to configuration when Ok button is pressed.

* [AD-522] read_preference.h fix

* add guards to read_preference.h fix
* include read_preference.h in dsn_configuration_window.cpp
* add draft code for read preference in dsn_configuration_window.cpp

* [AD-522] update fields in configuration window

* modified order of fields in the configuration window.
- Retry reads checkbox is now in the first row under "Additional Settings".
- SSH Strict host key check is now in the last row under "Internal SSH Tunnel Settings"
- Allow invalid hostnames checkbox now is in 2nd row under "TLS/SSL settings"
* added warning sentences for Allow invalid hostnames checkbox and SSH strict host key check

* Improvements from code review + address some warnings

* Change connection _test to use hostname key instead of address

* Change connection_test to use databse key instead of schema key

* [AD-522] implement read Preference

* add SpaceToUnderscore function
* implement read Preference in Config window and save the value

* Attempt to fix windows 32 build

* [AD-522] refactor dsn_configuration_window.cpp

refactor code format; remove spaces and  unnecessary comments

* [AD-522] fix config window margins

now the config window is symmetric

* Fix formatting

* [AD-522] add HasText() function to window.h

* previously, the definition of HasText was mistakenly committed in commit 74dd190fac5ef642d0d83015766fe33dfefb3489.
* add header of HasText() to resolve building errors in origin
* update HasText() function to make code simpler

* [AD-522] make "rs0" the default value of replicaSet

* [AD-522] implement database, hostname, port fields. And disable ok button unless all required fields are filled

* implement database, hostname, port fields.
* disable ok button unless all required fields (database, hostname, port, username, password) are filled
* added boolean created to indicate whether the configuration window has been created. If has, then check for disabling/enabling the Ok button

* [AD-522] rename SSH Known Hosts File label for clarity

* [AD-522] refactor logs for retrieving TSL settings

Make the sentences align

* [AD-522] rename TLS CA File label for clarity

* change label length to the same label length as connection settings group

* [AD-522] move authorization setting fields to connection fields

reason: only username and password are authorization settings, the rest are not. We chose to put username, password in the connections group because all fields in the connections group are required.

* [AD-522] bugfix-comment out address field related code in RetrieveConnectionParameters

* [AD-522] rename sslSettings to tlsSettings and add comment for connection settings group

* [AD-522] remove protocol version from config window

* [AD-522] bug fix sshKnownHostsFile not saved when Ok button is pressed

* [AD-522] remove commented out #include headers

* [AD-522] disable  SSH Known Hosts File edit when SSH Strict Host Key Check check box is unchecked

* move SSH Strict Host Key Checking check box above SSH Known Hosts File edit in the config window

* [AD-522] refactor - remove commented out header code

* [AD-522] add comments in configuration.cpp for planning

* modify commented out example code in dsn_configuration_window.cpp for clarity

* [AD-522] change from SSL to TLS for consistency

* SSL_SETTINGS_GROUP_BOX is changed to TLS_SETTINGS_GROUP_BOX

* [AD-522] remove commented out code

Removed commented out code:
* RetrieveAuthParameters
* CreateAuthSettingsGroup
* CreateAdditionalSettingsGroup function and CreateSslSettingsGroup function for 1 column window
* protocol version - related code
* authSettingsGroupBox code

* [AD-522] remove std::auto_ptr<Window> authSettingsGroupBox;

* [AD-522] remove address field auto pointer initialization

* [AD-522] remove address field from config window constructor

* [AD-522] refactor - remove unneeded commented out code in HasText

* [AD-522] refactor - remove white spaces and update comment

* [AD-522] debug scan_method.h

* SpaceToUnderscore is called in scan_method.cpp to make reads better
* added guards to scan_method.h

* [AD-522] implement Schema group settings box

* fixed bug in dsn_config.cpp to save scanLimit correctly

* [AD-522] update readPreference default value to unknown

* removed unneeded commented out code
* removed unnecessary code to get string value on readPreference

* [AD-522] refactor - remove unnecessary comments

* [AD-522] adjust window size and additional settings group label size

* additional settings group label size is now the same as other settings group in the same column

* Revert "Merge branch 'develop' into alinaliBQ/AD-522/config_window"

This reverts commit 696db387de8f509d846ad6351d5cdfcc9937a514, reversing
changes made to 8d81a183bea353fef47a62e542bf3fd6f53c13ab.

* [AD-522] keyboard user interface design + disable DSN empty strimg warning dialog

* make cursor go to checkboxes on the Config window and not skip them when tab key is pressed
* when DSN field is empty, Ok Button is disabled
* make default replicaSet value empty string (default is disable repliaSet)
* refactor - removed unneeded comments

* [AD-522] add end_point header to resolve build errors

end_point related code still exist in connection.cpp, so I readded the header for end_point

* Revert "Revert "Merge branch 'develop' into alinaliBQ/AD-522/config_window""

This reverts commit 788cad79f72a7f99ff66c5032bb1532eb71b0da5.

* [AD-522] resolve errors after merging develop branch

* GetAddresses() is deprecated, replaced it with GetHostname() and GetTcpPort() to get hostname and port number, respectively.
* replaced GetSchema() with GetSchemaName()
* removed commented out code
* refactor with dsn_configuration_window.cpp

* [AD-512] Resolve issue with missing include file.

* [AD-522] make BoolParseResult recognizable

* [AD-522] fix build errors after develop branch merge

* update schema checkbox string
* make edit row height single
* update SetTcpPort to SetPort etc
* define default value for sshEnable

* [AD-522] enable encrypt password for ssh private key passphrase

* modify refresh schema checkbox string to fit better
* remove log message for ssh private key passphrase to increase security

* [AD-521] add comments and notes for starting AD-521

* [AD-509] update readMe to include directions about saving Java bin and server directories.

* reason: ODBC JNI calls have a dependency on `jvm.dll`, as a result, the ODBC driver cannot operate properly unless the Java bin and server directories are included in the path.

* [AD-522] reduce space between ssh private key passphrase and the checkbox below

* [AD-521] define constants for Java GetSshLocalPort method call

* [AD-521] integrate code review feedback from Andie

* refactors with comment changes and log changes

* [AD-521] refactor code

* make open brace bracket start on new line for JVMException
* remove excess space

* [AD-427] Tracer Code - Limited Capability to load metadata - work-in-progress.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Windows build.

* [AD-427] Fix Windows build.

* [AD-427] Fix Windows build.

* [AD-522] remove space and add new line at the end of enum types

* [AD-522] add new lines at the end of Enum type files

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-521] add changes for Java.cpp from Bruce's AD-427 branch

* those changes are not included in the previous merge, and are now added

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-427] Fix Mac build.

* [AD-521] JNI wrapper calls to iSshTunnelActive and getSshLocalPort

* some draft comments are left to be cleaned up

* [AD-521] debug getSshLocalPort

* change function type signature from optional to int
* refactor - remove comments

* [AD-521] change return value in getSshLocalPort from boolean to a 0

* try to fix access memory issue in TestDriverManagerGetConnection

* [AD-522] resolve build errors after merge

* fix bug for schema name not correctly loaded on the config window
* removed unnecessary headers

* [AD-427] Added ResultSet.next and ResultSet.GetString()

* [AD-522] address code review comments

* change to enum class type for read_preference.h and scan_method.h
* change the way enum class objects are read
* add static cast to enum types so SetSelection function can work with it
* refactor - removed unnecessary comments
* refactor - changes code doc in connection_string_parser.h

* [AD-521] implement tests for JNI wrapper code

* test for TestDocumentDbConnectionGetSshTunnelPortSshTunnelNotActive is not yet active, need to be enabled when we can get external SSH tunnel working.
* bug fix: re-implemented DocumentDbConnectionGetSshLocalPort and DocumentDbConnectionIsSshTunnelActive.
connection.Get()->GetRef() is passed as first parameter because jobject is required for CallIntMethod and CallBooleanMethod.
* better JNI wrapper code is needed in the future so warnings/errors pop up when we pass jobject instead of jclass to the methods.

* [AD-521] refactor - remove comments and commented out code

* [AD-589] implement JNI method call for GetDatabaseMetadata

* define the method constant for DocumentDbConnection.getDatabaseMetadata()
* create JNI wrapper function for DocumentDbConnection.getMetaData

* [AD-589] implement unit test for DocumentDbConnection.getMetaData

* [AD-521] add calls to AutoCloseConnection in unit tests

* pass & errInfo instead of * errInfo in DocumentDbConnectionGetSshLocalPort and DocumentDbConnectionIsSshTunnelActive.

* [AD-521] remove unnecessary comment

* [AD-589] fix merge issues

* I missed this part of code when I was resolving the merge conflicts. Now they are corrected

* [AD-589] update getMetaData JNI wrapper code to pass &errInfo

* implement unit test for getMetaData

* [AD-589] implement JNI wrapper for DocumentDbDatabaseSchemaMetadataGetSchemaName

* implement Java class for
* define constant for DocumentDbDatabaseSchemaMetadataGetSchemaName
*  add empty test function for TestDocumentDbDatabaseSchemaMetadataGetSchemaName

* [AD-589] implement unit test for DbDatabaseSchemaMetadataGetSchemaName

* [AD-521] refactor - change case of ssh to SSH for consistency

* [AD-589] address code review comments

* rename DocumentDbConnectionGetMetaData to DocumentDbConnectionGetDatabaseMetaData

* [AD-589] change case from MetaData to Metadata to match Java method name

* [AD-589] refactor: change from metaData to metadata

changes occurred in:
* DocumentDbConnectionGetDatabaseMetadata
* DocumentDbDatabaseSchemaMetadataGetSchemaName
* TestDocumentDbConnectionGetDatabaseMetadata
* TestDocumentDbDatabaseSchemaMetadataGetSchemaName

* Birschick bq/sync up (#21)

* Updating JDBC version (#72)

* [AD-370] upload odbc driver build on push to develop (#71)

* [AD-730] upload odbc driver build at push to develop

* [AD-730] update github actions to upload with v3

* [AD-730] Test artifact upload action

* temporarily comment out if statement guard to test upload-artifact github action

* [AD-730] upload performance test executable

* [AD-730] update readme

* [AD-730] upload Release folder as artifact

* our odbc needs more than just the ignite.odbc.dll to run properly

* [AD-730] update to upload artifacts for release build

* previously, I uploaded the Debug builds, but the Release build is the one that requires less dependencies.

* [AD-730] add back if statment

* add the if statement back in win-build.yml to ensure that the odbc driver performance artifact is only published when pushed to develop

* [AD-730] fix typo of performance test plan

Co-authored-by: affonsoBQ <[email protected]>
Co-authored-by: Alina (Xi) Li <[email protected]>

* [AD-735] large dataset crash issue (#23)

* first test

* fix performance test crash

* fix performance test crash

* fix whitespace issue

* rework based on comments

Co-authored-by: Andie Montoya <[email protected]>
Co-authored-by: Alina (Xi) Li <[email protected]>
Co-authored-by: Alina (Xi) Li <[email protected]>
Co-authored-by: affonsoBQ <[email protected]>
Co-authored-by: RoyZhang2022 <[email protected]>
  • Loading branch information
6 people authored May 18, 2022
1 parent 116d44d commit 390bdb6
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 19 deletions.
2 changes: 1 addition & 1 deletion src/tests/performance/include/performance_test_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ typedef SQLLEN SQLROWOFFSET;
* GLOBAL CONSTANTS
*******************************************************/

#define BIND_SIZE 255 // used for SQLBindCol function
#define BIND_SIZE 512 // used for SQLBindCol function

// CSV header definitions (input csv file must match)
#define QUERY_HEADER "query"
Expand Down
26 changes: 8 additions & 18 deletions src/tests/performance/src/performance_test_runner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -406,13 +406,9 @@ void performance::PerformanceTestRunner::RecordExecBindFetch(
SQLRETURN ret;
SQLSMALLINT total_columns = 0;
SQLROWSETSIZE row_count;
SQLROWSETSIZE rows_fetched;
SQLUSMALLINT* row_status;
long long time_exec_ms;
long long time_bind_fetch_ms;

// Dynamic memory allocated
row_status = new SQLUSMALLINT[test_case.limit];
std::vector< Col > cols;

// Query
std::string temp_str =
Expand All @@ -429,7 +425,6 @@ void performance::PerformanceTestRunner::RecordExecBindFetch(
auto time_exec_end = std::chrono::steady_clock::now();

if (ret != SQL_SUCCESS) {
delete[] row_status;
LogAnyDiagnostics(SQL_HANDLE_STMT, *hstmt, ret);
test_case.status = error;
test_case.err_msg = "SQLExecDirect failed";
Expand All @@ -446,39 +441,35 @@ void performance::PerformanceTestRunner::RecordExecBindFetch(
ret = SQLNumResultCols(*hstmt, &total_columns);

if (ret != SQL_SUCCESS) {
delete[] row_status;
LogAnyDiagnostics(SQL_HANDLE_STMT, *hstmt, ret);
test_case.status = error;
test_case.err_msg = "SQLNumResultCols failed";
return; // continue to next test case
}

std::vector< std::vector< Col > > cols(total_columns);
for (size_t i = 0; i < cols.size(); i++) {
cols[i].resize(test_case.limit);
if (static_cast< size_t >(total_columns) > cols.size()) {
cols.resize(static_cast< size_t >(total_columns));
}

// Bind and fetch and record time
auto time_bind_fetch_start = std::chrono::steady_clock::now();
for (size_t i = 0; i < cols.size(); i++) {
for (size_t i = 0; i < static_cast< size_t >(total_columns); i++) {
ret = SQLBindCol(*hstmt, static_cast< SQLUSMALLINT >(i + 1),
SQL_C_CHAR,
static_cast< SQLPOINTER >(&cols[i][0].data_dat[i]),
BIND_SIZE, &cols[i][0].data_len);
static_cast< SQLPOINTER >(&cols[i].data_dat[0]),
BIND_SIZE, &cols[i].data_len);
if (ret != SQL_SUCCESS) {
delete[] row_status;
LogAnyDiagnostics(SQL_HANDLE_STMT, *hstmt, ret);
test_case.status = error;
test_case.err_msg = "SQLBindCol failed";
return; // continue to next test case
}
}

while (SQLExtendedFetch(*hstmt, SQL_FETCH_NEXT, 0, &rows_fetched,
row_status)
== SQL_SUCCESS) {
while (SQLFetch(*hstmt) == SQL_SUCCESS) {
row_count++;
}

auto time_bind_fetch_end = std::chrono::steady_clock::now();

// Store bind and fetch time
Expand All @@ -490,7 +481,6 @@ void performance::PerformanceTestRunner::RecordExecBindFetch(

test_case.time_ms.push_back(time_exec_ms + time_bind_fetch_ms);
}
delete[] row_status;
test_case.status = success;
}

Expand Down

0 comments on commit 390bdb6

Please sign in to comment.