JTDS Driver support #2320
JTDS Driver support #2320
Conversation
Pull Request Test Coverage Report for Build 9709042874Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
staticlibs
force-pushed
the
jtds_issue_2137
branch
2 times, most recently
from
37d2152 to
29ead71
Compare
|
Thanks for your comments and code pointers! In patch update I've added support for running JDBC tests with JTDS. When JTDS-specific tests The intention here is to have a minimal working test-suite (should it be enabled as an additional GitHub action run?) with a minimal working JTDS patch, and then to iterate on the JTDS specific problems gradually extending the test suite. Please let me know whether such approach (basically to merge PR with the current backend patch and follow up in subsequent PRs) is acceptable, or whether JTDS support needs to be substantially improved (with the details from your previous comments) before merging the PR? In the latter case I can iterate on test suite in PR branch. I also can improve/adjust test harness changes as needed (tried to keep its changes to minimum). |
I agree with your suggested iterative approach. But there are still some areas where we dont have clarity yet, such as, what all is not working for JTDS?, We can file separate issues for them but who will be owner to drive that? How are we going to track all such details? And on side note, we should have separate GH action to test with JTDS driver. |
staticlibs
force-pushed
the
jtds_issue_2137
branch
from
29ead71 to
c0174fc
Compare
I don't have the full picture yet. Basic DDL/DML/selects are working fine. Introspection also working (at least it is enough for DBeaver DB browser). But there are failures with inserts and selects for (n)varchar types, for lengths less then
I intend to add more existing datatype tests to JTDS runs.
I am ready to continue with this task, cannot do that full time, but will be able to spare a few hours per week for it.
I see 2 options: either commit the basic working test harness and then track each failure in a separate PR, or add more commits to this PR without merging it. I am fine with doing it either way.
I've added GH action run with JTDS, it is a copy of existing JDBC workflow with useJTDSInsteadOfMSSQLJDBC variable set. |
|
Hi @staticlibs, Thanks for adding support for the jTDS driver in JDBC Test Framework. You have already added tests for binary, varchar(max) and nvarchar(max) but to ensure we are fully able to support jTDS driver capabilities for Babelfish we will need to test the following:
We already have tests added for the above, and you can reuse the same tests to test Babelfish with jTDS driver. Similar to how you are reusing the TestBinary file. You can try adding the following files in your jTDS schedule file as a start:
Also, we need to make sure that the APIs we use in the JDBC test framework are compatible with the jTDS driver. If that is not the case, we will need to add support for the jTDSised version of the APIs in the framework. Additionally support for jTDS specific APIs (if any) should be added with tests for the same. |
|
Thanks for the details! And apologies for lack of activity on this, I was side-tracked by other Babelfish problems (prompted by usage of Cognos Analytics (#2404) and pgAgent (#2413) with Babelfish). I intend to return to this issue next week. About the JDBC APIs: I am not too concerned about jTDS problems in the test harness (going to adjust the test harness as needed), but there is a concern about the public JDBC API (besides query execution) that is used by client apps. Foremost is the DatabaseMetaData, it can run non-trivial SQL for some of the calls and currently Babelfish is not fully compatible with it on mssql-jdbc (#997), let alone jTDS. I have a separate PR in preparation to add test coverage to these metadata calls (with new |
staticlibs
force-pushed
the
jtds_issue_2137
branch
from
c0174fc to
42a89e2
Compare
|
I've added all suggested existing tests to jTDS runs. For tests that have different output on jTDS, but behave the same on both MSSQL and Babelfish, I've created copies with The following tests are found to be causing issues when run on jTDS:
Currently test runs are disabled for these tests, I will follow-up on them one by one. |
| @@ -61,6 +61,9 @@ else if(e instanceof SQLServerException) { | |||
| } | |||
| bw.write("~~ERROR (Message: "+ errorMsg + ")~~"); | |||
| } | |||
| else { // JTDS throws plain SQLServerException | |||
There was a problem hiding this comment.
Did you mean SQLException? We already handle SQLServerException in above else if condition
There was a problem hiding this comment.
Yes, this is a typo, it throws plain SQLException. Will fix this and give harness changes another pass.
There was a problem hiding this comment.
Thanks. The test framework changes look good to me. Could you let us know how you plan on following up on the tests that are failing?
There was a problem hiding this comment.
I've updated the following tests:
- TestSimpleErrors
- TestErrorsWithTryCatch
- TestTransactionSupportForProcedure
They work correctly on jTDS, but behaviour of JDBC API methods (and the test output because of this) is slightly different. I intend to proceed with Cursors, XML and Varchar tests (that use methods unsupported on jTDS) and with remaining tests after that.
staticlibs
force-pushed
the
jtds_issue_2137
branch
from
42a89e2 to
7fbdbe0
Compare
|
Added TableType tests, they work correctly with slightly different output. Investigated Cursor tests: cursors usage with jTDS is not supported on Babelfish. jTDS calls |
|
Added TestXML, it works correctly with minor differences in metadata output. |
staticlibs
force-pushed
the
jtds_issue_2137
branch
3 times, most recently
from
a543b53 to
fe7419a
Compare
staticlibs
force-pushed
the
jtds_issue_2137
branch
from
b9ef5db to
2b3a30e
Compare
|
Remaining test failures (with comments for corresponding test groups):
Fractional parts of records inserted with jTDS differ from ones inserted with mssql-jdbc.
Multiple failures on both
These tests use cursor options unsupported on Babelfish. |
|
Fixed the problems with time and datetime2 fractional seconds and fixed the length check for incoming (n)varchar(max) long strings. Remaining failures:
|
staticlibs
force-pushed
the
jtds_issue_2137
branch
from
d4ff377 to
74a594a
Compare
|
Added support for returning sql_variant date values as nvarchar, it is clear now how to fix remaining TestSQLVariant failures with other date/time types. |
Signed-off-by: Alex Kasko <[email protected]>
…inary(max) results Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
…AFT] Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
Signed-off-by: Alex Kasko <[email protected]>
staticlibs
force-pushed
the
jtds_issue_2137
branch
from
510e803 to
b4b5c0b
Compare
|
Added support for sp_cursorprepare, this fixes Remaining failures (fractional parts of records inserted with jTDS differ from ones inserted with mssql-jdbc):
|
Signed-off-by: Alex Kasko <[email protected]>
|
Fixed the remaining tests: For For With all tests from the initial list passing now on jTDS - requesting a review: @Deepesh125 , @thephantomthief . |
|
Just for the record (if someone will need this for release notes in future), SELECT @@MAX_PRECISION
SET TRANSACTION ISOLATION LEVEL READ COMMITTED
SET IMPLICIT_TRANSACTIONS OFF
SET QUOTED_IDENTIFIER ON
SET TEXTSIZE 2147483647This query is failing with |
Signed-off-by: Alex Kasko <[email protected]>
| @@ -1989,41 +1989,45 @@ TdsProcessLogin(Port *port, bool loadedSsl) | |||
| TdsErrorContext->phase = 0; | |||
| TdsErrorContext->reqType = TDS_LOGIN7; | |||
|
|
|||
| PG_TRY(); | |||
| if (!TdsReadNextBuffer() && TdsCheckMessageType(TDS_PRELOGIN)) | |||
There was a problem hiding this comment.
- Let's add a comment why we may not expecting TDS_PRELOGIN as the first request.
- The check should be
if (TdsReadNextBuffer() != EOF && TdsCheckMessageType(TDS_PRELOGIN))
- If the prelogin message is not sent, what's the SSL behavior - TDS_ENCRYPT_NOT_SUP or TDS_ENCRYPT_OFF? OFF means the login will still happen in an encrypted manner.
- According the code, the loadEncryption defaults to 0 which is TDS_ENCRYPT_OFF. So, we're freeing the ssl structure towards the end of the function without setting up the socket. Also, according to the code, the login is probably not happening in an encrypted manner.
Actually, let's not modify the SSL code at all. Something like as following (after confirming the SSL behavior):
/* comment why this is needed */
if (TdsReadNextBuffer() != EOF && TdsCheckMessageType(TDS_PRELOGIN))
{
TdsErrorContext->err_text = "Parsing PreLogin Request";
/* Pre-Login */
rc = ParsePreLoginRequest();
if (rc < 0)
return rc;
TdsErrorContext->err_text = "Make PreLogin Response";
loadEncryption = MakePreLoginResponse(port, loadedSsl);
TdsFlush();
}
else
{
TDS_DEBUG(TDS_DEBUG3, "TDS7 Prelogin Message Skipped");
loadEncryption = TDS_ENCRYPT_OFF;
}
| @@ -1785,6 +1785,7 @@ PrepareRowDescription(TupleDesc typeinfo, PlannedStmt *plannedstmt, List *target | |||
| } | |||
|
|
|||
| SetAttributesForColmetada(col); | |||
| col->atttypmod = atttypmod; | |||
There was a problem hiding this comment.
Why is this needed? The actual typmod is calculated based on the datatype. Can we rely on this value?
| * [+|-]hh:mm'). and Making use of default collation Oid. | ||
| */ | ||
| SetColMetadataForCharTypeHelper(col, TDS_TYPE_NVARCHAR, serverCollationOid, 64); | ||
| SetColMetadataForCharTypeHelper(col, TDS_TYPE_NVARCHAR, serverCollationOid, 68); |
There was a problem hiding this comment.
Have we added a testcase for this with TDS version lower than 7.3A?
| /* | ||
| * SendReturnValueIntInternal | ||
| * | ||
| * status - stored procedure (0x01) or UDF (0x02) | ||
| */ | ||
| void | ||
| SendReturnValueIntInternal(uint8 status, int32 value) | ||
| { | ||
| SendPendingDone(true); | ||
|
|
||
| /* token type */ | ||
| TDS_DEBUG(TDS_DEBUG2, "SendReturnValueIntInternal: token=0x%02x", TDS_TOKEN_RETURNVALUE); | ||
|
|
||
| /* TokenType */ | ||
| TdsPutUInt8(TDS_TOKEN_RETURNVALUE); | ||
| /* ParamOrdinal */ | ||
| TdsPutUInt16LE(0x0d); | ||
| /* ParamName */ | ||
| TdsPutUInt8(0); | ||
| /* Status */ | ||
| TdsPutUInt8(status); | ||
| /* UserType */ | ||
| if (GetClientTDSVersion() <= TDS_VERSION_7_1_1) | ||
| TdsPutUInt16LE(0); | ||
| else | ||
| TdsPutUInt32LE(0); | ||
| /* Flags */ | ||
| TdsPutUInt16LE(0); | ||
| /* TypeInfo */ | ||
| TdsPutUInt8(TDS_TYPE_INTEGER); | ||
| TdsPutUInt8(0x04); | ||
| /* Value */ | ||
| TdsPutUInt8(0x04); | ||
| TdsPutInt32LE(value); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are we not using SendReturnValueTokenInternal()? We can create an empty parameter token using MakeEmptyParameterToken() and call the same function -
returnToken = MakeEmptyParameterToken("", TDS_TYPE_INTEGER, 4, 0);
returnToken->paramOrdinal = 0;
SendReturnValueTokenInternal(returnToken, 0x01, NULL,
UInt32GetDatum(value), false, false);
| GenerateBindParamsData(req); | ||
| HandleSPCursorOpenCommon(req); | ||
| break; | ||
| case SP_CURSORPREPARE: | ||
| HandleSPCursorOpenCommon(req); | ||
| break; |
There was a problem hiding this comment.
GenerateBindParamsData() inititlizes some critical member of TDSRequest structure. Why are we not calling the same?
| if (req->spType == SP_CURSORPREPARE) | ||
| { | ||
| SendReturnValueIntInternal(1, req->cursorPreparedHandle); | ||
| TdsSendDone(TDS_TOKEN_DONEPROC, TDS_DONE_FINAL, 0xe0, 0); | ||
| } | ||
| else | ||
| SendCursorResponse(req); |
There was a problem hiding this comment.
Why not modify SendCursorResponse and handle the scenarios if it's not handling a case already?
| SendReturnValueIntInternal(1, rownum); | ||
| SendReturnValueIntInternal(1, nrows); |
There was a problem hiding this comment.
Should be
if (req->cursorExtraArg2 && (req->cursorExtraArg2->flags & 0x01) == 1)
SendReturnValueTokenInternal(req->cursorExtraArg2 0x01, NULL,
UInt32GetDatum(rownum), false, false);
if (req->cursorExtraArg3 && (req->cursorExtraArg3->flags & 0x01) == 1)
SendReturnValueTokenInternal(req->cursorExtraArg3, 0x01, NULL,
UInt32GetDatum(nrows), false, false);
| if (GetClientTDSVersion() <= TDS_VERSION_7_1_1 && | ||
| col->metaEntry.type3.tdsTypeId == TDS_TYPE_TEXT) | ||
| return TdsSendTypeText(finfo, value, vMetaData); | ||
|
|
There was a problem hiding this comment.
If we're handling it here, why do we need to handle in PrepareRowDescription as well? Also, this is not setting sendTableName to true although we're sending as text.
| @@ -1989,41 +1989,45 @@ TdsProcessLogin(Port *port, bool loadedSsl) | |||
| TdsErrorContext->phase = 0; | |||
There was a problem hiding this comment.
This is a really really large PR. We shouldn't merge this PR as a single commit as this is handling several new features together. Each feature requires its own explanation in the commit message:
Let's split the PR as per following:
- JDBC test framework changes for jTDS driver testing
- Prelogin changes
- SP_cursorprepare
- SP_cursorfetch with fetchinfo
- Datatype changes (Individual datatypes should be addressed independently if the fixes are not related)
Each PR should be backed by respective testcases instead of putting all the test case in a single PR.
| if (req->dataParameter->isNull || req->metaDataParameterValue->len == 0) | ||
| paramdefstr = NULL; | ||
| else | ||
| paramdefstr = req->metaDataParameterValue->data; |
There was a problem hiding this comment.
Just for my understanding -- what is the test case? and how would this paramdefstr look?
| if (req->spType == SP_CURSORPREPARE) | ||
| { | ||
| SendReturnValueIntInternal(1, req->cursorPreparedHandle); | ||
| TdsSendDone(TDS_TOKEN_DONEPROC, TDS_DONE_FINAL, 0xe0, 0); | ||
| } | ||
| else | ||
| SendCursorResponse(req); |
There was a problem hiding this comment.
This basically states that we never tested this SP_CURSORPREAPRE code? how are we ensuring that we have enough test cases for this now?
| @@ -1989,41 +1989,45 @@ TdsProcessLogin(Port *port, bool loadedSsl) | |||
| TdsErrorContext->phase = 0; | |||
| TdsErrorContext->reqType = TDS_LOGIN7; | |||
|
|
|||
| PG_TRY(); | |||
| if (!TdsReadNextBuffer() && TdsCheckMessageType(TDS_PRELOGIN)) | |||
There was a problem hiding this comment.
we can have more safer check by checking whether client has ssl=off. If that's the case then only we should skip pre-login msg. Throw an FATAL error otherwise and should terminate connection
| InlineCodeBlockArgs *create_args(int numargs); | ||
| void read_param_def(InlineCodeBlockArgs *args, const char *paramdefstr); |
| @@ -2051,10 +2083,10 @@ PrepareRowDescription(TupleDesc typeinfo, PlannedStmt *plannedstmt, List *target | |||
| /* | |||
| * If client being connected is using TDS version lower | |||
| * than 7.3A then TSQL treats DATETIMEOFFSET as NVARCHAR. | |||
| * Max len here would be 64('YYYY-MM-DD hh:mm:ss[.nnnnnnn] | |||
| * Max len here would be 68('YYYY-MM-DD hh:mm:ss[.nnnnnnn] | |||
| char* | ||
| TdsTimeGetTimeAsString(TimeADT value, int scale) | ||
| { | ||
| struct pg_tm tt, | ||
| *tm = &tt; | ||
| fsec_t fsec; | ||
| char *buf; | ||
|
|
||
| memset(tm, '\0', sizeof(struct pg_tm)); | ||
| time2tm(value, tm, &fsec); | ||
|
|
||
| buf = palloc0(16 + 1); | ||
| if (!pg_strftime(buf, 10 + 1, "%H:%M:%S", tm)) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), | ||
| errmsg("date out of range"))); | ||
|
|
||
| AppendFractionalSeconds(buf, fsec, scale); | ||
|
|
||
| return buf; | ||
| } |
There was a problem hiding this comment.
Please explain why is this needed instead of directly calling time_out? Worth adding comment explaining the same. Only thing I could make out this is to match the provided scale by appending extra zeros which can be very well achieved by appending zeros to output of time_out. What am I missing?
| buf[idx] = '0'; | ||
| } | ||
|
|
||
| for (idx = 0; idx < scale; idx++) |
There was a problem hiding this comment.
If column is defined with TIME(7) then atttypmod = -1 and hence scale will also be -1. we will never ever get into this loop. But this part of code comes into picture only for TDS client with version < 7.3 for which we always need to set scale to 7 (DATETIMEOFFSETMAXSCALE) if stored attypmod is -1. Our defined buf is also not able to handle this length 7.
I am little bit more worried now in terms of how are testing this code, if existing test coverage good enough to save some seg fault. I am still inclined towards reusing time_out and either append zeros or truncate if required. Truncate will never be needed though.
| @@ -3355,6 +3411,33 @@ TdsSendTypeTime(FmgrInfo *finfo, Datum value, void *vMetaData) | |||
| return rc; | |||
| } | |||
|
|
|||
| static int | |||
| TdsSendDatetime2AsNVarcharHelper(FmgrInfo *finfo, Datum value, void *vMetaData) | |||
There was a problem hiding this comment.
Same as TdsSendTimeAsNVarcharHelper, please evaluate if we really need to do this?
| @@ -4155,6 +4325,33 @@ TdsRecvTypeDatetimeoffset(const char *message, const ParameterToken token) | |||
| return result; | |||
| } | |||
|
|
|||
| static int | |||
| TdsSendDatetimeoffsetAsNVarcharHelper(FmgrInfo *finfo, Datum value, void *vMetaData) | |||
There was a problem hiding this comment.
please reconsider if this is really needed?
| /* | ||
| * dataformat: totalLen(4B) + baseType(1B) + metadatalen(1B) + | ||
| * encodingLen(5B) + dataLen(2B) + data(dataLen) | ||
| */ | ||
| static int | ||
| TdsSendTypeSqlvariantAsNVarcharHelper(const char *st) |
There was a problem hiding this comment.
this can cause confusion with regular nvarchar sender for sql_variant datatype. Please add comment stating why is it needed
There was a problem hiding this comment.
Most of this code shares common code with TdsSendTypeSqlvariant to send string data, please consider if we can move this into some common helper code. It will help maintain code in long term
|
@kuntalghosh , @Deepesh125 , thanks for the reviews! I've filed #2860 with the PRELOGIN change and copied review comments there. Going to follow up with JDBC test framework changes. |
|
Submitted test harness changes in #2861. |
|
Just FYI, I will be able to respond to review comments and follow up with cursor and datatype changes in about 2 weeks. |
|
@staticlibs, what support would help you best to move this forward? |
Description
This patch contains initial working implementation of JTDS support. With it applied, Babelfish DB can be opened and browsed from DBeaver (using "MS SQL Server / SQL Server (Old driver, jTDS)" connection setup included there). Both
ssl=offandssl=requiremodes are supported.Two problems with JTDS were identified:
no
PRELOGINrequest sent when SSL is offBabelfsh uses Partially Length-Prefixed bytes (PLP) [1] for returning
varcharandnvarcharfields. PLP was introduced in TDS 7.2, but JTDS only supports TDS 7.1Patch adds a message type check before reading
PRELOGINand a TDS version check to decide whether to use PLP responses.Known shortcomings of the patch:
no test runs with JTDS driver (would appreciate pointers how to introduce them, perhaps run a subset of existing JDBC tests with JTDS?)
not clear whether the patch covers all necessary PLP cases (should Spatial responses be changed too?)
[1] https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-tds/3f983fde-0509-485a-8c40-a9fa6679a828
Issues Resolved
#2137
Check List
By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.