-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JTDS Driver support #2320
base: BABEL_4_X_DEV
Are you sure you want to change the base?
JTDS Driver support #2320
Changes from all commits
464cf60
a440eb6
2e9ae44
c14b475
7cbb614
5f76a0a
8e0290d
88cb3a0
a7a0be1
e7cdc92
246b750
d1e255c
ce0a624
693a45f
610d03c
a3b0608
3e227e0
1490752
11390b9
5b862b0
c3affa7
617583a
f4287b7
01f3991
99287cb
b4b5c0b
9c84754
63e262a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,175 @@ | ||
name: JDBC Tests with JTDS | ||
on: [push, pull_request] | ||
|
||
jobs: | ||
run-babelfish-jdbc-tests: | ||
env: | ||
INSTALL_DIR: psql | ||
useJTDSInsteadOfMSSQLJDBC: "true" | ||
runs-on: ubuntu-20.04 | ||
steps: | ||
- uses: actions/checkout@v2 | ||
id: checkout | ||
|
||
- name: Install Dependencies | ||
id: install-dependencies | ||
if: always() | ||
uses: ./.github/composite-actions/install-dependencies | ||
|
||
- name: Build Modified Postgres | ||
id: build-modified-postgres | ||
if: always() && steps.install-dependencies.outcome == 'success' | ||
uses: ./.github/composite-actions/build-modified-postgres | ||
with: | ||
install_dir: 'psql' | ||
code_coverage: 'yes' | ||
|
||
- name: Compile ANTLR | ||
id: compile-antlr | ||
if: always() && steps.build-modified-postgres.outcome == 'success' | ||
uses: ./.github/composite-actions/compile-antlr | ||
|
||
- name: Build Extensions | ||
id: build-extensions | ||
if: always() && steps.compile-antlr.outcome == 'success' | ||
uses: ./.github/composite-actions/build-extensions | ||
|
||
- name: Build tds_fdw Extension | ||
id: build-tds_fdw-extension | ||
if: always() && steps.build-extensions.outcome == 'success' | ||
uses: ./.github/composite-actions/build-tds_fdw-extension | ||
|
||
- name: Build vector Extension | ||
id: build-vector-extension | ||
if: always() && steps.build-tds_fdw-extension.outcome == 'success' | ||
uses: ./.github/composite-actions/build-vector-extension | ||
|
||
- name: Build PostGIS Extension | ||
id: build-postgis-extension | ||
if: always() && steps.build-vector-extension.outcome == 'success' | ||
uses: ./.github/composite-actions/build-postgis-extension | ||
|
||
- name: Install Extensions | ||
id: install-extensions | ||
if: always() && steps.build-postgis-extension.outcome == 'success' | ||
uses: ./.github/composite-actions/install-extensions | ||
with: | ||
wal_level: logical | ||
|
||
- name: Run JDBC Tests | ||
id: jdbc | ||
if: always() && steps.install-extensions.outcome == 'success' | ||
timeout-minutes: 60 | ||
uses: ./.github/composite-actions/run-jdbc-tests | ||
|
||
- name: Start secondary server | ||
id: start-secondary | ||
if: always() && steps.jdbc.outcome == 'success' | ||
uses: ./.github/composite-actions/install-extensions | ||
with: | ||
psql_port: 5433 | ||
tsql_port: 8199 | ||
wal_level: logical | ||
|
||
- name: Setup Publication and Subscription | ||
id: setup-pub-sub | ||
if: always() && steps.start-secondary.outcome == 'success' | ||
run: | | ||
~/${{env.INSTALL_DIR}}/bin/psql -v ON_ERROR_STOP=1 -d jdbc_testdb -U runner -c "CREATE PUBLICATION my_pub;" | ||
~/${{env.INSTALL_DIR}}/bin/psql -v ON_ERROR_STOP=1 -d jdbc_testdb -U runner -p 5433 -c "CREATE SUBSCRIPTION my_sub CONNECTION 'host=localhost port=5432 user=jdbc_user dbname=jdbc_testdb password=12345678' PUBLICATION my_pub;" | ||
|
||
- name: Run Replication Tests | ||
id: replication | ||
if: always() && steps.setup-pub-sub.outcome == 'success' | ||
timeout-minutes: 60 | ||
uses: ./.github/composite-actions/run-jdbc-tests | ||
with: | ||
input_dir: 'replication' | ||
|
||
- name: Cleanup babelfish database | ||
id: cleanup | ||
if: always() && steps.replication.outcome == 'success' | ||
run: | | ||
sudo ~/psql/bin/psql -d postgres -U runner -v user="jdbc_user" -v db="jdbc_testdb" -f .github/scripts/cleanup_babelfish_database.sql | ||
|
||
- name: Upload Log | ||
if: always() && (steps.jdbc.outcome == 'failure' || steps.replication.outcome == 'failure') | ||
uses: actions/upload-artifact@v2 | ||
with: | ||
name: postgres-log-jdbc | ||
path: | | ||
~/psql/data/logfile | ||
~/psql/data_5433/logfile | ||
|
||
# The test summary files contain paths with ':' characters, which is not allowed with the upload-artifact actions | ||
- name: Rename Test Summary Files | ||
id: test-file-rename | ||
if: always() && (steps.jdbc.outcome == 'failure' || steps.replication.outcome == 'failure') | ||
run: | | ||
cd test/JDBC/Info | ||
timestamp=`ls -Art | tail -n 1` | ||
cd $timestamp | ||
mv $timestamp.diff ../output-diff.diff | ||
mv "$timestamp"_runSummary.log ../run-summary.log | ||
cd .. | ||
# get the replication output diff as well if it is present | ||
dir_count=`ls | wc -l` | ||
if [[ $dir_count -eq 2 ]];then | ||
timestamp=`ls -rt | tail -n 2 | sort -r | tail -n 1` | ||
cd $timestamp | ||
mv $timestamp.diff ../replication-output-diff.diff | ||
mv "$timestamp"_runSummary.log ../replication-run-summary.log | ||
|
||
- name: Upload Run Summary | ||
if: always() && steps.test-file-rename.outcome == 'success' | ||
uses: actions/upload-artifact@v2 | ||
with: | ||
name: run-summary.log | ||
path: | | ||
test/JDBC/Info/run-summary.log | ||
test/JDBC/Info/replication-run-summary.log | ||
|
||
- name: Upload Output Diff | ||
if: always() && (steps.jdbc.outcome == 'failure' || steps.replication.outcome == 'failure') | ||
uses: actions/upload-artifact@v2 | ||
with: | ||
name: jdbc-output-diff.diff | ||
path: | | ||
test/JDBC/Info/output-diff.diff | ||
test/JDBC/Info/replication-output-diff.diff | ||
|
||
- name: Check and upload coredumps | ||
if: always() && (steps.jdbc.outcome == 'failure' || steps.replication.outcome == 'failure') | ||
uses: ./.github/composite-actions/upload-coredump | ||
|
||
- name: Generate Code Coverage | ||
id: generate-code-coverage | ||
if: always() && (steps.jdbc.outcome == 'success' && steps.replication.outcome == 'success') | ||
run: | | ||
export PG_CONFIG=~/psql/bin/pg_config | ||
export PG_SRC=~/work/postgresql_modified_for_babelfish | ||
export cmake=$(which cmake) | ||
cd contrib | ||
for ext in babelfishpg_common babelfishpg_money babelfishpg_tds babelfishpg_tsql | ||
do | ||
cd $ext | ||
/usr/bin/lcov --gcov-tool /usr/bin/gcov -q --no-external -c -d . -d ./ -o lcov_test.info | ||
cd .. | ||
done | ||
shell: bash | ||
|
||
- name: Summarize code coverage | ||
id: code-coverage-summary | ||
if: always() && steps.generate-code-coverage.outcome == 'success' | ||
run: | | ||
cd contrib/ | ||
lcov -a babelfishpg_tsql/lcov_test.info -a babelfishpg_tds/lcov_test.info -a babelfishpg_common/lcov_test.info -a babelfishpg_money/lcov_test.info -o jdbc-lcov.info | ||
lcov --list jdbc-lcov.info | ||
|
||
- name: Upload Coverage Report for Babelfish Extensions | ||
if: always() && steps.code-coverage-summary.outcome == 'success' | ||
uses: actions/upload-artifact@v3 | ||
with: | ||
name: coverage-babelfish-extensions-jdbc | ||
path: contrib/jdbc-lcov.info | ||
retention-days: 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually, let's not modify the SSL code at all. Something like as following (after confirming the SSL behavior):
|
||
{ | ||
TdsErrorContext->err_text = "Parsing PreLogin Request"; | ||
/* Pre-Login */ | ||
rc = ParsePreLoginRequest(); | ||
PG_TRY(); | ||
{ | ||
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(); | ||
|
||
TdsErrorContext->err_text = "Setup SSL Handshake"; | ||
/* Setup the SSL handshake */ | ||
if (loadEncryption == TDS_ENCRYPT_ON || | ||
loadEncryption == TDS_ENCRYPT_OFF || | ||
loadEncryption == TDS_ENCRYPT_REQ) | ||
rc = SecureOpenServer(port); | ||
} | ||
PG_CATCH(); | ||
{ | ||
PG_RE_THROW(); | ||
} | ||
PG_END_TRY(); | ||
|
||
/* | ||
* If SSL handshake failure has occurred then no need to go ahead with | ||
* login, Just return from here. | ||
*/ | ||
if (rc < 0) | ||
return rc; | ||
|
||
TdsErrorContext->err_text = "Make PreLogin Response"; | ||
if (loadEncryption == TDS_ENCRYPT_ON) | ||
TDSInstrumentation(INSTR_TDS_LOGIN_END_TO_END_ENCRYPT); | ||
|
||
loadEncryption = MakePreLoginResponse(port, loadedSsl); | ||
TdsFlush(); | ||
|
||
TdsErrorContext->err_text = "Setup SSL Handshake"; | ||
/* Setup the SSL handshake */ | ||
if (loadEncryption == TDS_ENCRYPT_ON || | ||
loadEncryption == TDS_ENCRYPT_OFF || | ||
loadEncryption == TDS_ENCRYPT_REQ) | ||
rc = SecureOpenServer(port); | ||
} | ||
PG_CATCH(); | ||
{ | ||
PG_RE_THROW(); | ||
} | ||
PG_END_TRY(); | ||
|
||
/* | ||
* If SSL handshake failure has occurred then no need to go ahead with | ||
* login, Just return from here. | ||
*/ | ||
if (rc < 0) | ||
return rc; | ||
|
||
if (loadEncryption == TDS_ENCRYPT_ON) | ||
TDSInstrumentation(INSTR_TDS_LOGIN_END_TO_END_ENCRYPT); | ||
|
||
PG_TRY(); | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1785,6 +1785,7 @@ PrepareRowDescription(TupleDesc typeinfo, PlannedStmt *plannedstmt, List *target | |
} | ||
|
||
SetAttributesForColmetada(col); | ||
col->atttypmod = atttypmod; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? The actual typmod is calculated based on the datatype. Can we rely on this value? |
||
|
||
switch (finfo->sendFuncId) | ||
{ | ||
|
@@ -1855,15 +1856,36 @@ PrepareRowDescription(TupleDesc typeinfo, PlannedStmt *plannedstmt, List *target | |
/* If this is vector datatype, we should adjust the typmod */ | ||
if (is_sys_vector_datatype(col->pgTypeOid)) | ||
atttypmod = -1; | ||
|
||
SetColMetadataForCharTypeHelper(col, TDS_TYPE_VARCHAR, | ||
att->attcollation, (atttypmod == -1) ? | ||
atttypmod : (atttypmod - 4)); | ||
/* | ||
* If client being connected is using TDS version lower than or equal to | ||
* 7.1 then TSQL treats varchar(max) as Text. | ||
*/ | ||
if (tdsVersion <= TDS_VERSION_7_1_1 && atttypmod == -1) | ||
{ | ||
SetColMetadataForTextTypeHelper(col, TDS_TYPE_TEXT, | ||
att->attcollation, (atttypmod - 4)); | ||
sendTableName |= col->sendTableName; | ||
} | ||
else | ||
SetColMetadataForCharTypeHelper(col, TDS_TYPE_VARCHAR, | ||
att->attcollation, (atttypmod == -1) ? | ||
atttypmod : (atttypmod - 4)); | ||
break; | ||
case TDS_SEND_NVARCHAR: | ||
SetColMetadataForCharTypeHelper(col, TDS_TYPE_NVARCHAR, | ||
att->attcollation, (atttypmod == -1) ? | ||
atttypmod : (atttypmod - 4) * 2); | ||
/* | ||
* If client being connected is using TDS version lower than or equal to | ||
* 7.1 then TSQL treats nvarchar(max) as NText. | ||
*/ | ||
if (tdsVersion <= TDS_VERSION_7_1_1 && atttypmod == -1) | ||
{ | ||
SetColMetadataForTextTypeHelper(col, TDS_TYPE_NTEXT, | ||
att->attcollation, (atttypmod - 4) * 2); | ||
sendTableName |= col->sendTableName; | ||
} | ||
else | ||
SetColMetadataForCharTypeHelper(col, TDS_TYPE_NVARCHAR, | ||
att->attcollation, (atttypmod == -1) ? | ||
atttypmod : (atttypmod - 4) * 2); | ||
break; | ||
case TDS_SEND_MONEY: | ||
if (col->attNotNull) | ||
|
@@ -1976,8 +1998,18 @@ PrepareRowDescription(TupleDesc typeinfo, PlannedStmt *plannedstmt, List *target | |
case TDS_SEND_VARBINARY: | ||
if (atttypmod == -1 && tle != NULL) | ||
atttypmod = resolve_varbinary_typmod_from_exp((Node *) tle->expr); | ||
SetColMetadataForBinaryType(col, TDS_TYPE_VARBINARY, (atttypmod == -1) ? | ||
atttypmod : atttypmod - VARHDRSZ); | ||
/* | ||
* If client being connected is using TDS version lower than or equal to | ||
* 7.1 then TSQL treats varbinary(max) as Image. | ||
*/ | ||
if (tdsVersion <= TDS_VERSION_7_1_1 && atttypmod == -1) | ||
{ | ||
SetColMetadataForImageType(col, TDS_TYPE_IMAGE); | ||
sendTableName |= col->sendTableName; | ||
} | ||
else | ||
SetColMetadataForBinaryType(col, TDS_TYPE_VARBINARY, (atttypmod == -1) ? | ||
atttypmod : atttypmod - VARHDRSZ); | ||
break; | ||
case TDS_SEND_UNIQUEIDENTIFIER: | ||
SetColMetadataForFixedType(col, TDS_TYPE_UNIQUEIDENTIFIER, TDS_MAXLEN_UNIQUEIDENTIFIER); | ||
|
@@ -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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch |
||
* [+|-]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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have we added a testcase for this with TDS version lower than 7.3A? |
||
else | ||
{ | ||
if (atttypmod == -1) | ||
|
@@ -2362,6 +2394,42 @@ SendReturnValueTokenInternal(ParameterToken token, uint8 status, | |
(token->paramMeta.sendFunc) (finfo, datum, (void *) &token->paramMeta); | ||
} | ||
|
||
/* | ||
* SendReturnValueIntInternal | ||
* | ||
* status - stored procedure (0x01) or UDF (0x02) | ||
*/ | ||
Comment on lines
+2397
to
+2401
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add comment explaining usage of this function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and should define macro for the constant |
||
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); | ||
} | ||
|
||
Comment on lines
+2397
to
+2432
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we not using SendReturnValueTokenInternal()? We can create an empty parameter token using MakeEmptyParameterToken() and call the same function -
|
||
int | ||
GetTypModForToken(ParameterToken token) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Each PR should be backed by respective testcases instead of putting all the test case in a single PR.