Fix to GitHub issues 574 and 580 #592
Conversation
source/shared/core_results.cpp
Outdated
| @@ -45,7 +45,7 @@ namespace { | |||
|
|
|||
| // *** internal constants *** | |||
|
|
|||
| const int INITIAL_FIELD_STRING_LEN = 256; // base allocation size when retrieving a string field | |||
| const int INITIAL_FIELD_STRING_LEN = 1024; // base allocation size when retrieving a string field | |||
There was a problem hiding this comment.
can you only increase the initial buffer size when column encryption is enabled? Also, if column encryption is enabled, I would love to blast the initial buffer size to 8000 (max size for varchar(n))
There was a problem hiding this comment.
I think it's reasonable to simply use 8000 regardless of AE setting. We use this buffer size for varchar / char parameters with unknown size.
| // fetch from $tableName | ||
| $row = $stmt->fetch(PDO::FETCH_NUM); | ||
| if ($row) { | ||
| echo(substr($row[0], 0, 15)) . PHP_EOL; |
There was a problem hiding this comment.
this only echoes the first 10 characters, what if something has gone wrong in between? Would be great of you can compare it with $phrase instead
| createTable($conn, $tableName1, $columns); | ||
|
|
||
| // insert one row to each table | ||
| $phrase = str_repeat('This is a test ', 250); |
There was a problem hiding this comment.
if you're going to change the initial buffer size, this number will have to change as well
| @@ -0,0 +1,186 @@ | |||
| --TEST-- | |||
| GitHub issue 574 - Fetch Next Result Test | |||
There was a problem hiding this comment.
please change this test as well according to the comments in the PDO_SQLSRV equivalent test
Codecov Report
@@ Coverage Diff @@
## dev #592 +/- ##
=========================================
- Coverage 74.96% 74.47% -0.5%
=========================================
Files 50 50
Lines 14933 14949 +16
=========================================
- Hits 11195 11133 -62
- Misses 3738 3816 +78
Continue to review full report at Codecov.
|
source/shared/core_results.cpp
Outdated
| @@ -45,7 +45,7 @@ namespace { | |||
|
|
|||
| // *** internal constants *** | |||
|
|
|||
| const int INITIAL_FIELD_STRING_LEN = 1024; // base allocation size when retrieving a string field | |||
| const int INITIAL_FIELD_STRING_LEN = 8000; // base allocation size when retrieving a string field | |||
There was a problem hiding this comment.
Now that I think about it, changing this value for non-AE may cause a breaking change. Say a user is trying to fetch a string that is 5000 bytes long. Before this change, the user needs to call sqlsrv_next_result + sqlsrv_get_field many times to retrieve the complete string. With this change however, the user only needs to call sqlsrv_get_field once the retrieve the entire string. This seems like a good thing. But if the user has multiple sqlsrv_next_result + sqlsrv_get_field in their code, error will be thrown telling the user there's no more result left. Therefore, I think it's best to only change the buffer size for if column encryption is enabled. It's impossible to change the constant here though, you may have to go through the code, check where INITIAL_FIELD_STRING_LEN is used and change it there.
This change is