Skip to content
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.

Sign up for GitHub

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

Closing opened connections #116

Merged
merged 14 commits into from
Feb 17, 2020
+51 −18
Merged

Closing opened connections #116

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0f3810d
Closing opened connections
manmorjim Dec 20, 2019
9c41443
Update NEWS
manmorjim Dec 23, 2019
ac86607
Close opened connections
manmorjim Dec 26, 2019
581cb4a
Removing handlers NULL assignations
manmorjim Jan 8, 2020
a1119ee
Closing connection after sending DONE
manmorjim Jan 8, 2020
0d21f00
Checking disconnect errors
manmorjim Jan 24, 2020
45c2d69
Managing disconnection correctly
manmorjim Jan 24, 2020
e2fb4ba
Clean up connection messages
manmorjim Jan 29, 2020
59b187d
Merge branch 'master' into fix_close_connections
jgoizueta Feb 17, 2020
56de118
Update NEWS
jgoizueta Feb 17, 2020
efd8ba1
Start MySQL before updating it
jgoizueta Feb 17, 2020
a0a8a43
Remove mysql from apt addon installation
jgoizueta Feb 17, 2020
44b530f
Change MySQL root authentication
jgoizueta Feb 17, 2020
49cbb5b
Merge pull request #120 from CartoDB/debug-travis
jgoizueta Feb 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
Released XXXX-XX-XX

Changes:
- Fixes #96 by closing connections (https://github.com/CartoDB/odbc_fdw/pull/116).
- Update CI dependencies (https://github.com/CartoDB/odbc_fdw/pull/102).
- PG 12 compatibility (https://github.com/CartoDB/odbc_fdw/pull/104).

Expand Down
48 changes: 34 additions & 14 deletions odbc_fdw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ typedef struct odbcFdwExecutionState
{
AttInMetadata *attinmeta;
odbcFdwOptions options;
SQLHENV env;
SQLHDBC dbc;
SQLHSTMT stmt;
int num_of_result_cols;
int num_of_table_cols;
Expand Down Expand Up @@ -214,6 +216,7 @@ static void extract_odbcFdwOptions(List *options_list, odbcFdwOptions *extracted
static void init_odbcFdwOptions(odbcFdwOptions* options);
static void copy_odbcFdwOptions(odbcFdwOptions* to, odbcFdwOptions* from);
static void odbc_connection(odbcFdwOptions* options, SQLHENV *env, SQLHDBC *dbc);
static void odbc_disconnection(SQLHENV *env, SQLHDBC *dbc);
static void sql_data_type(SQLSMALLINT odbc_data_type, SQLULEN column_size, SQLSMALLINT decimal_digits, SQLSMALLINT nullable, StringInfo sql_type);
static void odbcGetOptions(Oid server_oid, List *add_options, odbcFdwOptions *extracted_options);
static void odbcGetTableOptions(Oid foreigntableid, odbcFdwOptions *extracted_options);
Expand Down Expand Up @@ -414,6 +417,27 @@ odbc_connection(odbcFdwOptions* options, SQLHENV *env, SQLHDBC *dbc)
ret = SQLDriverConnect(*dbc, NULL, (SQLCHAR *) conn_str.data, SQL_NTS,
OutConnStr, 1024, &OutConnStrLen, SQL_DRIVER_COMPLETE);
check_return(ret, "Connecting to driver", dbc, SQL_HANDLE_DBC);
elog_debug("connection opened >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>");
manmorjim marked this conversation as resolved.
Show resolved Hide resolved
}

/*
* Close the ODBC connection
*/
static void
odbc_disconnection(SQLHENV *env, SQLHDBC *dbc)
{
if (dbc && *dbc)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is suspicious and likely what's causing crashes on Windows builds. You are setting the contents of dbc to NULL, but is SQLHDBC declared as a NULLable type in the ODBC standard (I'm guessing it comes from there)?

In any case, the fact that we are asking ourselves this question means that this approach is wrong. Either always use a pointer or ensure you only call the disconnect once so you don't need the NULL guards.

{
SQLFreeHandle(SQL_HANDLE_DBC, *dbc);
SQLDisconnect(*dbc);
*dbc = NULL;
}
if (env && *env)
{
SQLFreeHandle(SQL_HANDLE_ENV, *env);
*env = NULL;
}
elog_debug("connection closed <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<");
}

/*
Expand Down Expand Up @@ -893,18 +917,7 @@ odbcGetTableSize(odbcFdwOptions* options, unsigned int *size)
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
stmt = NULL;
}
if (dbc)
{
SQLFreeHandle(SQL_HANDLE_DBC, dbc);
dbc = NULL;
}
if (env)
{
SQLFreeHandle(SQL_HANDLE_ENV, env);
env = NULL;
}
if (dbc)
SQLDisconnect(dbc);
odbc_disconnection(&env, &dbc);
}

static int strtoint(const char *nptr, char **endptr, int base)
Expand Down Expand Up @@ -1105,6 +1118,7 @@ Datum odbc_tables_list(PG_FUNCTION_ARGS)
datafctx->currentRow = currentRow;
SRF_RETURN_NEXT(funcctx, result);
} else {
odbc_disconnection(&env, &dbc);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will never be called since SRF_RETURN_DONE will return from the function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. This function uses SRF and I was trying to disconnect it using wrong handlers. I stored these handlers into the user_fctx and using them to disconnect.

SRF_RETURN_DONE(funcctx);
}
}
Expand Down Expand Up @@ -1466,6 +1480,8 @@ odbcBeginForeignScan(ForeignScanState *node, int eflags)
festate = (odbcFdwExecutionState *) palloc(sizeof(odbcFdwExecutionState));
festate->attinmeta = TupleDescGetAttInMetadata(node->ss.ss_currentRelation->rd_att);
copy_odbcFdwOptions(&(festate->options), &options);
festate->env = env;
festate->dbc = dbc;
festate->stmt = stmt;
festate->table_columns = columns;
festate->num_of_table_cols = num_of_columns;
Expand Down Expand Up @@ -1825,6 +1841,7 @@ odbcEndForeignScan(ForeignScanState *node)
SQLFreeHandle(SQL_HANDLE_STMT, festate->stmt);
festate->stmt = NULL;
}
odbc_disconnection(&festate->env, &festate->dbc);
}
}

Expand Down Expand Up @@ -1977,6 +1994,7 @@ odbcImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
}
SQLCloseCursor(query_stmt);
SQLFreeHandle(SQL_HANDLE_STMT, query_stmt);
odbc_disconnection(&env, &dbc);

tables = lappend(tables, (void*)options.table);
table_columns = lappend(table_columns, (void*)col_str.data);
Expand Down Expand Up @@ -2075,6 +2093,7 @@ odbcImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
SQLCloseCursor(tables_stmt);

SQLFreeHandle(SQL_HANDLE_STMT, tables_stmt);
odbc_disconnection(&env, &dbc);
}
else if (stmt->list_type == FDW_IMPORT_SCHEMA_LIMIT_TO)
{
Expand All @@ -2088,12 +2107,12 @@ odbcImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
{
elog(ERROR,"Unknown list type in IMPORT FOREIGN SCHEMA");
}

odbc_connection(&options, &env, &dbc);
foreach(tables_cell, tables)
{
char *table_name = (char*)lfirst(tables_cell);

odbc_connection(&options, &env, &dbc);

/* Allocate a statement handle */
SQLAllocHandle(SQL_HANDLE_STMT, dbc, &columns_stmt);

Expand Down Expand Up @@ -2141,6 +2160,7 @@ odbcImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
SQLFreeHandle(SQL_HANDLE_STMT, columns_stmt);
table_columns = lappend(table_columns, (void*)col_str.data);
}
odbc_disconnection(&env, &dbc);
}

/* Generate create statements */
Expand Down